-
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
Magento_Backend store switcher: Use nowdoc instead of heredoc #32249
Magento_Backend store switcher: Use nowdoc instead of heredoc #32249
Conversation
Hi @fnogatz. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
never thought code styles like that can exists in the core. Mix between js-php with hard to track imho |
@magento create issue |
@magento run all tests |
Hi @bgorski, thank you for the review.
|
@fnogatz thank you for your contribution! |
@magento run Static Tests, Integration Tests, Functional Tests EE, Functional Tests CE, Functional Tests B2B |
@magento run Static Tests, Functional Tests EE |
@magento run Functional Tests B2B |
@magento run Static Tests, Functional Tests EE, Functional Tests B2B |
@magento run Static Tests, Functional Tests EE, Functional Tests B2B |
@magento run Functional Tests EE, Functional Tests B2B |
1 similar comment
@magento run Functional Tests EE, Functional Tests B2B |
Not sure why the bot moved that back to 'ready for testing' after I added the Improvement label, but I manually corrected that. |
Hi @fnogatz, thank you for your contribution! |
Description (*)
The heredoc in Magento/Backend/view/adminhtml/templates/store/switcher.phtml#L203-L216 is not valid, because the embedded JavaScript contains
$this
, which happens to be a valid variable name in PHP, too. Consequently, PHP tries to replace its occurrences by the current class object which results in an error. This can be avoided by using nowdoc instead of heredoc, which is the aim of this PR.In Magento core, the
is_using_iframe
flag is never set. As a consequence, the else-branch in Magento/Backend/view/adminhtml/templates/store/switcher.phtml#L203-L216 is never called, so the error does not occur in a standard environment. However, we use Amasty's Improved Layered Navigation, which sets this flag to true for one of their backend configurations. The resulting error (in developer mode) looks as follows:Related Pull Requests
Two years ago, there was #20656 to replace all heredoc by nowdoc, but it was closed.
Fixed Issues (if relevant)
No separate issue created.
Manual testing scenarios (*)
For reproducing this error, it is not necessary to install Magento or the referenced Amasty extension at all. Simply paste the snippet into a fresh PHP file
test.php
:Calling the script via
php -f test.php
yields a similar error. Changing the first line to<<<'script'
resolves this issue.Questions or comments
Contribution checklist (*)
Resolved issues: