Incompatible with RecursiveIterator::hasChildren() Zend\Navigation\AbstractContainer #4517

Closed
noopable opened this Issue May 21, 2013 · 6 comments

Comments

Projects
None yet
5 participants
Contributor

noopable commented May 21, 2013

Zend\Navigation\AbstractContainer implements RecursiveIterator.
But Zend\Navigation\AbstractContainer::hasChildren is incompatible with RecursiveIterator::hasChildren().

RecursiveIterator::hasChildren() is that
Returns if an iterator can be created for the current entry.
// (at)see http://php.net/manual/en/recursiveiterator.haschildren.php

Not that returns if the iterator has entries.

For example, RecursiveArrayIterator::hasChildren returns whether current entry is an array or an object.

It should be

    public function hasChildren()
    {
        return $this->current()->hasPages();
    }

Otherwise, it causes some problems with using RecursiveIteratorIterator::beginChildren and so on.

Member

froschdesign commented Oct 31, 2014

@noopable
Can you provide an unit test? Thanks.

noopable added a commit to noopable/zf2 that referenced this issue Oct 31, 2014

noopable added a commit to noopable/zf2 that referenced this issue Oct 31, 2014

noopable added a commit to noopable/zf2 that referenced this issue Nov 1, 2014

noopable added a commit to noopable/zf2 that referenced this issue Nov 1, 2014

Contributor

noopable commented Nov 1, 2014

I made it.
Thank you.

noopable added a commit to noopable/zf2 that referenced this issue Nov 1, 2014

@Ocramius Ocramius added the bug label Nov 22, 2014

@Ocramius Ocramius self-assigned this Nov 22, 2014

@Ocramius Ocramius added this to the 2.3.4 milestone Nov 22, 2014

Ocramius added a commit that referenced this issue Nov 22, 2014

@Ocramius Ocramius closed this in 8e7b14d Nov 22, 2014

Ocramius added a commit that referenced this issue Nov 22, 2014

Ocramius added a commit that referenced this issue Nov 22, 2014

Merge branch 'hotfix/#6825-zend-navigation-recursive-iterator-compati…
…bility' into develop


Close #6825
Close #4517
Forward port #6825
Forward port #4517
Member

Ocramius commented Nov 22, 2014

Handled in #6825

Moring commented Jan 16, 2015

@Ocramius suggested that I ask here: to me, feel free to have other opinions, hasChildren() seems to have different than expected behavior of an RecursiveIterator implementation. Perhaps we call this out in the documentation somewhere?

Contributor

noopable commented Jan 18, 2015

but does this seem to be different than expected behavior of an RecursiveIterator implementation

Indeed, the name ,RecursiveIterator::hasChildren(), sounds like if RecursiveIterator has children or not.
But the fact is that, it is , so to speak, RecursiveIterator::currentHasChildren(). And so, RecursiveIterator::getChildrenFromCurrent(). -- These name are so strange.

In the PHP Manual,
http://php.net/manual/en/recursiveiterator.haschildren.php
It returns if an iterator can be created for the current entry.

And this relatives to getChildren
http://php.net/manual/en/recursiveiterator.getchildren.php
It returns an iterator for the current iterator entry.

The ordinary recursive flow is below.

  1. check the current to have children or not. (hasChildren())
  2. If true, get the children of the current. (getChildren())

You can see and compare this pattern in the RecursiveArrayIterator or the RecursiveDirectoryIterator and so on.
Or other classes that implements \RecursiveIterator.
https://github.com/zendframework/zf2/blob/master/library/Zend/Mail/Storage/Folder.php#L61
https://github.com/zendframework/zf2/blob/master/library/Zend/Permissions/Rbac/AbstractIterator.php#L87

Thanks.

gianarb pushed a commit to zendframework/zend-navigation that referenced this issue May 15, 2015

gianarb pushed a commit to zendframework/zend-navigation that referenced this issue May 15, 2015

gianarb pushed a commit to zendframework/zend-navigation that referenced this issue May 15, 2015

gianarb pushed a commit to zendframework/zend-navigation that referenced this issue May 15, 2015

gianarb pushed a commit to zendframework/zend-navigation that referenced this issue May 15, 2015

However, this should have been marked as Breaking Change in the Release notes! Checking for subpages with hasChildren() will now fail in navigation partials. One has to use hasPages() instead.

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