-
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
Telephone Widget > Template > Removing this and helper #25533
Telephone Widget > Template > Removing this and helper #25533
Conversation
Hi @rafaelstz. Thank you for your contribution
For more details, please, review the Magento Contributor Guide documentation. |
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.
Hi @rafaelstz. Thanks for collaboration. Please take a look at my review comment.
* | ||
* @return string | ||
*/ | ||
public function getValidationClass() |
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.
Why we need this method? We can use directly $this->getAttributeValidationClass('telephone').
ced6e3f
to
3c05965
Compare
Hi @VladimirZaets |
Hi @rafaelstz. First of all, we can't change the interface of the public method because it's backward-incompatible changes. You are right, in magento we use this class just for telephone widget template, but our extension developers and customers can use/extend this class in they implementations and your fix can break them. |
ca0c4f2
to
53515bd
Compare
Hi @VladimirZaets, I've updated the pull-request to avoid backward-incompatibility. |
This functionality is already covered by integration test. |
Hi @VladimirZaets, thank you for the review. |
✔️ QA Passed |
Hi @rafaelstz, thank you for your contribution! |
Description (*)
I provided this PR to fix the issue that I had in the PR #25321, to follow the Magento Coding Standards I'm removing the
helper
and$this
from the template file.I used a method in block class to enable me to do it instead of calling the
helper
directly in the template file. I created a new method as well to get the telephone validation directly as this class and phtml are dedicated to the telephone widget.Fixed Issues (if relevant)
helper
and$this
into the template fileContribution checklist (*)