Skip to content
This repository has been archived by the owner on Dec 3, 2023. It is now read-only.

[SymfonyEventDispatcher] deprecation in favor of contributte #168

Closed
TomasVotruba opened this issue May 16, 2017 · 29 comments
Closed

[SymfonyEventDispatcher] deprecation in favor of contributte #168

TomasVotruba opened this issue May 16, 2017 · 29 comments

Comments

@TomasVotruba
Copy link
Member

TomasVotruba commented May 16, 2017

Hey Guys,

I've got a question for you. Since Symfony 3.3, there have been added new EventDispatcher features, that
dropped added value of SymplifyEventDispatcher for Symfony.

Only usable case remains Nette. There is similar package by @f3l1x :
https://github.com/contributte/event-dispatcher

That does pretty much the same thing. Last few months I've realized I can invest to package that add more than just a few % of added value, instead of maintaining a 90% similar product.

1. So I would be happy to deprecate this package in favor of @f3l1x one. That means, it would be still available for years and current composer supported versions, just not developed.**

My question is - would you consider that a good way to continue or not? And why?

2. I asked @enumag on PM, and he would be ok, just missing ::class based event naming instead of string one.

$this->eventDispatcher(ApplicationRequestEvent::NAME); 
// alows to open event right away

instead of

$this->eventDispatcher(ApplicationEvents::ON_STARTUP);
// contributte, string only

For me, that would a detail, since on my own events I can use ::class regardless the package.

What do you think? About 1. and 2.

/cc @f3l1x @enumag @lexinek @fprochazka

@f3l1x
Copy link

f3l1x commented May 16, 2017

👍


I like this whole thing. Don't split energy to more projects, rather focus on one. If it could be contributte/event-dispatcher I couldn't be happier.


I'm not sure what do you mean with $this->eventDispatcher(ApplicationRequestEvent::NAME);. It's just a name?

@TomasVotruba
Copy link
Member Author

TomasVotruba commented May 16, 2017

I though you'd be happy!


I'm not sure what do you mean with $this->eventDispatcher(ApplicationRequestEvent::NAME);. It's just a name?

The point is not the constant, but the event class. You can click to the event class and see it's arguments and methods. I tried my gif skills 📷

dispatcher

@f3l1x
Copy link

f3l1x commented May 16, 2017

@TomasVotruba I get it now. It might be useful. I could add it to contributte/event-dispatcher too.

@fprochazka
Copy link

FYI I intent to drop the dispatcher integration from Kdyby/Events too, and probably gonna add suggest for contributte :)

@f3l1x
Copy link

f3l1x commented May 16, 2017

@fprochazka That sounds great. Where does this intent came from? :)

Do you mean to drop Kdyby/Evens or just symfony events or only nette packages events?

@fprochazka
Copy link

@f3l1x
Copy link

f3l1x commented May 17, 2017

@fprochazka I get it now. :) Thanks.

@TomasVotruba
Copy link
Member Author

@fprochazka Another point for this. Thank you.

@lexinek What do you think? I'd like to hear your opinion.

@JanMikes
Copy link
Contributor

JanMikes commented May 17, 2017

  1. In this case, as a user i always want the newest version, with bug fixes, new features etc... it is always hard to deprecate package, for me it is sign of that i should not use it anymore
    For example i am using symplify/symbiotic-controller that depends on symplify/symfony-event-dispatcher. Will you update it and replace symplify/symfony-event-dispatcher implementation with the one from @f3l1x ?

If the answer is YES, i am happy and i will approve this instantly for you.
If the answer is NO, what can i, as user, do with that deprecation warning in composer install/update? I want to use a package that depends on deprecated package and i feel like i am doomend (i would never intentionally want to use deprecated package).

  1. Definitely agree, i am using events this way and it is really addictive!

Overall i think this is good way to go, focus on more important things that will bring more added value to community and developers, instead of bringing the wood into forest (maintaining package solving problem that is already solved by other maintained package)

@TomasVotruba
Copy link
Member Author

TomasVotruba commented May 17, 2017

Thank you so much for your complete and easy to read answer.

  1. I say YES to this.

  2. In that case, please create an issue at conttribute package and let talk go on there.

Bringing wood to the forrest exactly describes my feeling. Nice one.

@TomasVotruba
Copy link
Member Author

Resolved via #170

Thank you all for your words, ideas and opinions. It's much easier for me to decide upon many united voices.

@TomasVotruba
Copy link
Member Author

It's all yours now @f3l1x Enjoy ❤️

image

@TomasVotruba
Copy link
Member Author

TomasVotruba commented May 19, 2017

@f3l1x Bit late, but I've just noticed my app stopped working after switching to your bundle.

To me while to figure out only contributte interface are automitically added, not Symfony\Component\EventDispatcher\EventSubscriberInterface. Not sure why, since it is empty.

This is a huge BC break, that would force all people using Symplify extension to change all their subscribers. Would you be open to use original interface?

/cc @lexinek @enumag

@f3l1x
Copy link

f3l1x commented May 20, 2017

Sure, I will change it. :)

@TomasVotruba
Copy link
Member Author

@f3l1x That would be great 🍰

I'm sorry if my tone was too hursh, I had weird and tough night yesterday.

@JanMikes
Copy link
Contributor

I definitely agree.

Btw i had similar bug in my application once and spent 2 hours finding out that in my use statement there is other EventSubscriberInterface than the one from Symfony :-)

@TomasVotruba
Copy link
Member Author

Hehe, that must have hurt. Which one? :D

We might start some PHP Trollo package!

@JanMikes
Copy link
Contributor

JanMikes commented May 20, 2017

In my case it was JMS\Serializer\EventDispatcher\EventDispatcherInterface
TBH i dont know why they have their own interface for this (it is empty, probably same use case as in this repository).
I was writing class ... extends Dispatcher PHPStorm autocompleted for me the JMS namespace was on the first place and without checking i pressed enter and then i was doomed.
It was crazy like hell, wtf factor 9/10, i almost jumped out of the window, i was like "WTF this subscriber works and this just does not" :-)

Imho best practice is to support only symfony event dispatcher interface, as it leads to terrible use cases such as i described, but it would cause BC Break to this (contributte) package.

@enumag
Copy link
Member

enumag commented May 20, 2017

@lexinek Yeah I hate PHPStorm for suggesting the JMS Dispatcher. Is there some way to prevent PHPStorm from doing so?

@JanMikes
Copy link
Contributor

@enumag i did not find any way to do it yet :-/

@TomasVotruba
Copy link
Member Author

@enumag @lexinek Could you try removing it?

@enumag
Copy link
Member

enumag commented May 20, 2017

@TomasVotruba No, we need the bundle where it is.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented May 20, 2017

@enumag So - not the preties solution, but could work - maybe https://pehapkari.cz/blog/2017/01/20/jak-snadno-a-rychle-upravovat-soubory-ve-vendoru/

@enumag
Copy link
Member

enumag commented May 20, 2017

@TomasVotruba The bundle uses the interace internally so the change would be too heavy. This is not a good way to fix an IDE issue.

@TomasVotruba
Copy link
Member Author

TomasVotruba commented May 20, 2017

@enumag Too bad. One of few first things I learnt in Symfony world, was that JMS Bundles are better to skip and manage by own code.

@enumag
Copy link
Member

enumag commented May 20, 2017

Possibly. I didn't personally add it to the codebase so I'll investigate why it's there. It's probably for FOSRestBundle though and I don't know any other good bundle for rest API. Can you recommend any?

@TomasVotruba
Copy link
Member Author

TomasVotruba commented May 20, 2017

I think few guys might know good REST bundles or approach in Symfony /cc @fprochazka @VasekPurchart @mhujer

@fprochazka
Copy link

We've been using friendsofsymfony/rest-bundle + jms/serializer-bundle. And apart from often useless error messages from jms/serializer-bundle, I quite liked the setup. I didn't have to set it up tho, @VasekPurchart did it and I've just been using it, so I haven't really had a chance to hit the juicy stuff everyne is hating on.

@VasekPurchart
Copy link

Yes, our setup relied on JMS, which I agree is quite a pain to use sometimes, but I think this was caused mainly by the fact that the author was not actively developing them. We were using them because of their broad capabilities and quite good extension options and we had some custom made infrastructure set up on it :)

But anyway I didn't find any better combination of bundles to serve my purposes, so I wanted to implement my own, but of course, there was no time, so we were stuck with this. If my next project is a Symfony API though, I'll try to create it :)

@deprecated-packages deprecated-packages locked as resolved and limited conversation to collaborators Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

No branches or pull requests

6 participants