Feature Unit/System Tests b/c break PR-6.0-dev Pending

User tests: Successful: Unsuccessful:

avatar SharkyKZ
SharkyKZ
9 Dec 2022

Summary of Changes

Currently, the use of event dispatcher in plugins has some issues. Joomla\CMS\Plugin\CMSPlugin requires the dispatcher to be passed to the constructor but under normal circumstances it is not used because the dispatcher is always overwritten when importing the plugins using Joomla\CMS\Plugin\PluginHelper::importPlugin(). This is allowed by the fact that Joomla\CMS\Extension\PluginInterface implements Joomla\Event\DispatcherAwareInterface which has a setter method for injecting the dispatcher. This, however, means that for plugins that do not require it in the constructor, the dispatcher is optional and may not be set. This does not make any sense for plugins. The dispatcher must be mandatory and made available explicitly when registering listeners with it.

This PR proposes:

  1. Deprecate the dispatcher as an argument for Joomla\CMS\Plugin\CMSPlugin::__construct().
  2. Deprecate Joomla\CMS\Extension\PluginInterface extending Joomla\Event\DispatcherAwareInterface.
  3. Deprecate not passing the dispatcher to Joomla\CMS\Plugin\PluginHelper::importPlugin().
  4. Add dispatcher argument to Joomla\CMS\Extension\PluginInterface::registerListeners(). Currently nullable to allow J4/J5 compatibility.

B/C break for developers

Plugin developers need to update the registerListeners() method signature to support both J4 and J5.

Testing Instructions

Test that plugins continue working. Login, edit content, run smart search indexer, etc.

Link to documentations

Please select:

  • Documentation link for docs.joomla.org: TBD

  • No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org: joomla/Manual#70

  • No documentation changes for manual.joomla.org needed

avatar joomla-cms-bot joomla-cms-bot - change - 9 Dec 2022
Category Libraries Unit Tests
avatar SharkyKZ SharkyKZ - open - 9 Dec 2022
avatar SharkyKZ SharkyKZ - change - 9 Dec 2022
Status New Pending
avatar SharkyKZ SharkyKZ - change - 9 Dec 2022
The description was changed
avatar SharkyKZ SharkyKZ - edited - 9 Dec 2022
avatar SharkyKZ SharkyKZ - change - 9 Dec 2022
The description was changed
avatar SharkyKZ SharkyKZ - edited - 9 Dec 2022
avatar SharkyKZ SharkyKZ - change - 9 Dec 2022
Labels Added: ? PR-5.0-dev
avatar SharkyKZ SharkyKZ - change - 12 Dec 2022
The description was changed
avatar SharkyKZ SharkyKZ - edited - 12 Dec 2022
avatar SharkyKZ SharkyKZ - change - 12 Dec 2022
The description was changed
avatar SharkyKZ SharkyKZ - edited - 12 Dec 2022
avatar HLeithner
HLeithner - comment - 8 Mar 2023

This PR is not possible, this would break all plugins overriding the registerListeners method, which means you can't have a plugin which supports 4 and 5.

We need this in a way which is b/c.

avatar SharkyKZ
SharkyKZ - comment - 9 Mar 2023

It's true that existing plugins with overridden method would need to be updated to account for the new method signature. But such plugins will work with both 4.0 and 5.0. This is mentioned in the manual. And when interface method signature is changed again in 6.0 to require a dispatcher instance, no changes will be necessary. So a plugin that's updated with 5.0 signature today will end up working with 4.0, 5.0 and 6.0.

We need this in a way which is b/c.

Interesting. Can you post a link to the new versioning policy of Joomla? I've seen claims that it supposedly follows SemVer but that seems to have changed.

avatar Fedik
Fedik - comment - 3 Apr 2023

This changes can be done in b/c way, without changing method signatures:

  • keep DispatcherAwareInterface interface
  • call $plugin->setDispatcher($dispatcher) in PluginHelper::import(), as it already is
  • modify registerListeners() to throw an exception when $this->getDispatcher() return trash.
avatar HLeithner
HLeithner - comment - 3 Apr 2023

@laoneo what would you say?

avatar SharkyKZ
SharkyKZ - comment - 3 Apr 2023

This changes can be done in b/c way, without changing method signatures:

* keep `DispatcherAwareInterface` interface

* call `$plugin->setDispatcher($dispatcher)` in `PluginHelper::import()`, as it already is

* modify `registerListeners()` to throw an exception when `$this->getDispatcher()` return trash.

?‍♂️
The point here is to get rid of the totally misused DispatcherAwareInterface. But, apparently, the project is going backwards, introducing -aware interfaces in places where they absolutely don't belong.

avatar Fedik
Fedik - comment - 3 Apr 2023

Why it is not belong, what is belong then? Sorry, I do not understand, maybe my knowledge just not enogh.
In current case for me no big diference here if it DispatcherAwareInterface or PotatoInterface ;)

I can also sugest other way around, without DispatcherAwareInterface:

  • add new method to PluginInterface, kind of registerListenersInDispatcher(?DispatcherInterface $dispatcher = null);
  • deprecate registerListeners()
  • add use of new method, and proxy it to registerListeners(), untill 6.x

And add description why and when registerListeners() should be removed, and replaced with new method.
Because after 1 year no one will remember what the heck is going on.

avatar Fedik
Fedik - comment - 3 Apr 2023

Hmhm, or you mean that DispatcherAwareInterface should be used only when Class triggers some event?
Well, then it may have a sense, even though it not an obvious thing.
Then a new method should be a less risky change, than changing existing, in current case.

avatar wilsonge
wilsonge - comment - 29 Jun 2023

So I think that removing the Dispatcher as a constructor argument is a good thing.

My only question would be do we want to go down this approach OR to have the dispatcher get the events out (which is how I think this was intended to work originally using the subscriber interface - i.e. https://github.com/joomla/joomla-cms/pull/39387/files#diff-76977b095cc108785d3c5026e26a2df447db05cd1ad28a0a22820cf72573965bR230-R234 ). I always assumed once we removed support for Joomla 3.x style plugins and moved people over to SubscriberInterface that we'd remove the entire registerListeners method.

introducing -aware interfaces in places where they absolutely don't belong

I totally agree with this.

avatar HLeithner
HLeithner - comment - 30 Sep 2023

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

avatar HLeithner
HLeithner - comment - 24 Apr 2024

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

avatar HLeithner HLeithner - change - 24 Apr 2024
Title
[5.0] Remove class-level event dispatcher dependency in plugins
[5.2] Remove class-level event dispatcher dependency in plugins
avatar HLeithner HLeithner - edited - 24 Apr 2024
avatar Hackwar
Hackwar - comment - 24 Jul 2024

I'm sorry, but looking at the registerListener, this wont be able to be merged into 5.x and would have to wait for 6.0 at least.

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] Remove class-level event dispatcher dependency in plugins
[5.3] Remove class-level event dispatcher dependency in plugins
avatar HLeithner HLeithner - edited - 2 Sep 2024
avatar Fedik
Fedik - comment - 2 Mar 2025

@SharkyKZ I think we going to use Lazy Objects for plugins, this is where static getSubscribedEvents() come in handy.
The plugin will be initialized only at time when actual event is triggered.

We should say farewell to registerListeners() because it will always trigger Lazy Objects instantiation.

Please update your PR:

  • revert changes for registerListeners(), it going to be deprecated with #43395
  • resolve conflicts, note that use of DispatcherAwareInterface is deprecated already in #43430

Only issue is need to be resolved, is conditional registration (eg: if admin, if site etc). It seems main reason why some developers override registerListeners() in their plugins.
One of ideas is:

  • #43657 (drawback: it still will instantiate plugin, but only when it is explicitly implemented by plugin)
  • use static calls of Factory::getApplication() inside getSubscribedEvents(), which is not very nice

If you or someone else have a better ideas, please write in that PR.

Thanks!

avatar HLeithner
HLeithner - comment - 4 Mar 2025

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

avatar HLeithner HLeithner - change - 4 Mar 2025
Title
[5.3] Remove class-level event dispatcher dependency in plugins
[6.0] Remove class-level event dispatcher dependency in plugins
avatar HLeithner HLeithner - edited - 4 Mar 2025
avatar SharkyKZ SharkyKZ - change - 11 Apr 2025
Labels Added: Feature Unit/System Tests b/c break PR-6.0-dev
Removed: ? PR-5.0-dev
0cfd4f8 15 Apr 2025 avatar SharkyKZ CS
avatar Fedik
Fedik - comment - 1 Jun 2025

@SharkyKZ If you really want this feature (a possibility to do listeners registration inside the plugin), then it is need a new interface with different method names.
Otherwise there is no way it going to be merged, because of hard b/c break, which prevent of using the same plugin across multiple Joomla version.

You could do something like I tried in:

But with 2 methods:

  • registerEventListeners(DispatcherInterface $dispatcher)
  • unregisterEventListeners(DispatcherInterface $dispatcher)

Or something like that.
Then it probably can be even merged in to 5.4. If other developers don't mind.

Add a Comment

Login with GitHub to post a comment