Duplicate of bug #61780, fixed in master. (unmatched groups will be NULL)

Not sure if it's quite a duplicate. IIRC we now use null instead of "" if unmatched, but we still don't fill up null values at the end.

Hmm, yes, I misunderstood this report: 61780 is about turning empty strings from unmatched subpatterns into NULLs while this is about *adding* trailing unmatched subpatterns.
I don't see anything in the 61780 changes that clearly indicate this is also fixed, but some parts look like they might do it accidentally. Can anyone check? (It'd be nice if 3v4l could do master too...)

Actually I can. Yay for Linux on Windows!

~/php/master/bin# ./php
<?php var_dump(preg_match('/(a)?(b)?(c)?/', 'b', $matches), $matches);
int(1)
array(3) {
  [0]=>
  string(1) "b"
  [1]=>
  NULL
  [2]=>
  string(1) "b"
}

[1] is clearly NULL but there is no [3] for the unmatched (c)? group.

For PREG_PATTERN_ORDER unmatched subpatterns are already added[1];
something like that would also have to be done for PREG_SET_ORDER.

However, that would cause BC issues, and adding another flag isn't
appealing to me. I also think that NULL and unset are close enough
to stick with the current behavior for now; checking `isset()`
would suffice.

[1] <https://github.com/php/php-src/blob/PHP-7.1.0/ext/pcre/php_pcre.c#L862-L871>

As noted by cmb, this bug does not appear with the PREG_PATTERN_ORDER flag.
Thus I switched the flag and my code to use PREG_PATTERN_ORDER and implemented my own preg_set_order() function.
But I found a nearly identical bug with the PREG_OFFSET_CAPTURE flag.

Consider the code

  preg_match_all("#(a)?(b)(c)?#","b",$matches,PREG_OFFSET_CAPTURE);
  var_dump($matches);

Returns

  array(4) {
    [0] => array(1) {
      [0] => array(2) {
        [0] => string(1) "b" [1] => int(0)
      }
    } [1] => array(1) {
      [0] => array(2) {
        [0] => string(0) "" [1] => int(-1)
      }
    } [2] => array(1) {
      [0] => array(2) {
        [0] => string(1) "b" [1] => int(0)
      }
    } [3] => array(1) {
      [0] => string(0) ""
    }
  }

As you can see, the third optional capture group returns something different than the first optional capture group.
This is too a bug. Again, I don't care what representation is chosen. But it must be the same for any optional capture no matter if trailing or not.

Regarding BC
The proposed change would be to change the representation of missed capture groups  from 2 representations to 1 representation.
The only code that would break is code that supported only the more obscure representation that only appears with trailing groups. And if that code exists, then it will still break on non trailing missing groups even without the change.

Regarding the BC break: when fixing <https://bugs.php.net/61780> I
would have preferred to unset unmatched captures, but that
appeared too much of a BC break (consider somebody is count()ing
the $matches). Therefore I changed the empty strings to NULL.

Adding unmatched trailing captures as NULL would still have the
same BC concerns – I don't think we can do that before PHP 7.2,
and even that might require the RFC process.

I'm glad we didn't go the unset route. Having fixed sized arrays is useful for looping by groups/matches independently of the Flag used.
You also can very easily know how many groups/matches the regex returned/contained.
By making the output array dynamicly sized. You only allow for an easy way to determine one of these things. While making the other more complex.

If you wish to delay this change until 7.2, that sounds fine.
But why would you need an RFC for a bugfix?

> But why would you need an RFC for a bugfix?

An RFC might be over the top, but at least a PR to get some
attention appears to be appropriate.

Since we now have PREG_UNMATCHED_AS_NULL, the solution appears to
be straight forward: fill up NULL values at the end, only if
PREG_UNMATCHED_AS_NULL is used.  Since this option is only
available as of PHP 7.2.0, the resulting BC break seems to be
acceptable.

The filling should happen with or without PREG_UNMATCHED_AS_NULL. What it should control whether the filler is NULLs or empty strings.

@requinix: That's impossible for BC reasons. I agree with @cmb that this should only be done for PREG_UNMATCHED_AS_NULL.

For those stuck on older PHP's, I'm wondering if the following is sufficient to work around this bug?

$pattern.='(.??)';
preg_match($pattern,$haystack,$matches);
array_pop($matches);

I can't yet find any instance where the fairly innocuous (.??) could possibly cause a problem, it should match in all cases, when there's chars left or not, and being non-greedy will not eat earlier matches' chars.

PR for PHP 7.4: https://github.com/php/php-src/pull/3964

> But I found a nearly identical bug with the PREG_OFFSET_CAPTURE flag.

Fixed this issue with https://github.com/php/php-src/commit/f53e7394eb61085c09928b47f84123bd90b64dfb on the PHP 7.4 branch.