Skip to content

EventListenerToEventSubscriberRector runs also if AsEventSubscriber-Attribute is present#589

Closed
CMH-Benny wants to merge 4 commits intorectorphp:mainfrom
CMH-Benny:testcase/skip_event_listetner_to_event_subscriber_when_as_event_listener_attribute_is_used
Closed

EventListenerToEventSubscriberRector runs also if AsEventSubscriber-Attribute is present#589
CMH-Benny wants to merge 4 commits intorectorphp:mainfrom
CMH-Benny:testcase/skip_event_listetner_to_event_subscriber_when_as_event_listener_attribute_is_used

Conversation

@CMH-Benny
Copy link
Copy Markdown
Contributor

@CMH-Benny CMH-Benny commented Mar 11, 2024

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

@stefantalen
Copy link
Copy Markdown
Contributor

stefantalen commented Mar 11, 2024

Was working on a change because I encountered something similar: #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
Copy link
Copy Markdown
Contributor Author

CMH-Benny commented Mar 11, 2024

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: rectorphp/rector#8532 (comment)

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
Copy link
Copy Markdown
Member

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
Copy link
Copy Markdown
Contributor

stefantalen commented Mar 11, 2024

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
Copy link
Copy Markdown
Contributor Author

CMH-Benny commented Mar 11, 2024

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:

  • Add a EventListenerToAsEventListenerAttributeRector and add it to the fitting SYMFONY_6X set
  • Remove EventListenerToEventSubscriberRector from SYMFONY_CODE_QUALITY set
  • Extend EventListenerToEventSubscriberRector to also convert to EventSubscriber, even if AsEventListener Attribute is present for anyone who want to use it

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
Copy link
Copy Markdown
Contributor Author

Any update on this?

@stefantalen
Copy link
Copy Markdown
Contributor

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:

  • Remove EventListenerToEventSubscriberRector from SYMFONY_CODE_QUALITY set
  • Extend EventListenerToEventSubscriberRector to also convert to EventSubscriber, even if AsEventListener attribute is present for anyone who want to use it

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
Copy link
Copy Markdown
Contributor Author

CMH-Benny commented Mar 27, 2024

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
Copy link
Copy Markdown
Member

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 🙏

@TomasVotruba
Copy link
Copy Markdown
Member

I've cherry picked your fixtures in #616

Thanks 👍

@CMH-Benny
Copy link
Copy Markdown
Contributor Author

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

@CMH-Benny CMH-Benny deleted the testcase/skip_event_listetner_to_event_subscriber_when_as_event_listener_attribute_is_used branch June 27, 2024 14:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants