Support counting non-Iterator Traversable objects

[jmikola]

Per the commit message:

Extension classes may implement Traversable without also implementing Iterator or IteratorAggregate.

I discovered this with MongoDB\Driver\Cursor, which is only Traversable (it implements PHP's internal iteration handlers).

While this does remove the assumption of an Iterator API, it also defeats the purpose of the original patch from #1125 to undoing the side-effect of iterator_count() advancing the object. I have two observations on that:

• Some Traversables or Iterators may not support rewinding. While NoRewindIterator::rewind() is implemented as a NOOP, I can imagine some other advance-only iterators throwing exceptions when calling rewind(). This is the case with MongoDB\Driver\Cursor, as we cannot rewind the result set coming from the database.
• Traversable objects that are neither an Iterator or IteratorAggregate interface may still support rewinding. In that case, we may want to compose them in an IteratorIterator to allow the original fix from Fixed a bug in PHPUnit_Framework_Constraint_Count #1125 to apply.

I'm open to feedback on the above points and can revise the PR as needed.

[codecov-io]

Codecov Report

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

@@             Coverage Diff              @@
##             master    #2642      +/-   ##
============================================
+ Coverage     53.84%   53.85%   +0.01%     
- Complexity     2744     2745       +1     
============================================
  Files           102      102              
  Lines          7191     7193       +2     
============================================
+ Hits           3872     3874       +2     
  Misses         3319     3319

Impacted Files	Coverage Δ	Complexity Δ
src/Framework/Constraint/Count.php	86.11% <100%> (+0.81%)	16 <0> (+1)	⬆️

---

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 104a9bd...e9277ff. Read the comment docs.

[keradus]

test is needed for this change

[jmikola]

How do you propose testing this in userland if Traversable cannot be implemented directly?

Given the current implementation, I could exercise the ! $iterator instanceof Iterator condition by having an IteratorAggregate instance return a second IteratorAggregate; however, that makes an assumption that the earlier logic that resolves an IteratorAggregate will always stop after one level of resolution.

I'd also like to discuss the two outstanding questions in the OP before making further changes. It would be premature to invest time in a test case until we establish what the correct fix should be.

[keradus]

traversable = array or iterable follower, you need to ensure both are covered by tests

[jmikola]

traversable = array or iterable follower, you need to ensure both are covered by tests

I don't think we're on the same page. The iterable type may be an array or Traversable. A Traversable may be a userland class that implements Iterator or IteratorAggregate, but it may also be an internal class that implements Traversable directly and the get_iterator Zend object handler. This is touched upon in the following note from the Traversable documentation:

Internal (built-in) classes that implement this interface can be used in a foreach construct and do not need to implement IteratorAggregate or Iterator.

Such a test case would require a Traversable instance that is neither an Iterator or an IteratorAggregate instance. Short of adding a dev dependency on a third-party PHP extension that provides an internal Traversable class, I am asking how you would propose testing this. Although the approach described in my last comment ("IteratorAggregate returns another IteratorAggregate") would technically exercise the changed LOC, it makes assumptions about the current implementation of getCountOf(), which I would argue is broken per my explanation below.

---

The getCountOf() function has an early case to handle Countable instances or array types, which is outside the scope of my change. I assume that is already tested.

This PR is modifying logic within the case for a Traversable instance. 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.
2. It assumes any Iterator can be rewound.

Ignoring the need for test coverage for a second, it was not my intention that this PR be merged as-is. The existing change is only a suggestion that demonstrates the first problem with the existing implementation. My goal was to have the questions in the OP answered before submitting any changeset for review (hence the "RFC" in the title).

[sebastianbergmann]

I understand what you want to achieve, @jmikola, and I am all for it. I am not entirely sure that I understand your patch. I agree with you that this change cannot be tested without adding a development-time dependency on a third-party PHP extension that provides an internal Traversable implementation.

[jmikola]

I am not entirely sure that I understand your patch.

The current patch only attempts to avoid calling a nonexistent method, since it is possible at the modified LOC that we have an object that is Traversable but not an Iterator.

I agree with you that this change cannot be tested without adding a development-time dependency on a third-party PHP extension that provides an internal Traversable implementation.

Searching the PHP sources for zend_ce_traversable turned up the DatePeriod class, which has existed since PHP 5.3 and implements Traversable without the Iterator or IteratorAggregate interfaces (thanks @derickr). I'll see if this can be used in a test.

In the interest of keeping this PR limited in scope, I will punt on the two questions in my OP and will instead create a separate GitHub issue for future discussion. IMO, these edge cases are the result of an incomplete design in #1125.

Lastly, the issue I described with unwrapping multiple, nested IteratorAggregate objects should be easy to resolve and test. I will create a separate PR for that.

[jmikola]

Previously, this PR used a ternary when assigning $key. This guard conditional is likely clearer and avoids making an assumption about how a null value of $key will be handled later in the code (i.e. deciding whether to attempt rewinding the iterator).