SPL: Fix for bug 66405 RecursiveDirectoryIterator w/ CURRENT_AS_PATHNAME #665
Conversation
…HNAME Commit bc75208 introduced a change to make RecursiveDirectoryIterator::getChildren() return the current directory name when the FilesystemIterator::CURRENT_AS_PATHNAME flag is used. However, per the RecursiveIterator interface getChildren() is supposed to return an Iterator (or subclass of). So when you use RecursiveDirectoryIterator with FilesystemIterator::CURRENT_AS_PATHNAME and wrap it in a RecursiveIteratorIterator and try to foreach: RecursiveIteratorIterator throws an UnexpectedValueException with message 'Objects returned by RecursiveIterator::getChildren() must implement RecursiveIterator' becuase it got a string when it should have gotten an Iterator object. This commit removed the change introduced by commit bc75208 so that RecursiveDirectoryIterator::getChildren() always returns a new instance of itself. This fix is against the PHP-5.4 branch (as per https://wiki.php.net/vcs/gitworkflow applied to lowest stable branch). I believe it should get ported up to 5.5, 5.6, and master. A test case (bug66405.phpt) is includes. All tests in ext/spl/tests pass.
|
I'm not sure why it is necessary. If you tell FilesystemIterator to return pathname, it would return pathname, what's wrong with that? |
|
Yes FilesystemIterator and RecursiveDirectoryIterator should return the pathname if you ask for in in |
|
I do apologize for not doing push against the correct branch. Instructions on wiki.PHP.net say to do PR against lowest branch. Maybe somebody should update that. |
|
@paulyg the instructions on the wiki are for developers (people doing actual commit/merge), not pull authors. I'll review this patch soon but I wonder since we've had it there since 5.3.0 and looks like the only point of bc75208 was to do this, maybe it makes sense to reach out to @colder (colder at php.net) and ask him what was the idea there. |
|
@smalyshev OK. If you need me to rebase the PR on a different branch let me know. |
|
I have to say I don't agree with my commit back then. It makes very little sense, getChildren should always return an iterator, but this iterator should be customized to return the pathname when calling current(). I will need a bit more time to take a careful look at this fix here. |
|
@colder ok, thanks, please update when you have reviewed it. |
|
The patch looks good to me. The only small worry I have is the, in essence, new $this($path, $flags) which means that each subclass of RecursiveDirectoryIterator needs to provide such a constructor and there is no easy way of extending this. Since this is not supposed to be a re-design of RecursiveIterator I think this is a reasonable constraint to keep, and it is much better than returning an instance of the base class. |
Commit bc75208 introduced a change to make
RecursiveDirectoryIterator::getChildren() return the current directory name
when the FilesystemIterator::CURRENT_AS_PATHNAME flag is used. However, per the
RecursiveIterator interface getChildren() is supposed to return an Iterator
(or subclass of). So when you use RecursiveDirectoryIterator with
FilesystemIterator::CURRENT_AS_PATHNAME and wrap it in a
RecursiveIteratorIterator and try to foreach: RecursiveIteratorIterator throws
an UnexpectedValueException with message 'Objects returned by
RecursiveIterator::getChildren() must implement RecursiveIterator' becuase it
got a string when it should have gotten an Iterator object.
This commit removed the change introduced by commit bc75208 so that
RecursiveDirectoryIterator::getChildren() always returns a new instance of
itself.
This fix is against the PHP-5.4 branch
(as per https://wiki.php.net/vcs/gitworkflow applied to lowest stable branch).
I believe it should get ported up to 5.5, 5.6, and master. A test case
(bug66405.phpt) is includes. All tests in ext/spl/tests pass.