-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[5.3] Remove class-level event dispatcher dependency in plugins #39387
base: 5.3-dev
Are you sure you want to change the base?
Conversation
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. |
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.
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:
|
@laoneo what would you say? |
🤦♂️ |
Why it is not belong, what is belong then? Sorry, I do not understand, maybe my knowledge just not enogh. I can also sugest other way around, without
And add description why and when |
Hmhm, or you mean that |
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.
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. |
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. |
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 usingJoomla\CMS\Plugin\PluginHelper::importPlugin()
. This is allowed by the fact thatJoomla\CMS\Extension\PluginInterface
implementsJoomla\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
extendingJoomla\Event\DispatcherAwareInterface
.Joomla\CMS\Plugin\PluginHelper::importPlugin()
.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: Plugin changes Manual#70
No documentation changes for manual.joomla.org needed