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

- Thread: https://externals.io/message/80185
- Original archive: https://news-web.php.net/php.internals/80185
- Started by F & N Laupretre on 2015-01-05T17:15:00+00:00
- Messages: 19
- Reply by email to internals@lists.php.net
- More for agents: search with https://externals.io/search.md?q={query} · JSON API at https://externals.io/api · documentation at https://externals.io/llms.txt

---

## #80185 — F & N Laupretre — 2015-01-05T17:15:00+00:00

URL: https://externals.io/message/80185#80185

Hi,

I just created a PR for a feature request lying around since 2006 :

https://github.com/php/php-src/pull/980

It adds support in `str_replace()` and `str_ireplace()` for the combination of
(string needle, array replace). In this case, each occurrence of the needle
is replaced with an element f the 'replace' array, looping if count(replace)
is lower than count(needles) in subject. It is interesting for cyclic
replacements (alternating line colors, for instance). The PR includes the
needed tests.

As the BC break is minimal, do you think it can be merged to the PHP7 branch
or do I need to write an RFC ?

Regards,

François

---

## #80187 — Andrea Faulds — 2015-01-05T17:20:52+00:00

URL: https://externals.io/message/80185#80187 · In reply to: #80185

Hey François,

> 
> 
> I just created a PR for a feature request lying around since 2006 :
> 
> https://github.com/php/php-src/pull/980
> 
> It adds support in `str_replace()` and `str_ireplace()` for the combination of
> (string needle, array replace). In this case, each occurrence of the needle
> is replaced with an element f the 'replace' array, looping if count(replace)
> is lower than count(needles) in subject. It is interesting for cyclic
> replacements (alternating line colors, for instance). The PR includes the
> needed tests.

This would have been really useful for me quite recently, I was disappointed to see it doesn’t already work. Please go ahead with this!

Thanks.
--
Andrea Faulds
http://ajf.me/

---

## #80191 — Ferenc Kovacs — 2015-01-05T18:06:38+00:00

URL: https://externals.io/message/80185#80191 · In reply to: #80187

> Hey François,
>
> > […]
> of
> > […]
> needle
> > […]
> count(replace)
> > […]
>
> This would have been really useful for me quite recently, I was
> disappointed to see it doesn’t already work. Please go ahead with this!
>
> Thanks.
> --
> Andrea Faulds
> http://ajf.me/
>
>
>
>
>
> --
> 
> 
>
>
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.


-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #80197 — Stanislav Malyshev — 2015-01-05T20:28:14+00:00

URL: https://externals.io/message/80185#80197 · In reply to: #80191

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

---

## #80198 — Andrea Faulds — 2015-01-05T20:30:21+00:00

URL: https://externals.io/message/80185#80198 · In reply to: #80197

> 
> 
> Hi!
> 
> > […]
> 
> 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.

To add to what Stas says: ZPP doesn’t allow array->string anyway, this is just a quirk in str_replace’s handling of its ‘z’ parameter. So users wouldn’t expect it to work anyway, right?

--
Andrea Faulds
http://ajf.me/

---

## #80202 — Ferenc Kovacs — 2015-01-05T22:03:05+00:00

URL: https://externals.io/message/80185#80202 · In reply to: #80197

On Mon, Jan 5, 2015 at 9:28 PM, Stanislav Malyshev <smalyshev@gmail.com>
wrote:

> Hi!
>
> > […]
>
> 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
>

depends on one's definition of bc. mine is that changing anything which
isn't explicitly documented otherwise or as as undefined behavior is a bc
break.
as I mentioned I agree that there should be no otherwise correct code which
depends on this behavior (yet we have seen people complaning about BC break
when we originally introduced the notice on implicit array -> string cast
in php 5.4.0) so I would possibly vote yes on this.

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #80203 — Ferenc Kovacs — 2015-01-05T22:10:25+00:00

URL: https://externals.io/message/80185#80203 · In reply to: #80198

>
> > […]
> "Array"
> > […]
>
> To add to what Stas says: ZPP doesn’t allow array->string anyway, this is
> just a quirk in str_replace’s handling of its ‘z’ parameter. So users
> wouldn’t expect it to work anyway, right?
>

users don't know much about how zend parameter parsing works in general,
and both $search and $replace is documented as mixed plus the only
reference about this usage in the documentation is the "The converse would
not make sense, though. " sentence which can be easy to miss.
So while I agree that the current working doesn't make much sense, I don't
think that it is that obvious that this shouldn't behave as it does atm.

-- 
Ferenc Kovács
@Tyr43l - http://tyrael.hu

---

## #80292 — francois@tekwire.net — 2015-01-09T00:08:26+00:00

URL: https://externals.io/message/80185#80292 · In reply to: #80185

Hi,

I just created the RFC :

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

I tried to include everything we agreed upon. Tell me if I missed something.

Please note that the current PR does not implement the whole RFC. You may
review the RFC while I update the code.

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

Regards,

François

---

## #80306 — Andrey Andreev — 2015-01-09T12:12:04+00:00

URL: https://externals.io/message/80185#80306 · In reply to: #80292

Hi,

Just wanted to say: +1 on this and thank you for proposing a patch.

I myself created a feature request for it on bugs.php.net some time
ago, now closed as duplicate: https://bugs.php.net/bug.php?id=62315

Cheers,
Andrey.


> Hi,
>
> I just created the RFC :
>
> https://wiki.php.net/rfc/cyclic-replace
>
> I tried to include everything we agreed upon. Tell me if I missed something.
>
> Please note that the current PR does not implement the whole RFC. You may
> review the RFC while I update the code.
>
> Please also give me your opinion about implementing a similar feature in
> `preg_replace()` and `preg_filter()`.
>
> Regards,
>
> François

---

## #80307 — francois@tekwire.net — 2015-01-09T14:55:51+00:00

URL: https://externals.io/message/80185#80307 · In reply to: #80292

> 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.

Still debugging the code and adding new tests.

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

So ?????

---

## #80312 — francois@tekwire.net — 2015-01-10T01:34:53+00:00

URL: https://externals.io/message/80185#80312 · In reply to: #80307

> De : François Laupretre [mailto:francois@tekwire.net]
>
> > […]
> 
> Just added full array multi-level recursion support to the RFC.

Final (?) patch pushed to PR. In sync with RFC.
 
> > […]

No opnion here ?????

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

---

## #80317 — Pierre Joye — 2015-01-10T05:31:05+00:00

URL: https://externals.io/message/80185#80317 · In reply to: #80312

>
> > […]
>
> Final (?) patch pushed to PR. In sync with RFC.
>
> > […]
in
> > […]
>
> No opnion here ?????
>
> Do we need to wait 2 weeks to start voting ? It seems no one has anything
> more to say.

Yes please.

Even more now as some are still in vacations.

---

## #80320 — Nikita Popov — 2015-01-10T11:53:38+00:00

URL: https://externals.io/message/80185#80320 · In reply to: #80292

On Fri, Jan 9, 2015 at 1:08 AM, François Laupretre <francois@tekwire.net>
wrote:

> Hi,
>
> I just created the RFC :
>
> https://wiki.php.net/rfc/cyclic-replace
>
> I tried to include everything we agreed upon. Tell me if I missed
> something.
>
> Please note that the current PR does not implement the whole RFC. You may
> review the RFC while I update the code.
>
> Please also give me your opinion about implementing a similar feature in
> `preg_replace()` and `preg_filter()`.
>

When submitting an RFC, please open a separate thread. Otherwise people who
do not follow every single discussion will not see it.

I'm against this RFC. Not because I don't like the implementation or
similar, I simply don't consider this functionality to be sufficiently
important to warrant both the additional internal complexity it adds and
the complexity it adds to our user-facing API.

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. 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.

Nikita

---

## #80324 — Andrey Andreev — 2015-01-10T16:01:10+00:00

URL: https://externals.io/message/80185#80324 · In reply to: #80320

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.

---

## #80325 — francois@tekwire.net — 2015-01-10T17:03:27+00:00

URL: https://externals.io/message/80185#80325 · In reply to: #80320

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

---

## #80328 — francois@tekwire.net — 2015-01-10T17:27:59+00:00

URL: https://externals.io/message/80185#80328 · In reply to: #80324

Hi Andrey,

> De : Andrey Andreev [mailto:narf@devilix.net]
>
> I have to agree here. Nested arrays are too much ... it should be
> limited to single-dimension arrays.

It makes the C code simpler to understand. But I am OK not to document the feature for search and replace. I'd like to keep it for subject, because it makes sense there (cf my reply to Nikita).

Regards,

François

---

## #80330 — christopher jones — 2015-01-10T20:44:30+00:00

URL: https://externals.io/message/80185#80330 · In reply to: #80325

> To summarize, I started with a rather simple feature request and was
> then pushed by external wishes.

See #13 at
https://blogs.oracle.com/opal/entry/the_mysterious_php_rfc_process
"Take advice on board but make sure your feature doesn't end up being designed by a committee."


-- 
christopher.jones@oracle.com  http://twitter.com/ghrd

---

## #80331 — francois@tekwire.net — 2015-01-10T20:48:44+00:00

URL: https://externals.io/message/80185#80331 · In reply to: #80328

> De : François Laupretre [mailto:francois@tekwire.net]
> > […]
> 
> It makes the C code simpler to understand. But I am OK not to document the
> feature for search and replace.

Actually, supporting an undocumented feature is not a good idea. If you agree, I will limit search recursion to 1 level but keep arbitrarily-nested subject.

François

---

## #80332 — francois@tekwire.net — 2015-01-10T21:32:05+00:00

URL: https://externals.io/message/80185#80332 · In reply to: #80330

> De : christopher jones [mailto:christopher.jones@oracle.com]
>
> > […]
>
> See #13 at
> https://blogs.oracle.com/opal/entry/the_mysterious_php_rfc_process
> "Take advice on board but make sure your feature doesn't end up being
> designed by a committee."

I appreciate, really.

I see that I was not clear enough when I said that I was 'pushed by external wishes'. I must reassure you as  I am generally well-known to be quite 'unpushable' when the advices I receive don't fit my objectives. The whole challenge, here, as anybody (should) know, is to avoid denaturing your vision, while keeping an open mind and accepting advices and constructive critics. When people are constructive and know what they are talking about, negotiating a proper mix is generally easy. When it is not the case, it is harder... but we are just weak humans and, sometimes, battles of egos are also part of the game...

These last 25 years (yes, I am probably MUCH older than everyone else here but that's not a reason to throw stones at me :), I have continuously opposed weak committee decisions and I am quite experienced (although tired) on this kind of process. This is also the reason why I didn't have only friends on the PHP internals ML, but that's all over now...

To illustrate this, I just modified the RFC to indicate that the trend was to limit search recursion to 1 level, as I also think that, even if attractive from a theoretical point of view, nested search would be too confusing for a marginal benefit. On the other side, I still insist on arbitrarily-nested subject because I think it is worth the (all-relative) added complexity.

I am waiting for your comments on the RFC.

PS: The only persistent fault I recognize is writing too long messages :)

Regards

François