-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Move additional dependencies from private getters to constructor - Magento_Captcha #26398
Move additional dependencies from private getters to constructor - Magento_Captcha #26398
Conversation
Hi @Bartlomiejsz. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
850f79b
to
e500c3d
Compare
) { | ||
$this->_helper = $helper; | ||
$this->_actionFlag = $actionFlag; | ||
$this->messageManager = $messageManager; | ||
$this->redirect = $redirect; | ||
$this->captchaStringResolver = $captchaStringResolver; | ||
$this->dataPersistor = $dataPersistor; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Bartlomiejsz, thank you for your contribution!
Even if this change doesn't affect a public API, I would recommend to make it as backward compatible as possible, adding the new constructor parameter as last and optional (DataPersistorInterface $dataPersisto = null
), with property initialized with object manager if the parameter is not passed.
e.g.
$this->dataPersistor = $dataPersistor ?: ObjectManager::getInstance()->get(DataPeristorInterface::class);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @aleron75, I can change it if it is required, sure, but I believe this would be great to specify final requirements, because almost each PR is processed differently :)
I.e. #26684 was accepted after modifying constructor to be not backward compatible, and in #26269 I was even asked to modify it and remove object manager usage.
What are your thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check and get back to you soon, thank you for pointing it out
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @Bartlomiejsz here I am; I verified and I apologize, you are right: we can accept backward-incompatible changes for non-api classes, so we could add new arguments without fallback to Object Manager.
Hi @aleron75, thank you for the review. |
✔️ QA Passed |
…structor - Magento_Captcha #26398
Hi @Bartlomiejsz, thank you for your contribution! |
Description (*)
This PR moves additionally introduced dependencies from private getter methods into constructor in Magento_Captcha module.
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)