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

Unpack nested IteratorAggregate objects for Count #2689

Merged

Conversation

jmikola
Copy link
Contributor

@jmikola jmikola commented May 22, 2017

From my previous comment in #2642 (comment):

Within the Traversable case, the function checks for an IteratorAggregate and attempts to unwrap it once by calling IteratorAggregate::getIterator(). The function then checks for a Generator instance and handles that separately. After that point, the existing implementation makes two assumptions, which I believe are premature:

  1. It assumes that the object is an Iterator instance when it invokes the key() method. This fails to acknowledge that IteratorAggregate::getIterator() can return any Traversable instance.

This PR adds logic to unwrap nested IteratorAggregate objects until we encounter a Traversable that is not an IteratorAggregate instance. This ensures that generator detection (#2149) and internal Traversables (#2642) are properly detected before we attempt to restore the Iterator's position after counting (#1125).

In a separate commit, the PR also adds missing regression tests for Count's handling of non-nested IteratorAggregate objects.

jmikola added 2 commits May 22, 2017 18:08
This allows generators (sebastianbergmann#2149) and internal Traversables (sebastianbergmann#2642) to be properly detected before attempting to restore the Iterator's position after counting (sebastianbergmann#1125).

/* This class is used for testing a chain of IteratorAggregate objects, since
* PHP does allow IteratorAggregate::getIterator() to return an instance of the
* same class. */
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See: zend_user_it_get_new_iterator(). If IteratorAggregate::getIterator() returns an instance of the same class, PHP throws an exception. This is a basic attempt to prevent infinite recursion, although it is certainly still possible if two different IteratorAggregate implementations return instances of each other.

@codecov-io
Copy link

codecov-io commented May 22, 2017

Codecov Report

Merging #2689 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #2689      +/-   ##
============================================
+ Coverage     53.85%   53.87%   +0.01%     
  Complexity     2745     2745              
============================================
  Files           102      102              
  Lines          7193     7193              
============================================
+ Hits           3874     3875       +1     
+ Misses         3319     3318       -1
Impacted Files Coverage Δ Complexity Δ
src/Framework/Constraint/Count.php 88.88% <100%> (+2.77%) 16 <0> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d3180f1...08451fe. Read the comment docs.

@sebastianbergmann sebastianbergmann self-assigned this Jun 1, 2017
@sebastianbergmann sebastianbergmann added the type/enhancement A new idea that should be implemented label Jun 1, 2017
@sebastianbergmann sebastianbergmann added this to the PHPUnit 6.2 milestone Jun 1, 2017
@sebastianbergmann sebastianbergmann merged commit a9439d3 into sebastianbergmann:master Jun 1, 2017
@sebastianbergmann
Copy link
Owner

Thanks!

@jmikola jmikola deleted the count-iteratoraggregate branch June 1, 2017 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type/enhancement A new idea that should be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants