Everything

php

BC break in master (7.2) - Distinguish between unmatched subpatterns and empty matches in preg_*()

php.internals

Nicolas Grekas

9 years ago

Hi,

in master, feature request https://bugs.php.net/61780 has been implemented
and merged into master thanks to https://github.com/php/php-src/pull/1303

But as noted in the PR and even in the UPGRADING file, this is a BC break:
. preg_match() and other PCRE functions now distinguish between unmatched
subpatterns and empty matches by reporting NULL and "" (empty string),
respectively. Formerly, either was reported as empty string.

While trying to run Symfony's test suite against 7.2, we noticed that this
BC break is hitting several components badly. If Symfony is hit, there will
be many more userland code impacted for sure.

As written explicitly in the releasing policy of php-src, BC breaks must
not happen in minor versions. Therefore, I'm kindly but strongly asking for
this BC break to be reconsidered and removed.

Thanks for your consideration,
Nicolas

Kalle Sommer Nielsen

9 years ago

Hello Nicolas

2017-05-15 8:57 GMT+02:00 Nicolas Grekas <nicolas.grekas@gmail.com>:

> Hi,
>
> in master, feature request https://bugs.php.net/61780 has been implemented
> and merged into master thanks to https://github.com/php/php-src/pull/1303
>
> But as noted in the PR and even in the UPGRADING file, this is a BC break:
>   . preg_match() and other PCRE functions now distinguish between unmatched
>     subpatterns and empty matches by reporting NULL and "" (empty string),
>     respectively. Formerly, either was reported as empty string.
>
> While trying to run Symfony's test suite against 7.2, we noticed that this
> BC break is hitting several components badly. If Symfony is hit, there will
> be many more userland code impacted for sure.
>
> As written explicitly in the releasing policy of php-src, BC breaks must
> not happen in minor versions. Therefore, I'm kindly but strongly asking for
> this BC break to be reconsidered and removed.

Generally we do trend to break BC in release versions. Take 7.1 for example:

call_user_func('extract', ['name' => 'Rasmus']);

Is no longer legal, tho I do wonder who would be insane enough to do
something like that, but some changes are for the better of all of us,
I would personally call these quality of life changes. Or the notice
for applying an arithmetic operation on an poor formatted integer,
also in 7.1:

var_dump(2 + '5 apples');

Now for this change in particular, it was in the PR tracker for almost
8 months before it was merged and there was one comment that was a
positive one to it. Now don't get me wrong here, it is not everyone
that actively develops PHP and follows, tests and play with everything
submitted to us, but it was first over 2 months after it was merged,
that users started to react to this change instead of simply updating
tests, like this guy[1] did before any "BC Break" comments was let
out. It could even be simpler, it seems more like it to me, that we
spend more time discussing the politics than actually just fixing the
code so, as it can easily be done to be backwards compatible in
multiple ways.

When that is said, I can totally understand the frustration it causes
that something breaks, but I think we should see the over all picture
for each individual change, and this one is a good quality of life
change to me (and other core developers too it seems). Had it been
something major, I could understand that you'd wish it be reverted,
but this is so minor that we shouldn't waste more keystrokes on it.

[1] https://github.com/squizlabs/PHP_CodeSniffer/commit/d397f87d58119cd04cba1bc4dda1216ebb9ba642

--
regards,

Kalle Sommer Nielsen
kalle@php.net

Pierre Joye

9 years ago

hi Nicolas,

On Mon, May 15, 2017 at 1:57 PM, Nicolas Grekas
<nicolas.grekas@gmail.com> wrote:

> Hi,
>
> in master, feature request https://bugs.php.net/61780 has been implemented
> and merged into master thanks to https://github.com/php/php-src/pull/1303
>
> But as noted in the PR and even in the UPGRADING file, this is a BC break:
>   . preg_match() and other PCRE functions now distinguish between unmatched
>     subpatterns and empty matches by reporting NULL and "" (empty string),
>     respectively. Formerly, either was reported as empty string.
>
> While trying to run Symfony's test suite against 7.2, we noticed that this
> BC break is hitting several components badly. If Symfony is hit, there will
> be many more userland code impacted for sure.
>
> As written explicitly in the releasing policy of php-src, BC breaks must
> not happen in minor versions. Therefore, I'm kindly but strongly asking for
> this BC break to be reconsidered and removed.

I agree that a minimum 5 years old possible bug, quite small, causing
BC breaks is not good. As far as the fix is critical or justified, I
think it is sometimes ok to break BC for edge cases. However I do not
see this is not the case here.

@RMs revert for 7.2 and re evaluate for later?

Cheers,

--
Pierre

@pierrejoye | http://www.libgd.org

Nicolas Grekas

9 years ago

Thanks Pierre,

2017-05-15 12:12 GMT+02:00 Pierre Joye <pierre.php@gmail.com>:

> hi Nicolas,
>
> On Mon, May 15, 2017 at 1:57 PM, Nicolas Grekas
> <nicolas.grekas@gmail.com> wrote:
> > Hi,
> >
> > in master, feature request https://bugs.php.net/61780 has been
> implemented
> > and merged into master thanks to https://github.com/php/php-
> src/pull/1303
> >
> > But as noted in the PR and even in the UPGRADING file, this is a BC
> break:
> >   . preg_match() and other PCRE functions now distinguish between
> unmatched
> >     subpatterns and empty matches by reporting NULL and "" (empty
> string),
> >     respectively. Formerly, either was reported as empty string.
> >
> > While trying to run Symfony's test suite against 7.2, we noticed that
> this
> > BC break is hitting several components badly. If Symfony is hit, there
> will
> > be many more userland code impacted for sure.
> >
> > As written explicitly in the releasing policy of php-src, BC breaks must
> > not happen in minor versions. Therefore, I'm kindly but strongly asking
> for
> > this BC break to be reconsidered and removed.
>
> I agree that a minimum 5 years old possible bug, quite small, causing
> BC breaks is not good. As far as the fix is critical or justified, I
> think it is sometimes ok to break BC for edge cases. However I do not
> see this is not the case here.
>
> @RMs revert for 7.2 and re evaluate for later?

Another possibility would be to make this opt-in, with a new flag.
FYI, I quickly spotted at least two places in the code base where we check
the matches with something like:

if ('' !== $matches[2]) {...} else {...}

This should be a pretty common check so the BC break will impact more
userland code when released.

Cheers,
Nicolas

Sara Golemon

9 years ago

On Mon, May 15, 2017 at 6:36 AM, Nicolas Grekas
<nicolas.grekas@gmail.com> wrote:

>> I agree that a minimum 5 years old possible bug, quite small, causing
>> BC breaks is not good. As far as the fix is critical or justified, I
>> think it is sometimes ok to break BC for edge cases. However I do not
>> see this is not the case here.
>>
>
> Another possibility would be to make this opt-in, with a new flag.
> FYI, I quickly spotted at least two places in the code base where we check
> the matches with something like:
>
> if ('' !== $matches[2]) {...} else {...}
>
> This should be a pretty common check so the BC break will impact more
> userland code when released.
>

I agree with the original bug report that being able to distinguish
between not-matched and empty-match is a useful and meaningful
distinction, and since most users won't care, then the decision to
just do it was *probably* right at the time.  Given the observation
about strict equality checks (which I don't have the data for, but it
rings true) it does seem optimistic in *handsight*.

Given that, I'd recommend a middle-ground such as the opt-in behavior
described here.  The extra effort to add that option flag in the rare
handful of cases where it matters is miniscule compared to the cost of
adjusting equality checks in the myriad places where it doesn't
matter, but shows up.

My 2 ¢
-Sara

Nicolas Grekas

9 years ago

2017-05-15 17:21 GMT+02:00 Sara Golemon <pollita@php.net>:

> On Mon, May 15, 2017 at 6:36 AM, Nicolas Grekas
> <nicolas.grekas@gmail.com> wrote:
> >> I agree that a minimum 5 years old possible bug, quite small, causing
> >> BC breaks is not good. As far as the fix is critical or justified, I
> >> think it is sometimes ok to break BC for edge cases. However I do not
> >> see this is not the case here.
> >>
> >
> > Another possibility would be to make this opt-in, with a new flag.
> > FYI, I quickly spotted at least two places in the code base where we
> check
> > the matches with something like:
> >
> > if ('' !== $matches[2]) {...} else {...}
> >
> > This should be a pretty common check so the BC break will impact more
> > userland code when released.
> >
> I agree with the original bug report that being able to distinguish
> between not-matched and empty-match is a useful and meaningful
> distinction, and since most users won't care, then the decision to
> just do it was *probably* right at the time.  Given the observation
> about strict equality checks (which I don't have the data for, but it
> rings true) it does seem optimistic in *handsight*.
>
> Given that, I'd recommend a middle-ground such as the opt-in behavior
> described here.

Here is a PR implementing the new flag:
https://github.com/php/php-src/pull/2526

Nicolas

Christoph Becker

9 years ago

On 15.05.2017 at 12:12, Pierre Joye wrote:

> On Mon, May 15, 2017 at 1:57 PM, Nicolas Grekas
> <nicolas.grekas@gmail.com> wrote:
>
>> in master, feature request https://bugs.php.net/61780 has been implemented
>> and merged into master thanks to https://github.com/php/php-src/pull/1303
>>
>> But as noted in the PR and even in the UPGRADING file, this is a BC break:
>>   . preg_match() and other PCRE functions now distinguish between unmatched
>>     subpatterns and empty matches by reporting NULL and "" (empty string),
>>     respectively. Formerly, either was reported as empty string.
>>
>> While trying to run Symfony's test suite against 7.2, we noticed that this
>> BC break is hitting several components badly. If Symfony is hit, there will
>> be many more userland code impacted for sure.
>>
>> As written explicitly in the releasing policy of php-src, BC breaks must
>> not happen in minor versions. Therefore, I'm kindly but strongly asking for
>> this BC break to be reconsidered and removed.
>
> I agree that a minimum 5 years old possible bug, quite small, causing
> BC breaks is not good. As far as the fix is critical or justified, I
> think it is sometimes ok to break BC for edge cases. However I do not
> see this is not the case here.
>
> @RMs revert for 7.2 and re evaluate for later?

I'm not against reverting the commit, but if we want to postpone this
change to PHP 8, I suggest not to set unmatched subpatterns at all,
then.  The current solution has been an attempt to keep the BC break
small, but that apparently has failed.

--
Christoph M. Becker

Remi Collet

9 years ago

Le 15/05/2017 à 08:57, Nicolas Grekas a écrit :

> Hi,
>
> in master, feature request https://bugs.php.net/61780 has been implemented
> and merged into master thanks to https://github.com/php/php-src/pull/1303

I think everyone agree that BC should be avoid, when possible.

Even if this one is quite small, and have been tried to be as small as
possible, it exists.

Dev time are designed to detect such issues as soon as possible,
so first, thanks for reporting.

IMHO, I think a new flag to enable the new feature seems the nicest way
to keep BC and offer the new feature/behavior.

> I'm not against reverting the commit,

Reverting the commit, will be a pity

> I suggest not to set unmatched subpatterns at all,

Can you give an example of expected output ?
(I think stable position for matched pattern is important)

php -r 'var_dump(preg_match("/(a)?([a-z]*)(\d*)/", "123", $matches),
$matches);'

Remi.

Christoph Becker

9 years ago

On 15.05.2017 at 17:49, Remi Collet:

>> I suggest not to set unmatched subpatterns at all,
>
> Can you give an example of expected output ?
> (I think stable position for matched pattern is important)
>
> php -r 'var_dump(preg_match("/(a)?([a-z]*)(\d*)/", "123", $matches),
> $matches);'

See the pcretest results danielklein posted in his last comment on
<https://bugs.php.net/61780>.  Basically, instead of assigning NULL as
values, the array indexes could be skipped, so that array_key_exists()
can be used to determine whether a certain subpattern matched or not.

--
Christoph M. Becker

Sara Golemon

9 years ago

On Tue, May 16, 2017 at 5:03 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:

>>> I suggest not to set unmatched subpatterns at all,
>>
>> Can you give an example of expected output ?
>> (I think stable position for matched pattern is important)
>>
>> php -r 'var_dump(preg_match("/(a)?([a-z]*)(\d*)/", "123", $matches),
>> $matches);'
>
> See the pcretest results danielklein posted in his last comment on
> <https://bugs.php.net/61780>.  Basically, instead of assigning NULL as
> values, the array indexes could be skipped, so that array_key_exists()
> can be used to determine whether a certain subpattern matched or not.
>

Well, but isset() would do this already with NULLs.

$x = ['foo'=>NULL];
// isset($x['foo'] === false)

I know people are generally getting trained to use array_key_exists()
to avoid this "set but null" issue, but this is a case where that
behavior of isset() is actually quite useful.

-Sara

Christoph Becker

9 years ago

On 17.05.2017 at 00:13, Sara Golemon wrote:

> On Tue, May 16, 2017 at 5:03 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:
>
>>>> I suggest not to set unmatched subpatterns at all,
>>>
>>> Can you give an example of expected output ?
>>> (I think stable position for matched pattern is important)
>>>
>>> php -r 'var_dump(preg_match("/(a)?([a-z]*)(\d*)/", "123", $matches),
>>> $matches);'
>>
>> See the pcretest results danielklein posted in his last comment on
>> <https://bugs.php.net/61780>.  Basically, instead of assigning NULL as
>> values, the array indexes could be skipped, so that array_key_exists()
>> can be used to determine whether a certain subpattern matched or not.
>
> Well, but isset() would do this already with NULLs.
>
> $x = ['foo'=>NULL];
> // isset($x['foo'] === false)
>
> I know people are generally getting trained to use array_key_exists()
> to avoid this "set but null" issue, but this is a case where that
> behavior of isset() is actually quite useful.

Personally, I rarely use array_key_exists(), but others may prefer it,
and it appears to be cleaner not to set unmatched subpatterns at all.
And if we're going to add a flag anyway, it appears to be reasonable to
change the behavior right away, i.e. to introduce something like
PREG_SKIP_UNMATCHED instead of PREG_UNMATCHED_AS_NULL.

--
Christoph M. Becker

Sara Golemon

9 years ago

On Tue, May 16, 2017 at 5:21 PM, Christoph M. Becker <cmbecker69@gmx.de> wrote:

> Personally, I rarely use array_key_exists(), but others may prefer it,
> and it appears to be cleaner not to set unmatched subpatterns at all.
> And if we're going to add a flag anyway, it appears to be reasonable to
> change the behavior right away, i.e. to introduce something like
> PREG_SKIP_UNMATCHED instead of PREG_UNMATCHED_AS_NULL.
>

ftr; I'm fine either way.  My only beef is that default behavior
doesn't unnecessarily break BC. :D

-Sara

Nicolas Grekas

9 years ago

>
> PREG_SKIP_UNMATCHED instead of PREG_UNMATCHED_AS_NULL.
>

I'm just wondering if having holes in the resulting array is a good idea.
It may complicate reading dumps and forces using "foreach" vs "for". I'm
also wondering if the original proposal wouldn't be better (having NULL for
*all* nonmatches, even trailing ones), if possible.

Anyway, I'm fine with either. It's just out of my skills right now so feel
free to take over my PR ... sorry.

Nicolas