User tests: Successful: Unsuccessful:
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:
Joomla\CMS\Plugin\CMSPlugin::__construct()
.Joomla\CMS\Extension\PluginInterface
extending Joomla\Event\DispatcherAwareInterface
.Joomla\CMS\Plugin\PluginHelper::importPlugin()
.Joomla\CMS\Extension\PluginInterface::registerListeners()
. Currently nullable to allow J4/J5 compatibility.Plugin developers need to update the registerListeners()
method signature to support both J4 and J5.
Test that plugins continue working. Login, edit content, run smart search indexer, etc.
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
Category | ⇒ | Libraries Unit Tests |
Status | New | ⇒ | Pending |
Labels |
Added:
?
PR-5.0-dev
|
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.
This changes can be done in b/c way, without changing method signatures:
DispatcherAwareInterface
interface$plugin->setDispatcher($dispatcher)
in PluginHelper::import()
, as it already isregisterListeners()
to throw an exception when $this->getDispatcher()
return trash.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.
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
:
PluginInterface
, kind of registerListenersInDispatcher(?DispatcherInterface $dispatcher = null);
registerListeners()
registerListeners()
, untill 6.xAnd 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.
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.
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.
This pull request has been automatically rebased to 5.1-dev.
This pull request has been automatically rebased to 5.2-dev.
Title |
|
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.
This pull request has been automatically rebased to 5.3-dev.
Title |
|
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.