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

6 years ago by Nicolas Grekas — view source

unread

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

6 years ago by Kalle Sommer Nielsen — view source

unread

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

6 years ago by Pierre Joye — view source

unread

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

6 years ago by Nicolas Grekas — view source

unread

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

6 years ago by Sara Golemon — view source

unread

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

6 years ago by Nicolas Grekas — view source

unread

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

6 years ago by Christoph M. Becker — view source

unread

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

6 years ago by Remi Collet — view source

unread

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.

6 years ago by Christoph M. Becker — view source

unread

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

6 years ago by Sara Golemon — view source

unread

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

6 years ago by Christoph M. Becker — view source

unread

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

6 years ago by Sara Golemon — view source

unread

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

6 years ago by Nicolas Grekas — view source

unread

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