Feature PR-5.2-dev Pending

User tests: Successful: Unsuccessful:

avatar Fedik
Fedik
28 Apr 2024

Pull Request for Issue # .

Summary of Changes

The PR deprecate CMSPlugin::registerListeners() as no longer needed when plugin will implement SubscriberInterface
(I wonder why it was not marked as deprecated before).

Testing Instructions

Apply PR, and navigate around the site, all should work as before.

The PR requires review from maintainers.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:
  • No documentation changes for docs.joomla.org needed
  • Pull Request link for manual.joomla.org: TBD
  • No documentation changes for manual.joomla.org needed

Ping @HLeithner @wilsonge

avatar Fedik Fedik - open - 28 Apr 2024
avatar Fedik Fedik - change - 28 Apr 2024
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 28 Apr 2024
Category Libraries
avatar Fedik Fedik - change - 28 Apr 2024
The description was changed
avatar Fedik Fedik - edited - 28 Apr 2024
avatar Fedik Fedik - change - 28 Apr 2024
The description was changed
avatar Fedik Fedik - edited - 28 Apr 2024
avatar Fedik Fedik - change - 28 Apr 2024
Labels Added: Feature PR-5.2-dev
avatar HLeithner
HLeithner - comment - 28 Apr 2024

make sense for me

avatar laoneo
laoneo - comment - 28 Apr 2024

I guess it was not marked as deprecated before becaus we changed the plugins way later.

avatar wilsonge
wilsonge - comment - 28 Apr 2024

The constructor change should be done using @SharkyKZ 's PR here #39387

avatar Fedik
Fedik - comment - 29 Apr 2024

The constructor change should be done using @SharkyKZ 's PR here #39387

Yeah, I have copied that code from him, as I wrote in the description.
When he will update his PR (to make it b/c), I will remove it from my PR.

avatar SharkyKZ
SharkyKZ - comment - 2 May 2024

Why are you hellbent on neutering plugins again? It's bad enough that you have already poorly implemented event classes while also introducing a number of undocumented B/C breaks without a deprecation period as well as garbage code like #40037 (comment). What benefit do you actually see in forcing plugins to use only SubscriberInterface? Is there actually some plan to make subscribers useful in the future or are you removing custom listeners just because?

avatar Fedik
Fedik - comment - 2 May 2024

Why are you hellbent on neutering plugins again?

I would like to see the plugins to be as light as possible (there still a long way to go).

poorly implemented event classes while also introducing a number of undocumented B/C breaks

Please more detailed. From quick look on the link, it is probably some bug in implementation, or misuse of the event, which was hiding the bugs in the past.

garbage code like ...

There a loot more garbage ?
But only who make nothing makes no mistakes ?

What benefit do you actually see in forcing plugins to use only SubscriberInterface?

Perfomance, and better code, did you seen code of registerListener and registerLegacyListeners?

But it was set to be this way before, in past, somewhere. From my understanding, it was planed to be similar to Symfony SubscriberInterface.

When we drop legacy listeners and use Dispatcher which will look for SubscriberInterface, then having registerListeners does not make sense to me.

Is there actually some plan to make subscribers useful in the future or are you removing custom listeners just because?

What is NOT usefull for you?

If you have some suggestion to improve it, or alternative ideas, just write it ?

avatar SharkyKZ
SharkyKZ - comment - 3 May 2024

Virtually none of what you said is valid. How exactly does removing support for custom listeners make plugins "as light as possible"? Why did you even mention registerLegacyListeners? It only exists for compatibility with J3 plugins so it's not relevant at all. And what poor performance or bad code do you see in registerListener method? It's literally the correct and most performant way to register listeners. The doc block does say it's the preferred way and should be the only way... So again I have to ask what actual benefit do you see in using subscribers? Because, unless you're cooking up some more Joomla! magic™, subscribers are always going to be slower than custom listeners. Plugins implementing SubscriberInterface are monolithic classes that have to have all their dependencies set up early on, even if their listeners are not used. Meanwhile, normal plugins can use any callable as a listener. Thus they can delegate the actual logic to other classes which can be lazy loaded when a given event is triggered. The event package does already provide Joomla\Event\LazyServiceEventListener class for doing something like that.

avatar Fedik
Fedik - comment - 3 May 2024

Virtually none of what you said is valid

Virtually everything I said is correct, except things that are not correct. Easy.

How exactly does removing support for custom listeners make plugins

Removing $reflectedObject = new \ReflectionObject($this);

And what poor performance or bad code do you see in registerListener

Sorry, I mean registerListeners, but registerListener also useless piece of code.
BTW, you said plugin should not implement DispatcherAwareInterface, but registerListener relies on this.
So, sorry I am cannot follow you here.

Plugins implementing SubscriberInterface are monolithic classes that have to have all their dependencies set up early on, even if their listeners are not used. Meanwhile, normal plugins can use any callable as a listener. Thus they can delegate the actual logic to other classes which can be lazy loaded when a given event is triggered.

Well, because we have all this bootPotato() stuff, developers can implement BootableExtensionInterface on it,
and do what they want with their "lazy" or "not very much lazy" stuff. I do not see problem here.

The event package does already provide Joomla\Event\LazyServiceEventListener class for doing something like that.

Keeping registerListeners does not help with it.
This "something like that", what no one know it exists and how to use it.

To me, removing registerListeners will turn the plugin to a "bare" entity, and the developers can decide if they want to implement SubscriberInterface or BootableExtensionInterface (for lazy stuff) or anything else.
But that is still an open topic (how to make it better), and does not prevent us from registerListeners deprection.

avatar SharkyKZ
SharkyKZ - comment - 4 May 2024

Removing $reflectedObject = new \ReflectionObject($this);

?‍♂️ Why are you bringing up J3 code again? You still don't understand it's irrelevant? It will be removed when J3 plugin support is dropped, regardless whether registerListeners remains or not.

Sorry, I mean registerListeners, but registerListener also useless piece of code.
BTW, you said plugin should not implement DispatcherAwareInterface, but registerListener relies on this.
So, sorry I am cannot follow you here.

You missed the part where the dispatcher would be passed to the method. But nevermind that, registerListener() is indeed a useless shortcut. It has similar limitations as SubscriberInterface in that in can only register public methods of the plugin class and not proper callables. Anyways, it's an implementation detail of CMSPlugin so its removal doesn't have a huge impact; plugin developers can re-implement it if they need it.

Well, because we have all this bootPotato() stuff, developers can implement BootableExtensionInterface on it,
and do what they want with their "lazy" or "not very much lazy" stuff. I do not see problem here.

That doesn't make sense. Not only is it not the right to place to register listeners but it also wouldn't work because the correct dispatcher instance is passed to the plugin after the plugin has been booted. This is correct and expected behavior but it's the way the dispatcher is passed that needs to be fixed.

This "something like that", what no one know it exists and how to use it.

What kind of argument is this supposed be? LazyServiceEventListener is not some sort of secret. Like any other class, it's part of Joomla's public API, it's already used in core and by 3rd party plugins. This just shows that you are unfamiliar with the event codebase and you're trying to introduce changes you don't understand with negative consequences to core and 3rd party developers.

avatar Fedik
Fedik - comment - 5 May 2024

Whatever, you just complaining about anything.

registerListeners need to be removed or replaced, and for this it need to be deprecated.
btw, if we going to keep setDispatcher() in CMSPlugin, it is already a replacement, kind of.

avatar joomla-cms-bot joomla-cms-bot - change - 6 May 2024
Category Libraries Libraries Front End Plugins
avatar Fedik
Fedik - comment - 6 May 2024

I have split the PR to 2:

Please review.
I close here for now, if they is not good I will reopen current.

avatar Fedik Fedik - close - 6 May 2024
avatar Fedik Fedik - change - 6 May 2024
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2024-05-06 12:11:52
Closed_By Fedik
avatar Fedik Fedik - change - 12 May 2024
Status Closed New
Closed_Date 2024-05-06 12:11:52
Closed_By Fedik
avatar Fedik Fedik - change - 12 May 2024
Status New Pending
avatar Fedik Fedik - reopen - 12 May 2024
avatar joomla-cms-bot joomla-cms-bot - change - 12 May 2024
Category Libraries Front End Plugins Libraries
avatar Fedik Fedik - change - 12 May 2024
Title
[5.2] CMSPlugin: deprecation for registerListeners and dispatcher dependency
[5.2] CMSPlugin: deprecation for registerListeners
avatar Fedik Fedik - edited - 12 May 2024
avatar Fedik Fedik - change - 12 May 2024
The description was changed
avatar Fedik Fedik - edited - 12 May 2024
avatar Fedik
Fedik - comment - 12 May 2024

For now, the replacement for deprecated registerListeners() method is in follow up PR:

avatar Fedik Fedik - change - 16 Jul 2024
The description was changed
avatar Fedik Fedik - edited - 16 Jul 2024
avatar degobbis degobbis - test_item - 24 Aug 2024 - Tested successfully
avatar degobbis
degobbis - comment - 24 Aug 2024

I have tested this item ✅ successfully on 267054c


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43395.

avatar HLeithner
HLeithner - comment - 2 Sep 2024

This pull request has been automatically rebased to 5.3-dev.

avatar HLeithner HLeithner - change - 2 Sep 2024
Title
[5.2] CMSPlugin: deprecation for registerListeners
[5.3] CMSPlugin: deprecation for registerListeners
avatar HLeithner HLeithner - edited - 2 Sep 2024

Add a Comment

Login with GitHub to post a comment