-
-
Notifications
You must be signed in to change notification settings - Fork 311
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 RegisterEventListenersAndSubscribersPass & event_subscribers service accessibility #196
Conversation
$container->getDefinition('jms_serializer.event_dispatcher') | ||
->addMethodCall('setListeners', array(call_user_func_array('array_merge', $listeners))); | ||
foreach ($listeners as $listener) { | ||
$evenDispatcherDefinition->addMethodCall('addListener', $listener); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason you are not using the setListeners
call anymore? I added it to avoid a single method call per listener.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setListeners do not transform the value it receives.
in RegisterEventListenersAndSubscribersPass
, $listeners
look like this (ordered by priority):
array(
array($eventData['event'], array($id, $method), $class, $format),
)
Whereas EventDispatcher\LazyEventDispatcher::$listeners
look like:
array(
'event' => array(
array(array('id', 'method'), 'type', 'format'),
array($callable, 'type', 'format'),
array(array('id', 'method'), 'type', 'format'),
),
)
I could also prepare the $listeners array to be set with setListeners
, but i felt like it would duplicate some "logic" from the EventDispatcher class ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add this conversion to the compiler pass, and then set the converted data using the setListeners
method?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
And for the public visibility, the compiler pass should check it IMO |
@stof Good point, added a commit about that |
Fix RegisterEventListenersAndSubscribersPass & event_subscribers service accessibility
No description provided.