Seperate js out template to avoid render issue #32434

mrtuvn · 2021-03-11T07:06:24Z

Description (*)

Main purpose of this PR to seperate js out of template avoid mix js-in-php

Previously if we set store to production with minify enabled. When access backend will raise error because phtml can't render properly

Browser screen return
"An error has happened during application run. See exception log for details."

Reference var/log/system.log

Here is the error log when we meet steps scenario
main.CRITICAL: ParseError: syntax error, unexpected '<<' (T_SL) in /var/www/html/var/view_preprocessed/pub/static/app/code/Magento/Backend/view/adminhtml/templates/store/switcher.phtml:44
Stack trace:
0 /var/www/html/lib/internal/Magento/Framework/View/Element/Template.php(273): Magento\Framework\View\TemplateEngine\Php->render()
1 /var/www/html/generated/code/Magento/Backend/Block/Store/Switcher/Interceptor.php(428): Magento\Framework\View\Element\Template->fetchView()

Related Pull Requests

Fixed Issues (if relevant)

Fixes magento/magento2#<issue_number>

Manual testing scenarios (*)

Switch to production mode
bin/magento deploy:mode:set production
Run command to config minify html
bin/magento config:set --lock-env dev/template/minify_html 1
Clear cache and run deploy static content for backend area
Navigate to admin and login with credentials

Questions or comments

Contribution checklist (*)

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds are green)

Resolved issues:

resolves [Issue] Seperate js out template to avoid render issue #32454: Seperate js out template to avoid render issue

m2-assistant · 2021-03-11T07:06:26Z

Hi @mrtuvn. Thank you for your contribution
Here are some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

@magento give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

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.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 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

ihor-sviziev · 2021-03-11T07:43:53Z

app/code/Magento/Backend/view/adminhtml/web/js/store-switcher.js

+], function(jQuery, $t, confirm) {
+    'use strict';
+
+    (function($) {


I have feeling that it should be inside the return function(config) { .... What do you thing?

app/code/Magento/Backend/view/adminhtml/web/js/store-switcher.js

app/code/Magento/Backend/view/adminhtml/templates/store/switcher.phtml

ihor-sviziev · 2021-03-11T07:46:57Z

app/code/Magento/Backend/view/adminhtml/templates/store/switcher.phtml

+                    "useConfirm": <?= /* @noEscape */ $block->getUseConfirm(); ?>,
+                    "isUsingIframe": <?= /* @noEscape */ $block->isUsingIframe(); ?>,
+                    "switchUrl": "<?= $block->escapeUrl($block->getSwitchUrl()); ?>",
+                    "storeId": <?= /* @noEscape */ $block->getStoreId(); ?>


I bevieve we should convert it to int before printing

modified as your suggestion

ihor-sviziev · 2021-03-11T07:50:54Z

@mrtuvn please add a reference to the following conversation c8a5013#commitcomment-48119352 in the description

ihor-sviziev · 2021-03-11T07:51:32Z

app/code/Magento/Backend/view/adminhtml/layout/adminhtml_dashboard_index.xml

+                <arguments>
+                    <argument name="is_using_iframe" xsi:type="string">0</argument>
+                </arguments>


Please explain why you did it?

This variable not set anywhere in code-base so i think we should declare it here

https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Backend/Block/Store/Switcher.php#L614
But it called in this class

Add default value here. Other modules core or 3rd-party providers extensions can easy override it via design area (adminhtml) by layout

setUseConfirm is unneccesary because it's already set via Block

It feels like it supposes to change the following flag:
https://github.com/magento/magento2/pull/32434/files#diff-5c07e420bed1b6e2edda1d6165e1ca2dc4eacaab999bf149eaffa4f53c3823afR47

ihor-sviziev · 2021-03-11T08:46:23Z

app/code/Magento/Backend/view/adminhtml/web/js/store-switcher.js

+                    confirm({
+                        content: $t('Please confirm scope switching. All data that hasn\'t been saved will be lost.'),
+                        actions: {
+                            confirm: function () {
+                                reload();
+                            },
+                            cancel: function () {
+                                obj.value = config.storeId ? config.storeId : '';
+                            }
+                        }
+                    });


maybe that's a bit stupid question, but can we wrap it to require and load 'mage/translate' and 'Magento_Ui/js/modal/confirm' components when this really needed? It should a bit improve page load time

ihor-sviziev · 2021-03-11T08:47:41Z

app/code/Magento/Backend/view/adminhtml/layout/adminhtml_dashboard_index.xml

+                <arguments>
+                    <argument name="is_using_iframe" xsi:type="string">0</argument>
+                </arguments>


It feels like it supposes to change the following flag:
https://github.com/magento/magento2/pull/32434/files#diff-5c07e420bed1b6e2edda1d6165e1ca2dc4eacaab999bf149eaffa4f53c3823afR47

ihor-sviziev · 2021-03-11T12:02:18Z

app/code/Magento/Backend/view/adminhtml/layout/adminhtml_dashboard_index.xml

+                <arguments>
+                    <argument name="use_confirm" xsi:type="string">1</argument>
+                    <argument name="is_using_iframe" xsi:type="string">0</argument>
+                </arguments>


Feels like it won't work, as we don't have this argument in constructor

you can get it via Block class no problem by getData('variable')

https://devdocs.magento.com/guides/v2.4/frontend-dev-guide/layouts/xml-instructions.html

ihor-sviziev · 2021-03-11T13:55:43Z

Looks good to me. Let's wait for the test results.
Note: I don't think there is a way to cover this use case with automated tests, so I added a label that tests are not required.

sivaschenko · 2021-03-11T17:41:44Z

@magento create issue

engcom-Oscar · 2021-04-08T11:50:37Z

✔️ QA passed

Was checked case from Manual testing scenarios

Switch to production mode
bin/magento deploy:mode:set production
Run command to config minify html
bin/magento config:set --lock-env dev/template/minify_html 1
Clear cache and run deploy static content for backend area
Navigate to admin and login with credentials

❌ Before:

✔️ After:
Admin logged-in successfully

m2-assistant · 2021-04-11T20:11:02Z

Hi @mrtuvn, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team added Area: Frontend Component: Backend Release Line: 2.4 labels Mar 11, 2021

mrtuvn force-pushed the move-admin-store-swticher-to-js branch from 89d8256 to 57c95bd Compare March 11, 2021 07:10

m2-community-project bot added the Progress: pending review label Mar 11, 2021

ihor-sviziev requested changes Mar 11, 2021

View reviewed changes

m2-community-project bot assigned ihor-sviziev Mar 11, 2021

ihor-sviziev added the Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. label Mar 11, 2021

m2-community-project bot added Progress: needs update and removed Progress: pending review labels Mar 11, 2021

mrtuvn force-pushed the move-admin-store-swticher-to-js branch from 57c95bd to 87c98a9 Compare March 11, 2021 07:48

ihor-sviziev reviewed Mar 11, 2021

View reviewed changes

m2-community-project bot added Progress: review and removed Progress: needs update labels Mar 11, 2021

mrtuvn force-pushed the move-admin-store-swticher-to-js branch from 87c98a9 to 1a8682e Compare March 11, 2021 08:03

mrtuvn requested a review from ihor-sviziev March 11, 2021 08:20

ihor-sviziev reviewed Mar 11, 2021

View reviewed changes

mrtuvn force-pushed the move-admin-store-swticher-to-js branch 3 times, most recently from 119a2f1 to 80f6682 Compare March 11, 2021 09:22

ihor-sviziev reviewed Mar 11, 2021

View reviewed changes

ihor-sviziev added the Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests label Mar 11, 2021

mrtuvn force-pushed the move-admin-store-swticher-to-js branch from 80f6682 to 895a386 Compare March 11, 2021 15:48

ihor-sviziev mentioned this pull request Mar 30, 2021

Let's discuss approved PRs #32625

Closed

16 tasks

gabrieldagama added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. and removed Priority: P3 May be fixed according to the position in the backlog. labels Apr 6, 2021

engcom-Oscar self-assigned this Apr 8, 2021

m2-community-project bot added the Priority: P3 May be fixed according to the position in the backlog. label Apr 8, 2021

engcom-Oscar added the QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope label Apr 8, 2021

m2-community-project bot added Progress: extended testing and removed Progress: ready for testing labels Apr 8, 2021

engcom-Foxtrot self-assigned this Apr 8, 2021

m2-community-project bot added Progress: ready for testing and removed Progress: extended testing labels Apr 8, 2021

m2-community-project bot added Progress: extended testing and removed Progress: ready for testing labels Apr 8, 2021

m2-community-project bot added Progress: accept and removed Progress: extended testing labels Apr 8, 2021

magento-engcom-team merged commit 89e15b0 into magento:2.4-develop Apr 11, 2021

magento-engcom-team pushed a commit that referenced this pull request Apr 11, 2021

ENGCOM-8928: Seperate js out template to avoid render issue #32434

6966362

gabrieldagama added the Type: Bug Fix label Apr 14, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Seperate js out template to avoid render issue #32434

Seperate js out template to avoid render issue #32434

mrtuvn commented Mar 11, 2021 •

edited

Loading

m2-assistant bot commented Mar 11, 2021

ihor-sviziev Mar 11, 2021

ihor-sviziev Mar 11, 2021

mrtuvn Mar 11, 2021

ihor-sviziev commented Mar 11, 2021 •

edited

Loading

ihor-sviziev Mar 11, 2021

mrtuvn Mar 11, 2021

mrtuvn Mar 11, 2021

mrtuvn Mar 11, 2021 •

edited

Loading

mrtuvn Mar 11, 2021

ihor-sviziev Mar 11, 2021

ihor-sviziev Mar 11, 2021 •

edited

Loading

mrtuvn Mar 11, 2021

ihor-sviziev Mar 11, 2021

ihor-sviziev Mar 11, 2021

mrtuvn Mar 11, 2021

mrtuvn Mar 11, 2021

ihor-sviziev commented Mar 11, 2021

sivaschenko commented Mar 11, 2021

engcom-Oscar commented Apr 8, 2021

m2-assistant bot commented Apr 11, 2021

Seperate js out template to avoid render issue #32434

Seperate js out template to avoid render issue #32434

Conversation

mrtuvn commented Mar 11, 2021 • edited Loading

Description (*)

Related Pull Requests

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Contribution checklist (*)

Resolved issues:

m2-assistant bot commented Mar 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihor-sviziev commented Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrtuvn Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihor-sviziev Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ihor-sviziev commented Mar 11, 2021

sivaschenko commented Mar 11, 2021

engcom-Oscar commented Apr 8, 2021

m2-assistant bot commented Apr 11, 2021

mrtuvn commented Mar 11, 2021 •

edited

Loading

ihor-sviziev commented Mar 11, 2021 •

edited

Loading

mrtuvn Mar 11, 2021 •

edited

Loading

ihor-sviziev Mar 11, 2021 •

edited

Loading