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

Compatibility with Silex 2 #17

Merged
merged 3 commits into from
Jun 2, 2016
Merged

Compatibility with Silex 2 #17

merged 3 commits into from
Jun 2, 2016

Conversation

jdreesen
Copy link
Collaborator

@jdreesen jdreesen commented May 19, 2016

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.

* @param EventDispatcherInterface $dispatcher
* @param string $subscriberClass
*/
private function removeSubscriber(EventDispatcherInterface $dispatcher, $subscriberClass)
Copy link
Collaborator Author

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator Author

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.

@jdreesen jdreesen mentioned this pull request May 19, 2016
@mnapoli
Copy link
Member

mnapoli commented May 19, 2016

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

@mnapoli mnapoli added this to the 2.0 milestone May 19, 2016
@jdreesen
Copy link
Collaborator Author

Oh wow, thx 😊
Will do soon!

private function removeSubscriber(EventDispatcherInterface $dispatcher, $subscriberClass)
{
if (!is_subclass_of($subscriberClass, EventSubscriberInterface::class, true)) {
throw new \InvalidArgumentException('$subscriberClass must implement ' . EventSubscriberInterface::class);
Copy link
Contributor

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?

Copy link
Collaborator Author

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...

@felixfbecker
Copy link
Contributor

Thanks alot @jdreesen for taking the work to adapt this all to Silex 2! Regarding your implementation of HttpKernelServiceProvider, I don't hink a cleaner implementation is possible. It actually is better than what we did before (overriding the whole subscription process). The loop shouldn't be too bad actually as it is asking for the specific event names of the subscriber.

@jdreesen
Copy link
Collaborator Author

I merged master into the silex-2 working branch and split the cleanup commit into another PR (which I'll open when this one is merged) to reduce the noise.
I've been a little busy the last days, sorry for the delay.

@jdreesen
Copy link
Collaborator Author

I'll write unit tests for the new HttpKernelServiceProvider if I find some time, hopefully tomorrow.

@mnapoli
Copy link
Member

mnapoli commented May 25, 2016

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.

@Brammm
Copy link

Brammm commented May 25, 2016

Hey guys, nothing to contribute at the moment, just wanted to let you know, awesome stuff. We're actively using this.

@jdreesen
Copy link
Collaborator Author

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.');
}

Copy link
Member

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?

Copy link
Collaborator Author

@jdreesen jdreesen May 28, 2016

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.

Copy link
Member

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).

Copy link
Collaborator Author

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

Copy link
Member

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.

@mnapoli
Copy link
Member

mnapoli commented Jun 2, 2016

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.

@mnapoli mnapoli merged commit d0e8987 into PHP-DI:silex-2 Jun 2, 2016
@mnapoli
Copy link
Member

mnapoli commented Jun 2, 2016

Thanks @jdreesen, I've tagged 2.0.0-beta1 -> https://github.com/PHP-DI/Silex-Bridge/releases/tag/2.0.0-beta1

@jdreesen jdreesen deleted the silex-2 branch June 2, 2016 17:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants