[#28808] - [4.0] Fix captcha 2: use private dispatcher
- Closed
- 27 Apr 2020
- Medium
- Build: 4.0-dev
- # 28808
- Diff
- Fedik:captcha-fix2
User tests: Successful: Unsuccessful:
Summary of Changes
This redo of #28785 but with events.
ping @wilsonge
Testing Instructions
Apply patch and make sure captcha works
Expected result
works
Actual result
works
Side note
@wilsonge I think Joomla have a little issue in events API, would be good to have some api to distinct private and global events.
Example the captcha has next events:
global: onPrivacyCollectAdminCapabilities
local: onInit, onDisplay, onCheckAnswer, onSetupField.
The global events should be loaded while PluginHelper::importPlugin('captcha');
And local only while instantiate of Joomla\CMS\Captcha\Captcha class.
Same goes for editor.
Currently if someone do:
PluginHelper::importPlugin('captcha');
PluginHelper::importPlugin('editors');
$app->dispatch('onInit');That will call onInit for all active editors and cpatcha plugins, that of course make no sense.
This is not critical, but an old issue
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
In the end it still an events
not usual (in joomla context) but still:
Captcha::getInstance()->getDispatcher()
->addListener('onInit', ['foobar' => 'doSomething'])“onInit” should be replaced with properly identifying event names.
This is much to do and b.c. and stuff, I am to lazy for it
The fact you have to manually subscribe to a second dispatcher is exactly why this is problematic. A plugin should NEVER EVER EVER EVER have to manually call $dispatcher->addListener() and if it is a requirement then the core architecture is FUBAR.
Fact is, onInit needs to be deprecated as an event name and replaced with captcha.init and editor.init as event names (don't get me started with this "onFoo" naming convention, that's only a limitation of the 3.x and earlier code directly mapping event names to methods on a listener class). It gets rid of the ambiguity and issue of two incompatible subsystems emitting the same event. A short term fix until unique events are the only way is to include context information in the emitted event arguments (the same way all the "onContentFoo" stuff works) and tell developers to check context (like they should be on every event).
Using a "private" dispatcher might monkey patch your way around the original problem, but it is honestly just introducing a new problem in the grand scheme of things. You're better off keeping the 3.x style code over introducing another broken mechanism into the 4.0 code that would go away if someone had the foresight into fixing architectural issues like this in 5.0.
okau, that maybe makes sense, in Joomla context
I better close it for now
| Status | Pending | ⇒ | Closed |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-04-27 08:59:19 |
| Closed_By | ⇒ | Fedik | |
| Labels |
Added:
?
|
||
You might as well rewrite captcha as not being plugins if you’re going this way. An event is an event, it really doesn’t matter whether it is emitted through a dispatcher with 100 listeners or 2 listeners.
You are misunderstanding the concept of an event system. An event is emitted so that anyone may subscribe to it to perform an action. Any possible listener may choose to act on an event or ignore it. It is not the responsibility of the code emitting the event to specify which listeners are eligible to receive the event.
The screw up in core is not using unique event identifiers, meaning you have things like “onInit” which have two totally different meanings to two totally different systems. “onInit” should be replaced with properly identifying event names.