search

rectorphp / rector-symfony

Rector upgrade rules for Symfony
http://getrector.com
MIT License
180 stars 86 forks source link

EventListenerToEventSubscriberRector runs also if AsEventSubscriber-Attribute is present #589

Closed CMH-Benny closed 2 months ago

CMH-Benny commented 6 months ago

Add 2 fixtures to cover Listener-Classes using AsEventListener-Attributes that are going to be changed to EventSubscriber without any need

stefantalen commented 6 months ago

Was working on a change because I encountered something similar: https://github.com/rectorphp/rector-symfony/pull/588

Looks like that will not fix your issue.

Lets elaborate on a solution @TomasVotruba @samsonasik @CMH-Benny

  1. Ignore when attribute is present
  2. Remove attribute when present and convert to subscriber
CMH-Benny commented 6 months ago

Yeah, it's a little confusing for me, I was cleaning a project that was using Listeners and Rector wanted to convert to EventSubscriber, I wondered why I did some digging and ended up creating this issue on the main repo: https://github.com/rectorphp/rector/issues/8532#issue-2175215591

If I follow the reasoning of the conversion to EventSubscriber it was because it doesn't need any additional config, but can be done in the EventSubscriber class - However with the Attribute this also applies to Listeners now and I would assume the Attribute Way even more modern, than the method returning an array within an array to register your event/method/priority - So for me the AsEventListener seems to be superior now. Not sure if there is a case where you still want/have to use the EventSubscriber, tho

samsonasik commented 6 months ago

single rule should handle single purpose, on this case, should be skipped if the AsEventSubscriber present.

New rule should be exists for new conversion.

stefantalen commented 6 months ago

single rule should handle single purpose, on this case, should be skipped if the AsEventSubscriber present.

The purpose of the rule is to convert a Listener to an EventSubscriber, so it should not skip a file if there is an #[AsEventListener] attribute right?

Maybe it would be better to remove the rule from the set, if you want to convert listeners to subscribers you should include it manually. With my draft PR it will then also correctly convert listeners registered through the attribute.

CMH-Benny commented 6 months ago

single rule should handle single purpose, on this case, should be skipped if the AsEventSubscriber present.

New rule should be exists for new conversion.

It's not AsEventSubscriber it's AsEventListener - EventSubscriber doesn't have an Attribute

Maybe it would be better to remove the rule from the set, if you want to convert listeners to subscribers you should include it manually. With my draft PR it will then also correctly convert listeners registered through the attribute.

I think that is also fine, that allows everyone to choose if one wants to convert to EventSubscriber from Any Listener. However it's still usefull for old EventListeners that are registered by config via kernel.listener - So we should not eliiminate the purpose of that Rector to get rid of unnecessary config. But Since AsEventListener Attribute doesn't have that config issue, there is no need to convert it to EventSubscriber in that case

The best solution probably is:

But this is only my 2 cents, I am fine with any solution and if the solution is to skip the EventListenerToEventSubscriberRector rule if I don't want to loose my AsEventListener Attributes, then that's how it is, but I think if Rector wants to help to modernize Code, going for Attributes makes the most sense

CMH-Benny commented 5 months ago

Any update on this?

stefantalen commented 5 months ago

Converting an EventListener to an EventSubscriber and replacing EventListeners with #[AsEventListener] are two seperate things (with their own use cases).

I'll see if I can wrap up my PR within a week to include:

It'll be up to the developer to include the rule or not 🙂

Maybe you can work on a EventListenerToAsEventListenerAttributeRector?

I don't think it should be part of a Symfony set, since using the attribute is still optional

CMH-Benny commented 5 months ago

Thanks for your answer, I am still not sure about that. Should that Rule really be dropped from the CODE_QUALITY? The idea of it was to ensure people don't use the old way with service tags somewhere configured in a config file, but to use the EventSubscriberInterface to have the config basically in the same class as the Subscriber. However, for the new AsEventListener Attribute this is no longer special to EventSubscriber, since the Attribute basically also puts the config in the Listener class. So there are 2 solutions now that would fit into that CODE_QUALITY (Mind)Set: AsEventListener-Attribute approach and the EventSubscriberInterface. So not sure which one is the better pick for that Set.

I lately migrated a symfony 5.4 project to symfony 6.4 and Presets for SYMFONY_6{0-4} started to convert MessageHandlers, Commands etc. to use Attributes, so such rules are part of the symfony rule sets already. For some reason it doesn't apply to AsEventListener Attributess (yet). So on one side I think it's not a good idea, to drop that EventSubscriberRector from the CODE_QUALITY, however I also highly doubt that it should revert back AsEventListener Attributes into EventSubscriberInterface. I am also not sure, what is considered best practice on that, as already stated we have two conflicting soutions that fulfil both the reasoning why they should be in the CODE_QUALITY set.

I think if someone is already using the AsEventListener Attribute, there is no need to convert that to a SubsciberInterface and it would be even more correct if rector would convert old Listener usages into those AsEventListener Attributes instead, because it would maintain the concept the developer picked for his implementation. To be honest I am not fully sure what the Difference between an EventListener and an EventSubscriber really is and when to use which, they seem to have the same idea. This probably goes beyond the scope of my understanding and I can only hope Symfony itself will make a decision at some point and deprecates all the others, but until then... Idk.

I for myself disabled/skipped the EventSubscriberRector so it doesn't destroy my Attributes. I don't see a reason why I would want to use the Interface over the Attribute, especially since Rector actively changed towards Attribute in other cases. Probably because the usage of the Interfaces was actively deprecated already and this is not yet the case for Listeners/Subscribers.

I just wanted to bring that topic to attention, sadly I won't have time to dig into the creation of rector rules for a while, so I am fine with every solution. If you are going to convert to EventSubscriber even if people use attributes, people can at any time decide to disable that rector as I did and maybe at some point the AsEventListener-Attribute becomes the best practice and Rector will adjust towards it. Maybe it's just too early for the AsEventListener-Attribute right now 😄

Thanks for your efforts, rector is really an amazing tool and if I ever get some time to dig deeper into this whole thing, I'd be happy to contribute at some point ❤️

TomasVotruba commented 2 months ago

Hi, thanks for patience with this. I was focusing on other issues, but I'm giving this priority now.

Goal of this rule is to move configuration logic from some config out there, directly to PHP code. Both EventSubscriberInterface and #[AsEventListener] make it happen.

https://symfony.com/doc/current/event_dispatcher.html#event-dispatcher_event-listener-attributes

#[AsEventListener(event: CustomEvent::class, method: 'onCustomEvent')]
#[AsEventListener(event: 'foo', priority: 42)]
#[AsEventListener(event: 'bar', method: 'onBarEvent')]
final class MyMultiListener

So we should go with:

Ignore when attribute is present

I'll process this today. Thanks again for your patience :pray:

TomasVotruba commented 2 months ago

I've cherry picked your fixtures in https://github.com/rectorphp/rector-symfony/pull/616

Thanks :+1:

CMH-Benny commented 2 months ago

Thank you so much! I will delete my branch then, happy to hear that my fixtures helped you =)