[BugFest] Feature request #38685: str_[i]replace(): Add support for (string needle, array replace) View as Markdown

Hi!

usually each BC break would warrant an RFC and a vote, but I'm not sure
many people would use str_replace("foo", array("bar"), $subject); and
expect the implicit array to string conversion of array("bar") => "Array"
which also emits a notice.
let's see what others think.

I wouldn't consider this a BC break. Anybody who relies on implicit
array->string conversion has a bug anyway, and we allow that implicit
conversion to survive only for legacy reasons, but I don't think it
should drag behind itself the promise we'd never support array parameter
where there was only string before. In summary, I'd say no BC break here.

Stas Malyshev
smalyshev@gmail.com

De : François Laupretre [mailto:francois@tekwire.net]

I just created the RFC :

https://wiki.php.net/rfc/cyclic-replace

Just added full array multi-level recursion support to the RFC.

Final (?) patch pushed to PR. In sync with RFC.

Please also give me your opinion about implementing a similar feature in
preg_replace() and preg_filter().

No opnion here ?????

Do we need to wait 2 weeks to start voting ? It seems no one has anything
more to say.

Hi,

But if I see this (taken from the tests) as the search & replacement values...

$search=array('[?]',array('(?)','d','e'),'a','R');
$repl=array(
array('ap(?)z[?]',"b?v",null,37,"[?]n[?]"),
array('a',array('b',null)),
array('k(?)j[?]')
);

... I have a pretty hard time figuring out what this is actually supposed
to do. At this point str_replace is working on arbitrarily-nested arrays
and also has four options to control its behavior.

Imho this is just too much complexity for a relatively minor use case.

I have to agree here. Nested arrays are too much ... it should be
limited to single-dimention arrays.

Cheers,
Andrey.

De : Nikita Popov [mailto:nikita.ppv@gmail.com]

str_replace is a simple function for string replacement. It has some (imho also unnecessary)
extra gimmicks to implicitly do a foreach() loop during the replacement. This is still relatively
simple API-wise. But if I see this (taken from the tests) as the search & replacement values...

$search=array('[?]',array('(?)','d','e'),'a','R');
$repl=array(
array('ap(?)z[?]',"b?v",null,37,"[?]n[?]"),
array('a',array('b',null)),
array('k(?)j[?]')
);
... I have a pretty hard time figuring out what this is actually supposed to do.

This is not a real use case, this is edge-case testing !

At this point str_replace is working on arbitrarily-nested arrays and also has four options to control its behavior.
Imho this is just too much complexity for a relatively minor use case.

Let's take this one by one :

First, this all started with two feature requests for this feature. I then asked on the list what people thought about it and the return what 100% positive.

So, I started a first implementation to just allow cyclic replace with a loop behavior. At this time, an empty replace array was considered as an empty string, with no warning.

Then R. Quadling complained that we should provide a way to control what happens when we arrive at the end of the replace array. I hadn't done it because I didn't want to change the API. We finally agreed on an additional options arg with five possible values. I don't like the additional arg but I accept it because : 1. It is added as last arg and most people will never see it, and 2. it can be useful if we need additional flags in the future. For instance, if we had defined this arg at the beginning, it could have handled case-sensitivity, which wouldn't have been worse than writing a sister function. I even think now to define a STR_REPLACE_CASE_INSENSITIVE flag as an alternative to str_ireplace(). This would provide all str_[i]replace features in a single function (don't worry, the BC break is too huge to deprecate str_ireplace()).

Then, you asked for an error on empty replace array. I added that.

Then, it came to my mind that, if we accept the combination of (string search, array replace).we must be consistent and, as we already accept (array search), we must also accept ((array of arrays) replace). The extension to arbitrarily-nested arrays is not essential but it didn't cost much and allowed for a cleaner multi-layered C design. Honestly, I hope we keep supporting it because removing it would add complexity in the code but I am not sure we must document recursion support beyond 1 level for search / 2 levels for replace (even if side notes will quickly document it). I agree that supporting (array search) may be a wrong decision (more complexity, just for performance reasons), but it was made a long time ago and a lot of people use it now...

I then thought that, as I had (quite useless) recursion for search/replace, the least I could do was to provide the same for subject because, here, it is really valuable. It is a quite natural use case to want converting every strings in an arbitrarily-nested array and I imagine people will quickly use this. As before, it also brings a cleaner design in C code.

To summarize, I started with a rather simple feature request and was then pushed by external wishes. About added complexity in code, the summary is about 100 lines of new (cruelly missing) comments, 100 lines added, and 130 lines modified. The rest comes from additional tests (which could be simpler, ok, ok). If you consider that the design is now simpler to understand, this is not so bad.

So, all in all, I think that the whole balance is positive, even if adding an argument is never wanted.

That said, I am always open to discussion and will never tell you that it must remain so because I have spent endless hours coding it :). Finally, a vote will take place starting January,20, which will probably require 2/3 of voices (not sure because BC break is very minimal, but we are adding an argument...).

Sorry for posting in the same thread. I will post a new [RFC]-prefixed message.

If you want your arguments to be more visible to newcomers, you may post them in the PR. If you do it, I will then post my reply.

Regards

François