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

Iterator #50

Merged
merged 5 commits into from
May 8, 2012
Merged

Iterator #50

merged 5 commits into from
May 8, 2012

Conversation

stof
Copy link
Collaborator

@stof stof commented May 3, 2012

This adds the RecursiveIterator interface on the iterator of the MenuItem, allowing to iterate on the whole tree by using a RecursiveIteratorIterator (and potentially limiting the depth as it can do it).

My plan is to include the new iterator in the 1.1 release (as it can already be useful). However, I'm not sure about the inclusion of the CurrentItemFilterIterator I wrote. My concern about it is that I will need to change the constructor for the 2.0 release to pass the Matcher too (see #49 for it), which would break code using it for people starting to use this iterator. On the other hand, they would probably already see some breakages. So what do you think about including it ?

@stof
Copy link
Collaborator Author

stof commented May 4, 2012

One thing to note: when using a RecursiveIteratorIterator, you will get all items of the tree except the root. This is due to the way the iteration works (the root item is not part of the iteration when iterating over it) and there is nothing to do about it.

@stof
Copy link
Collaborator Author

stof commented May 8, 2012

Ok, after this awesome community feedback (well, @merk agreed with me on IRC), I decided to ship the CurrentItemFilterIterator in 1.1

stof added a commit that referenced this pull request May 8, 2012
@stof stof merged commit d7eb547 into master May 8, 2012
@weaverryan
Copy link
Contributor

Sorry man, this has been sitting in my inbox but I hadn't had time to follow-up yet!

@stof
Copy link
Collaborator Author

stof commented May 8, 2012

there is other PRs waiting for feedback from interested guys if you want :)

@stof
Copy link
Collaborator Author

stof commented May 8, 2012

I found a way to iterate over all item of the tree, including the root:

<?php
$innerIterator = new \Knp\Menu\Iterator\ItemIterator(array($menu));
$fullTreeIterator = new \RecursiveIteratorIterator($innerIterator , \RecursiveIteratorIterator::SELF_FIRST);

Do you think it would make sense to have an iterator allowing to build this more easily like the following ?

<?php
// the parent item is displayed first as I find it a more sensible default than displaying only leaves
$fullTreeIterator = new \Knp\Menu\Iterator\FullTreeIterator($menu);
$fullTreeIterator = new \Knp\Menu\Iterator\FullTreeIterator($menu, \RecursiveIteratorIterator::SELF_FIRST);

// children before their parent item
$fullTreeIterator = new \Knp\Menu\Iterator\FullTreeIterator($menu, \RecursiveIteratorIterator::CHILD_FIRST);

// leaves only
$leavesIterator = new \Knp\Menu\Iterator\FullTreeIterator($menu, \RecursiveIteratorIterator::LEAVES_ONLY);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants