Everything

php

RFC: Deprecate mb_ereg_replace eval option

php.internals

Rouven Weßling

10 years ago

Dear internals,

I’d like to propose the following RFC to you https://wiki.php.net/rfc/deprecate_mb_ereg_replace_eval_option

The mandatory discussion period will end January 19th.

Best regards
Rouven

Dan Ackroyd

10 years ago

Hi Rouven,

On 4 January 2016 at 16:46, Rouven Weßling <me@rouvenwessling.de> wrote:

> I’d like to propose the following RFC to you https://wiki.php.net/rfc/deprecate_mb_ereg_replace_eval_option
>
> The mandatory discussion period will end January 19th.

Are the wrong functions are listed in this sentence: "Emit an
E_DEPRECATED error whenever mb_ereg_replace_callback or
mb_eregi_replace_callback is called with the e option." As the RFC is
meant to be about mb_ereg_replace, mb_eregi_replace right?


The RFC doesn't say what functions people should use in place of the
deprecated functions. This needs to be laid out clearly. I can guess
what should happen for mb_ereg_replace, but there's not clear
replacement for mb_eregi_replace, as there is no
"mb_eregi_replace_callback" function. That ought to be thought about
and added to the RFC.

> The mandatory discussion period will end January 19th.

That sentence is implying that you will put it to the vote as soon as
possible. Please don't do that.  There is no rush to deprecate these
functions; the earliest they would actually be removed is years away,
and there needs to be a path guiding the people on how to replace that
functionality in place before we deprecate them.

Additionally, people should be given time to respond with feedback.

cheers
Dan

Rouven Weßling

10 years ago

Hi Dan,

> On 04 Jan 2016, at 20:52, Dan Ackroyd <danack@basereality.com> wrote:
>
> Are the wrong functions are listed in this sentence: "Emit an
> E_DEPRECATED error whenever mb_ereg_replace_callback or
> mb_eregi_replace_callback is called with the e option." As the RFC is
> meant to be about mb_ereg_replace, mb_eregi_replace right?

Indeed. Thanks for spotting it.

> The RFC doesn't say what functions people should use in place of the
> deprecated functions. This needs to be laid out clearly. I can guess
> what should happen for mb_ereg_replace, but there's not clear
> replacement for mb_eregi_replace, as there is no
> "mb_eregi_replace_callback" function. That ought to be thought about
> and added to the RFC.

That information is the section "Backward Incompatible Changes”, which admittedly may not be the most obvious place to look. To quote that section:

“The suggested replacement, mb_ereg_replace_callback is has only been available since PHP 5.4.1. Projects which support both PHP 5.3 and PHP 7.1 may need two code paths to avoid deprecation warnings.

There is no mb_eregi_replace_callback function. Code using it will have to be converted to use mb_ereg_replace_callbackwith the i option."

>> The mandatory discussion period will end January 19th.
>
> That sentence is implying that you will put it to the vote as soon as
> possible. Please don't do that.  There is no rush to deprecate these
> functions; the earliest they would actually be removed is years away,
> and there needs to be a path guiding the people on how to replace that
> functionality in place before we deprecate them.

The implication wasn’t my attention. It was mostly a reminder for myself how long I have to keep the discussion open. That said, I hope this isn’t controversial due to the similar RFC about pre_replace passing 4 years ago.

Best regards
Rouven

Dan Ackroyd

10 years ago

On 5 January 2016 at 10:39, Rouven Weßling <me@rouvenwessling.de> wrote:

> Hi Dan,
>
> There is no mb_eregi_replace_callback function. Code using it will have to be converted to use mb_ereg_replace_callbackwith the i option."
>

This is possibly a stupid question, but one that needs to be asked anyway:

Have you checked that simply converting code that uses
mb_eregi_replace to using mb_ereg_replace_callback with the i flag
behaves the same in all circumstances?

I assume it should be fine.....but assumptions are the progenitor of
most....bad situations.

cheers
Dan

Rouven Weßling

10 years ago

> On 06 Jan 2016, at 18:03, Dan Ackroyd <danack@basereality.com> wrote:
>
> This is possibly a stupid question, but one that needs to be asked anyway:

Certainly not a stupid question.

> Have you checked that simply converting code that uses
> mb_eregi_replace to using mb_ereg_replace_callback with the i flag
> behaves the same in all circumstances?

Well the _callback function obviously behaves differently with regards to the, well, callback.

I haven’t actually tested it (kinda tough for “all situations”), but if we compare mb_eregi_replace with mb_ereg_replace with the i options is quite easy to see that they behave the same by looking at the source.

mb_eregi_replace (https://github.com/php/php-src/blob/master/ext/mbstring/php_mbregex.c#L1035) calls _php_mb_regex_ereg_replace_exec with the flag ONIG_OPTION_IGNORECASE (mb_ereg_replace calls the same function without that flag). When options are set on the function call, those are passed together with that flag to
_php_mb_regex_init_options (https://github.com/php/php-src/blob/master/ext/mbstring/php_mbregex.c#L578) which sets the ONIG_OPTION_IGNORECASE when the i option is specified. After that both functions take exactly the same path.

I hope that clears it up.

Best regards
Rouven

Rouven Weßling

10 years ago

> On 04 Jan 2016, at 17:46, Rouven Weßling <me@rouvenwessling.de> wrote:
>
> I’d like to propose the following RFC to you https://wiki.php.net/rfc/deprecate_mb_ereg_replace_eval_option

There hasn’t been any discussion on this for a while. Just to to be sure it didn’t just got forgotten due to the CoC discussion, let me remind you of it ;)

If there are no concerns brought up, I’ll open the vote tomorrow.

Best regards
Rouven

Stas Malyshev

10 years ago

Hi!

> There hasn’t been any discussion on this for a while. Just to to be
> sure it didn’t just got forgotten due to the CoC discussion, let me
> remind you of it ;)

I think it looks fine, not much to discuss :) It looks like /e should be
phased out here just as we did in other places.

--
Stas Malyshev
smalyshev@gmail.com