8000 [EventDispatcher] Adding IteratorAggregate to GenericEvent by mtdowling · Pull Request #5268 · symfony/symfony · GitHub
[go: up one dir, main page]

Skip to content

[EventDispatcher] Adding IteratorAggregate to GenericEvent #5268

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

Merged
merged 1 commit into from
Aug 18, 2012
Merged

[EventDispatcher] Adding IteratorAggregate to GenericEvent #5268

merged 1 commit into from
Aug 18, 2012

Conversation

mtdowling
Copy link
Contributor

No description provided.

@travisbot
Copy link

This pull request passes (merged 0ad00f8 into 50df1a7).

@ghost
Copy link
ghost commented Aug 16, 2012

What is the use case for this that it should be part of the Generic event?

@mtdowling
Copy link
Contributor Author

This allows for the GenericEvent to be even more generic. Now listeners don't need to know an exact key from the arguments, but rather can iterate over the arguments to find what they are looking for. This makes the GenericEvent more like an array.

@mtdowling
Copy link
Contributor Author

How would this be a nasty break? It's just giving the GenericEvent more capabilities with IteratorAggregate.

This is a completely separate PR from the one that flipped the constructor args.

@schmittjoh
Copy link
Contributor

Why are you not just doing foreach ($event->getArguments() as $arg) { /** ... */ }?

If you just have foreach ($event), to me at least it would not be so clear what we are actually iterating over.

@mtdowling
Copy link
Contributor Author

This class already has ArrayAccess. If you're already using this class like an array, then I think you should expect to be able to iterate it like an array. I'm just finishing that concept off by implementing IteratorAggregate.

@schmittjoh
Copy link
Contributor

Indeed, if we already have ArrayAccess which we probably don't want to remove again, then that seems reasonable.

fabpot added a commit that referenced this pull request Aug 18, 2012
Commits
-------

0ad00f8 [EventDispatcher] Adding IteratorAggregate to GenericEvent

Discussion
----------

[EventDispatcher] Adding IteratorAggregate to GenericEvent

---------------------------------------------------------------------------

by drak at 2012-08-16T07:43:29Z

What is the use case for this that it should be part of the Generic event?

---------------------------------------------------------------------------

by mtdowling at 2012-08-16T17:12:28Z

This allows for the GenericEvent to be even more generic. Now listeners don't need to know an exact key from the arguments, but rather can iterate over the arguments to find what they are looking for. This makes the GenericEvent more like an array.

---------------------------------------------------------------------------

by mtdowling at 2012-08-17T19:31:04Z

How would this be a nasty break? It's just giving the GenericEvent more capabilities with IteratorAggregate.

This is a completely separate PR from the one that flipped the constructor args.

---------------------------------------------------------------------------

by schmittjoh at 2012-08-17T19:34:47Z

Why are you not just doing ``foreach ($event->getArguments() as $arg) { /** ... */ }``?

If you just have ``foreach ($event)``, to me at least it would not be so clear what we are actually iterating over.

---------------------------------------------------------------------------

by mtdowling at 2012-08-17T19:39:23Z

This class already has ArrayAccess. If you're already using this class like an array, then I think you should expect to be able to iterate it like an array. I'm just finishing that concept off by implementing IteratorAggregate.

---------------------------------------------------------------------------

by schmittjoh at 2012-08-17T19:47:43Z

Indeed, if we already have ArrayAccess which we probably don't want to remove again, then that seems reasonable.
@fabpot fabpot merged commit 0ad00f8 into symfony:master Aug 18, 2012
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.

4 participants
0