Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixed bug #68128 #865

Closed
wants to merge 1 commit into from

Conversation

6 participants
@datibbaw
Copy link
Contributor

commented Oct 6, 2014

Fixes an issue that's been around since d81ea16.

Three issues are addressed:

  1. RecursiveRegexIterator::accept() should accept non-empty arrays without applying any regular expression and RegexIterator::accept() should not accept an array.
  2. RegexIterator::accept() should not accept an atom that fails to match anything, even when PREG_PATTERN_ORDER is used (which would return an array of empty arrays).
  3. RecursiveRegexIterator::getChildren() should pass all constructor arguments to its child iterator instead of just the regular expression.
@Hywan

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2014

👍

@Hywan

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2014

How to fix this in prior PHP versions (by extending the RecursiveRegexIterator class)?

@datibbaw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2014

@Hywan When you extend RecursiveRegexIterator, the following ::accept() implementation should do the trick:

function accept()
{
    return $this->hasChildren() || parent::accept();
}
@datibbaw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2014

@Hywan This patch can easily be back ported to earlier versions.

@Hywan

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2014

@datibbaw Exactly, it fixes the issue and tests are green again. Thanks!

@Hywan

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2014

Now, HHVM has the same bug…

@SiebelsTim

This comment has been minimized.

Copy link
Contributor

commented Oct 7, 2014

@datibbaw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 8, 2014

I've asked the list to review the code changes; if all is well, it will be merged by the end of this week.

@Hywan

This comment has been minimized.

Copy link
Contributor

commented Oct 8, 2014

Is the documentation need to be updated?

@datibbaw datibbaw force-pushed the datibbaw:bug68128 branch from a32845a to d42d645 Oct 8, 2014

@datibbaw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 9, 2014

@Hywan Could you log a separate Doc bug for that? The PCRE constants are declared on RegexIterator, so ideally it should be documented in that way.

@Hywan

This comment has been minimized.

Copy link
Contributor

commented Oct 9, 2014

@datibbaw On Github or on edit.php.net?

@JoelMarcey

This comment has been minimized.

Copy link

commented Oct 10, 2014

What are the chances that this pull request will be accepted? And, if it is, in what PHP versions would the fix be incorporated? Thanks.

@datibbaw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 11, 2014

@Tyrael ping! do you reckon this should be back ported to 5.6?

Fixed bug #68128
Three issues are addressed:

- RecursiveRegexIterator::accept() should accept non-empty arrays without
  applying any regular expression and RegexIterator::accept() should not accept
  an array.
- RegexIterator::accept() should not accept an atom that fails to match
  anything, even when PREG_PATTERN_ORDER is used (which would return an array
  of empty arrays).
- RecursiveRegexIterator::getChildren() should pass all constructor arguments
  to its child iterator instead of just the regular expression.

@datibbaw datibbaw force-pushed the datibbaw:bug68128 branch from d42d645 to a5d3748 Oct 14, 2014

@Tyrael

This comment has been minimized.

Copy link
Member

commented Oct 14, 2014

@datibbaw I would be fine having it in 5.6

@datibbaw

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2014

PHP-5.5: 71ba533
PHP-5.6: fb35d7c
master: 1f4f2ef

@datibbaw datibbaw closed this Oct 14, 2014

marcioAlmada added a commit to marcioAlmada/php-src that referenced this pull request Sep 19, 2016

fix bug caused by php#865
In case USE_KEY flag is active, RegexIterator->accept() should keep it's
old behavior which is to accept keys mapping arrays.

This broke after PHP 5.5 but was not noticed due to lack of tests for USE_KEY.
@marcioAlmada

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2016

@datibbaw I'm sorry to necromance your pull request 😅 but there is a sequel here #2133 you may want to review and voice an opinion.

marcioAlmada added a commit to marcioAlmada/php-src that referenced this pull request Sep 19, 2016

fix bug related to php#865
In case USE_KEY flag is active, RegexIterator->accept() should keep it's
old behavior which is to accept keys mapping arrays.

This broke after PHP 5.5 but was not noticed due to lack of tests for USE_KEY.

php-pulls pushed a commit that referenced this pull request Sep 22, 2016

fix bug related to #865
In case USE_KEY flag is active, RegexIterator->accept() should keep it's
old behavior which is to accept keys mapping arrays.

This broke after PHP 5.5 but was not noticed due to lack of tests for USE_KEY.

dstogov added a commit to zendtech/php-src that referenced this pull request Sep 23, 2016

Merge branch 'master' into jit-dynasm
* master:
  Removed impossible condition
  fix bug related to php#865
  Fix bug #69579
  Fix bug #69579
  update NEWS
  update NEWS
  Fixed bug #73126 Cannot pass parameter 1 by reference
  Limit size of result set for test query
  update NEWS
  PHP bug 67130: nextRowset should work with unfetched rows
  Move dtor before memory freed to avoid invalid read
  Fixed skip
  Skip failing FreeType tests for now
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.