[7.0] Move to captcha registry#47939
Conversation
|
This should work but I think it is a bit rush to drop legacy captcha support in J7. We only have Proof of Work captcha from 6.1, and developers have no clue how new captcha plugin looks like before. I think most of captcha plugins still use legacy structure these days and will need to be re-written to be compatible with J7. |
|
You are right, I changed the code to fallback to the old captcha. |
| $this->namespace = $this->element['namespace'] ? (string) $this->element['namespace'] : $this->form->getName(); | ||
|
|
||
| try { | ||
| $this->_captcha = Factory::getContainer()->get(CaptchaRegistry::class)->get($plugin); |
There was a problem hiding this comment.
It was planned to be used kind of following:
$captcha = new Joomla\CMS\Captcha\Captcha($name, $options);
$captcha->checkAnswer(); // For validation
echo $captcha->display(); // For displayDo not access CaptchaRegistry dirrectly, please.
There was a problem hiding this comment.
When should then the registry be used? Looks better to use a registry from the DI container than directly creating an instance of Captcha.
There was a problem hiding this comment.
Can provide registry when you create new instance of the Joomla\CMS\Captcha\Captcha.
$captcha = new Joomla\CMS\Captcha\Captcha($name, [
'registry' => $myRegistry,
]);joomla-cms/libraries/src/Captcha/Captcha.php
Lines 82 to 83 in d1118be
There was a problem hiding this comment.
Why do you want to keep the Captcha class? Looks for me that it doesn't have any meaningful usage. better to drop that class completely and rely only on the registry.
There was a problem hiding this comment.
In theory we could drop it, yes.
I would like to keep it as kind of proxy, to simplify future modification when we will be need it.
Also this is much safer for b/c.
I think no need to hurry with removing it.
There was a problem hiding this comment.
I don't hurry to remove it, all I did was to deprecate it. To use the registry, it is more inline with the rest of our code base.
There was a problem hiding this comment.
I mean I wrote that code to work like I comented before.
There was a problem hiding this comment.
With your changes you introduce many unnessesary checks and try/catch.
It does not look nice.
As I understand you want to deprecate $namespace property. From what I see this property is ignored in Captcha class already.
I think you can just add deprecation comment and leave rest of the code as it was. We will remove $namespace related code in future when the property will be removed.
Summary of Changes
Makes the final move to the captcha registry and deprecates the whole
Captchafacade class.Testing Instructions
Actual result BEFORE applying this Pull Request
Captcha works.
Expected result AFTER applying this Pull Request
Captcha works.
Link to documentations
Please select:
Documentation link for guide.joomla.org:
No documentation changes for guide.joomla.org needed
Pull Request link for manual.joomla.org: Move to captcha registry Manual#664
No documentation changes for manual.joomla.org needed