-
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
Add frontend template hints status command #26451
Conversation
Hi @WaPoNe. 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.
Please find comments to PR.
app/code/Magento/Developer/Console/Command/TemplateHintsShowCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsShowCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsShowCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsShowCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsShowCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
The checks are failing for some reasons that not depend by my files. |
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.
Comments are not necessary to const
:-)
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
@lbajsarowicz any news? |
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.
Good job! Only the last changes to be performed and we'll be done.
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
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.
Cosmetics :-)
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
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.
OK. Let's wait for all tests pass.
PR approved.
Parenthesis for return() is not necessary, but generally it's ok.
Hi @lbajsarowicz, thank you for the review.
|
app/code/Magento/Developer/Test/Unit/Console/Command/TemplateHintsStatusCommandTest.php
Show resolved
Hide resolved
app/code/Magento/Developer/Test/Unit/Console/Command/TemplateHintsStatusCommandTest.php
Show resolved
Hide resolved
app/code/Magento/Developer/Test/Unit/Console/Command/TemplateHintsStatusCommandTest.php
Show resolved
Hide resolved
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.
I see some regression, but we are really close!
app/code/Magento/Developer/Test/Unit/Console/Command/TemplateHintsStatusCommandTest.php
Show resolved
Hide resolved
{ | ||
$this->reinitableConfig->reinit(); | ||
$templateHintsStatus = | ||
$this->scopeConfig->isSetFlag(self::TEMPLATE_HINTS_STOREFRONT_PATH, 'default') |
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.
Wasn't that line extracted to private method?
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.
You're right @lbajsarowicz but I was able to test that private method that gave me strange errors.
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.
@lbajsarowicz I brought it back and changed unit tests to verify single interfaces.
app/code/Magento/Developer/Console/Command/TemplateHintsStatusCommand.php
Outdated
Show resolved
Hide resolved
You should not test private methods, but output of the interface, as unit tests are to test single interfaces, not single methods. |
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.
OK.
Hi @lbajsarowicz, thank you for the review. |
✔️ QA Passed |
@magento run all tests |
… 2.4-develop_WaPoNe
Hi @WaPoNe, thank you for your contribution! |
Description (*)
Add frontend template hints status command
Fixed Issues (if relevant)
This is a simple command to show the frontend template hints status in adding to dev:template-hints:enable and dev:template-hints:disable commands.
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)