-
Notifications
You must be signed in to change notification settings - Fork 8
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
Compatibility with Silex 2 #17
Conversation
* @param EventDispatcherInterface $dispatcher | ||
* @param string $subscriberClass | ||
*/ | ||
private function removeSubscriber(EventDispatcherInterface $dispatcher, $subscriberClass) |
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.
I'm not really happy with this, but I couldn't come up with a better way for now. Maybe somebody has suggestions for a better way to do this?
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.
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.
Because it calls removeListener()
with the given subscriber object internally, which only seems able to remove the original subscribed object that has been registered. And we don't have access to these objects to remove them this way.
That's great! I've given you write access to the repo so feel free to merge master back into the working branch, that way the PR will be cleaner |
Oh wow, thx 😊 |
private function removeSubscriber(EventDispatcherInterface $dispatcher, $subscriberClass) | ||
{ | ||
if (!is_subclass_of($subscriberClass, EventSubscriberInterface::class, true)) { | ||
throw new \InvalidArgumentException('$subscriberClass must implement ' . EventSubscriberInterface::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.
why not use a typehint on $subscriberClass
for EventSubscriberInterface
instead?
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.
Because $subscriberClass
is the string class name and not an instance. Although I'm not sure it would be "bad" to use an instance here instead. But it's not really needed because the only method we call on the class is a static one.
Maybe we don't even need this check at all because it's a private method...
Thanks alot @jdreesen for taking the work to adapt this all to Silex 2! Regarding your implementation of |
I merged |
I'll write unit tests for the new |
That's awesome! I haven't had too much time too I'll review asap FYI I am planning to release PHP-DI 5.3. It doesn't contain a new major feature so I was planning on releasing the Symfony bridge 2.0 at that time. I think I'll also wait for this new version of this bridge to advertise all those changes. |
Hey guys, nothing to contribute at the moment, just wanted to let you know, awesome stuff. We're actively using this. |
…d as a shared service
Actually, I'm not sure how to properly unit test this stuff. Besides that there are already functional tests for this behaviour. |
} elseif (null !== $ret) { | ||
throw new \RuntimeException('An after middleware returned an invalid response value. Must return null or an instance of Response.'); | ||
} | ||
|
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.
I'm curious why this was removed?
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.
It must have been overlooked when it was added. The TERMINATE
kernel event is triggered after the response has been sent and thus doesn't allow to set a response. This change aligns the code with Silex.
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.
But this is the after()
method (RESPONSE
event), you pointed to finish()
(which is TERMINATE
).
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.
No, this is finish()
(don't get fooled by the exception message which is wrong, too). after()
still has this piece of code: https://github.com/PHP-DI/Silex-Bridge/pull/17/files#diff-c9248b3167fc44af085b81db2e292837R140
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.
Oh right sorry, the way the diff is showing tricked me.
Sorry @jdreesen I was waiting for an answer on one of the comments and didn't see you did. Anyway if it's all good here I'll merge. I'll tag a 2.0.0-beta1! After a few weeks we'll tag a stable release and I'll write a blog post for this. |
Thanks @jdreesen, I've tagged 2.0.0-beta1 -> https://github.com/PHP-DI/Silex-Bridge/releases/tag/2.0.0-beta1 |
Due to the latest changes in this bridge it was a bit tricky to make it compatible with Silex 2 but the tests all pass now.