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

Fix collections when iterating over certain classes #11396

Merged
merged 6 commits into from Nov 15, 2017

Conversation

Projects
None yet
6 participants
@dakota
Member

dakota commented Nov 1, 2017

When a class that extends the base ArrayIterator class are iterated
over, the collection class would replace the class with a basic array or
ArrayIterator, breaking any custom logic in the original class.

This fixes the issue by checking if the class is an ArrayIterator,
rather than if it is simply a instance of ArrayIterator.

Fix collections when iterating over certain classes
When a class that extends the base ArrayIterator class are iterated
over, the collection class would replace the class with a basic array or
ArrayIterator, breaking any custom logic in the original class.

This fixes the issue by checking if the class is an ArrayIterator,
rather than if it is simply a instance of ArrayIterator.
@ADmad

This comment has been minimized.

Show comment
Hide comment
@ADmad

ADmad Nov 1, 2017

Member

phpstan seems to be unhappy with your changes :)

Member

ADmad commented Nov 1, 2017

phpstan seems to be unhappy with your changes :)

@markstory markstory added this to the 3.5.5 milestone Nov 1, 2017

@markstory

Any chance we could get a test to help reproduce this?

@markstory markstory modified the milestones: 3.5.5, 3.5.6 Nov 2, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Nov 2, 2017

Codecov Report

Merging #11396 into master will increase coverage by 0.49%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11396      +/-   ##
============================================
+ Coverage     92.63%   93.13%   +0.49%     
- Complexity    13006    13020      +14     
============================================
  Files           436      436              
  Lines         33695    33792      +97     
============================================
+ Hits          31215    31471     +256     
+ Misses         2480     2321     -159
Impacted Files Coverage Δ Complexity Δ
src/Collection/Iterator/ReplaceIterator.php 100% <100%> (ø) 6 <0> (ø) ⬇️
src/Collection/Iterator/ExtractIterator.php 100% <100%> (ø) 6 <0> (ø) ⬇️
src/Collection/CollectionTrait.php 99.08% <100%> (ø) 144 <0> (ø) ⬇️
src/Collection/Iterator/FilterIterator.php 95% <100%> (ø) 7 <0> (ø) ⬇️
src/Collection/Iterator/StoppableIterator.php 92% <100%> (ø) 8 <0> (ø) ⬇️
src/Routing/RouteCollection.php 90.86% <0%> (-2.32%) 25% <0%> (ø)
src/Http/Client/Adapter/Stream.php 98.27% <0%> (+0.18%) 37% <0%> (+3%) ⬆️
src/I18n/RelativeTimeFormatter.php 95.52% <0%> (+0.49%) 74% <0%> (ø) ⬇️
src/Cache/Engine/FileEngine.php 90.16% <0%> (+1.09%) 73% <0%> (ø) ⬇️
src/View/View.php 92.82% <0%> (+1.62%) 165% <0%> (+11%) ⬆️
... and 11 more

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 1f09411...74ace8a. Read the comment docs.

codecov-io commented Nov 2, 2017

Codecov Report

Merging #11396 into master will increase coverage by 0.49%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11396      +/-   ##
============================================
+ Coverage     92.63%   93.13%   +0.49%     
- Complexity    13006    13020      +14     
============================================
  Files           436      436              
  Lines         33695    33792      +97     
============================================
+ Hits          31215    31471     +256     
+ Misses         2480     2321     -159
Impacted Files Coverage Δ Complexity Δ
src/Collection/Iterator/ReplaceIterator.php 100% <100%> (ø) 6 <0> (ø) ⬇️
src/Collection/Iterator/ExtractIterator.php 100% <100%> (ø) 6 <0> (ø) ⬇️
src/Collection/CollectionTrait.php 99.08% <100%> (ø) 144 <0> (ø) ⬇️
src/Collection/Iterator/FilterIterator.php 95% <100%> (ø) 7 <0> (ø) ⬇️
src/Collection/Iterator/StoppableIterator.php 92% <100%> (ø) 8 <0> (ø) ⬇️
src/Routing/RouteCollection.php 90.86% <0%> (-2.32%) 25% <0%> (ø)
src/Http/Client/Adapter/Stream.php 98.27% <0%> (+0.18%) 37% <0%> (+3%) ⬆️
src/I18n/RelativeTimeFormatter.php 95.52% <0%> (+0.49%) 74% <0%> (ø) ⬇️
src/Cache/Engine/FileEngine.php 90.16% <0%> (+1.09%) 73% <0%> (ø) ⬇️
src/View/View.php 92.82% <0%> (+1.62%) 165% <0%> (+11%) ⬆️
... and 11 more

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 1f09411...74ace8a. Read the comment docs.

@dakota

This comment has been minimized.

Show comment
Hide comment
@dakota

dakota Nov 2, 2017

Member

@markstory Test added

@ADmad Very silly change added to make PHPStan happy

Member

dakota commented Nov 2, 2017

@markstory Test added

@ADmad Very silly change added to make PHPStan happy

dakota added some commits Nov 15, 2017

@dakota

This comment has been minimized.

Show comment
Hide comment
@dakota

dakota Nov 15, 2017

Member

@lorenzo Ready to go :D

Member

dakota commented Nov 15, 2017

@lorenzo Ready to go :D

@lorenzo

This comment has been minimized.

Show comment
Hide comment
@lorenzo

lorenzo Nov 15, 2017

Member

Sweet!

Member

lorenzo commented Nov 15, 2017

Sweet!

@lorenzo lorenzo merged commit e7a15f8 into master Nov 15, 2017

6 checks passed

codecov/patch 100% of diff hit (target 92.63%)
Details
codecov/project 93.13% (+0.49%) compared to 1f09411
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
stickler-ci No lint errors found.

@lorenzo lorenzo deleted the fix-collections branch Nov 15, 2017

markstory added a commit that referenced this pull request Nov 19, 2017

Fix access to inner iterator methods in some cases.
A fix was made to 3.5 that allows subclasses of ArrayIterator to have
userland methods defined on collections via IteratorIterator's __call
features. However a conflicting change was made in 3.next to block
access to the in-place mutation methods available on ArrayIterator. This
set of changes attempts to compromise between those two goals.

This also fixes the 3.next build which is currently broken.

Refs #11045
Refs #11396
});
$this->assertTrue(method_exists($newIterator, 'checkValues'), 'Our method has gone missing!');
$this->assertTrue($newIterator->checkValues());

This comment has been minimized.

@chinpei215

chinpei215 Nov 22, 2017

Member

@lorenzo Is this a correct use of the Collection class? Personally, I don't think so. Because, for example, if the method of inner iterator is getPrimeNumbers(), people would expect that it returns array(1), as 2, 3, 5 and 7 have been already filtered by filter() and reject() calls. However it would return array(1, 2, 3, 5, 7) actually, as Collection doesn't remove any elements of the original iterator. I thought that was why we blocked calling ArrayIterator::count() via the Collection class.

Refs #11440

@chinpei215

chinpei215 Nov 22, 2017

Member

@lorenzo Is this a correct use of the Collection class? Personally, I don't think so. Because, for example, if the method of inner iterator is getPrimeNumbers(), people would expect that it returns array(1), as 2, 3, 5 and 7 have been already filtered by filter() and reject() calls. However it would return array(1, 2, 3, 5, 7) actually, as Collection doesn't remove any elements of the original iterator. I thought that was why we blocked calling ArrayIterator::count() via the Collection class.

Refs #11440

This comment has been minimized.

@lorenzo

lorenzo Nov 26, 2017

Member

It is another method, though. Not the results you get when iterating, so it could be argued that this is vivo the correct and at the same time not.

@lorenzo

lorenzo Nov 26, 2017

Member

It is another method, though. Not the results you get when iterating, so it could be argued that this is vivo the correct and at the same time not.

This comment has been minimized.

@chinpei215

chinpei215 Nov 28, 2017

Member

If it's a feature I think it would need to be documented in the Collection page of our cookbook explicitly. Also, I would have to revert my PR #11045, as method_exists($collection, 'cuustomMethod') no longer works. IteretorIterator seems to handle method_exists() magically if the __call() method is not overridden, and we would not be able to emulate the behavior by any PHP code.

@chinpei215

chinpei215 Nov 28, 2017

Member

If it's a feature I think it would need to be documented in the Collection page of our cookbook explicitly. Also, I would have to revert my PR #11045, as method_exists($collection, 'cuustomMethod') no longer works. IteretorIterator seems to handle method_exists() magically if the __call() method is not overridden, and we would not be able to emulate the behavior by any PHP code.

This comment has been minimized.

@lorenzo

lorenzo Nov 29, 2017

Member

Let's not support that feature, then

@lorenzo

lorenzo Nov 29, 2017

Member

Let's not support that feature, then

This comment has been minimized.

@chinpei215

chinpei215 Nov 30, 2017

Member

OK. Then I will send a patch to fix the problematic test case.

@chinpei215

chinpei215 Nov 30, 2017

Member

OK. Then I will send a patch to fix the problematic test case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment