#10402 fix theme templates overrides in environment emulation cross-area #10403

alessandroniciforo · 2017-08-02T09:36:39Z

Row 141 and 145 are obviously returning a theme id, so we'd better load the theme by id. I'm not sure if execution will fall into row 143 what to expect.

Fixed Issues (if relevant)

Theme templates overrides don't work in environment emulation cross-area #10402: Theme templates overrides don't work in environment emulation cross-area

orlangur · 2017-08-02T09:49:41Z

I'm not sure if execution will fall into row 143 what to expect.

In such case $theme will not contain themeId and suggested code will fail :) While original one worked...

orlangur · 2017-08-02T09:50:47Z

Note that magento:2.1-develop target branch should be used for backports of pull requests already merged to develop branch only.

orlangur · 2017-08-02T09:55:39Z

lib/internal/Magento/Framework/View/Asset/Repository.php

Please rewrite method in such a way that if we go through line 141 getThemeById call is used and getThemeByFullPath otherwise.

Also, such case with emulated cross-area needs to be covered with integration test.

Actually, also for line 145 getThemeById should be called as getConfigurationDesignTheme returns the id. I don't have time to write integration test at this moment, unfortunately.
Should I open another PR on develop branch?

I changed base branch, please force push into the same branch applying your commit on top of develop.

Yeah, you are right about line 145.

Just finish your implementation without integration test then.

…cross-area Row 141 and 145 are obviously returning a theme id, so we'd better load the theme by id. I'm not sure if execution will fall into row 143 what to expect.

… cross-area Load theme by full path only in case we received a 'theme' in $params

…nParams

alessandroniciforo · 2017-08-03T07:32:17Z

Pushed the suggested change and the fix to the unit test.

orlangur · 2017-08-03T10:42:04Z

Please try to simplify modified method

1) Magento\Test\Php\LiveCodeTest::testCodeMess
PHP Code Mess has found error(s):
/home/travis/build/magento/magento2/lib/internal/Magento/Framework/View/Asset/Repository.php:130	The method updateDesignParams() has an NPath complexity of 240. The configured NPath complexity threshold is 200.
Failed asserting that 2 matches expected 0.

Maybe we can leave only one exception throwing and decrease amount of ifs?

…the requested area

… readability

alessandroniciforo · 2017-08-03T16:35:12Z

Just moving the exception throwing was not enough to go below 200. On top of that, I found that in case we load the theme by id we should ensure that the theme exists in the requested area (this is implicit in the load by full path), so I added that check and refactored a bit.
Hopefully it looks better now.

…he theme param returned by getConfigurationDesignTheme is an id or a path

alessandroniciforo · 2017-08-04T09:15:08Z

The issue unveiled by the integration tests run is that Magento\Theme\Model\View::getConfigurationDesignTheme() can either return:

theme id: if execution pass into rows #174 or #179 AND if the design configuration has been saved on the backend, thus inserting the config line into core_config_data
theme path: in all the other cases. There's a fallback at row #188 so that at worst it takes the theme passed in its constructor, injected in its di.xml, which is a theme path

So my (ugly) solution is that, in case we rely on Magento\Theme\Model\View::getConfigurationDesignTheme(), we should detect whether the returned value is an id or a path.

A more appropriate solution would be to modify the implementation of getConfigurationDesignTheme to have a mean of knowing whether the returned value is an id or a path, but I'm not confident enough to refactor that part too.

orlangur · 2017-08-04T11:12:18Z

lib/internal/Magento/Framework/View/Asset/Repository.php

-            }
-        } elseif (empty($params['themeModel'])) {
+        if (empty($params['themeModel'])) {
            $params['themeModel'] = $this->getDefaultParameter('themeModel');


For better readability please do $themeModel = ... instead of $params['themeModel'] = ... and only in the very end do $params['themeModel'] = $themeModel ?: $this->getDefaultParameter('themeModel').

orlangur · 2017-08-04T11:15:22Z

lib/internal/Magento/Framework/View/Asset/Repository.php

+     * @return \Magento\Framework\View\Design\ThemeInterface
+     */
+    private function getTheme($theme, $area)
+    {


Yeah, looking into \Magento\Theme\Model\View\Design::getConfigurationDesignTheme this seems to be the best option to me (although we won't support string theme identifiers consisting of digits only).

Can we eliminate this method and still pass PHPMD? getThemeById and getThemeByPath seem much more understandable to me at the place of call but I would like to avoid method named like getThemeByIdOrPath.

Moving this in the method we wouldn't pass the cyclomatic complexity

orlangur · 2017-08-04T11:16:01Z

lib/internal/Magento/Framework/View/Asset/Repository.php

+    private function getThemeByPath($themePath, $area)
+    {
+        $themeModel = $this->getThemeProvider()->getThemeByFullPath($area . '/' . $themePath);
+        if (!$themeModel) {


Please reword Could not find theme by full path ...

orlangur · 2017-08-04T11:16:18Z

lib/internal/Magento/Framework/View/Asset/Repository.php

+    private function getThemeById($themeId, $area)
+    {
+        $themeModel = $this->getThemeProvider()->getThemeById($themeId);
+        if (!$themeModel || $themeModel->getArea() != $area) {


Please reword Could not find theme by id ...

orlangur · 2017-08-04T11:18:51Z

lib/internal/Magento/Framework/View/Test/Unit/Asset/RepositoryTest.php

+        $this->themeProvider
+            ->expects($this->any())
+            ->method('getThemeByFullPath')
+            ->with('frontend/Magento/luma')


Please avoid Luma theme mentioning in framework, just use some fictional theme path (fix both occurrences).

orlangur · 2017-08-04T11:20:59Z

lib/internal/Magento/Framework/View/Asset/Repository.php

        if (empty($params['area'])) {
            $params['area'] = $this->getDefaultParameter('area');
        }



As this method was refactored significantly, please remove @SuppressWarnings(PHPMD.CyclomaticComplexity), probably it is already passing this check.

orlangur · 2017-08-04T11:22:57Z

lib/internal/Magento/Framework/View/Asset/Repository.php

+        return $themeModel;
+    }
+
+    /**


Change to int.

orlangur · 2017-08-04T11:27:07Z

Thanks, your findings look great to me, especially that there was theme_id in some tests not containing number and that there are actually some integration and functional tests enforcing this logic.

Would be great if you add case to check new logic into some existing integration test but it's not mandatory. Please check my comments to slightly polish up the code.

orlangur · 2017-08-09T17:17:37Z

@alessandroniciforo there is one real failure left: https://travis-ci.org/magento/magento2/jobs/261023165

There was 1 failure:
1) Magento\Deploy\DeployTest::testDeploy
Failed asserting that 3266.36328125 is less than 1177.6.
/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Deploy/DeployTest.php:232
/home/travis/build/magento/magento2/dev/tests/integration/testsuite/Magento/Deploy/DeployTest.php:151
                                                                        
FAILURES!                                                               
Tests: 1291, Assertions: 5716, Failures: 1, Incomplete: 43, Skipped: 11.

Could you please look into it?

ishakhsuvarov · 2017-08-30T09:24:15Z

@alessandroniciforo @orlangur Is there any progress on this Pull Request?

orlangur · 2017-08-30T09:57:13Z

@ishakhsuvarov I planned to look into this failure if Alessandro will not do it as fix seems quite valuable to me. Didn't have time yet, will have during this weekend.

Feel free to investigate failure yourself (maybe it occurred already in some other circumstances already?) or close this PR for now, I'll reopen when ready.

okorshenko · 2017-10-31T21:38:10Z

Hi All
We are closing this PR due to inactivity. If you would like to proceed with this PR, please reopen it any time

diamondavocado · 2019-06-04T16:21:20Z

@orlangur Could you please re-open this PR? It was very close to being finished and it would solve issues #10402 and #8884.

orlangur · 2019-06-04T16:45:06Z

@diamondavocado sure, not sure how it could help though since you are not an author :)

m2-assistant · 2019-06-04T16:45:10Z

Hi @alessandroniciforo. Thank you for your contribution
Here is 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.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

magento-cicd2 · 2019-06-04T16:45:13Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.

alessandroniciforo · 2019-06-11T22:22:39Z

Hey guys,
sorry but I couldn't understand the reason for the failure of the integration test and I've since moved away from Magento.
Looking at the PR now, it seems that the issue has been solved already on 2.3-develop by this commit d3e32b4.
I don't know if it makes sense to merge it to 2.1 (which I don't know if it's still maintained) or to 2.2

sidolov · 2019-08-16T19:43:38Z

Hi @alessandroniciforo , thank you for your contribution! Unfortunately, we are no longer accept contributions to 2.1-develop and 2.2-develop branches. Looks like the issue was fixed in the scope of d3e32b4, as you mentioned before.
I'm closing this PR as not relevant.
Thank you!

m2-assistant · 2019-08-16T19:43:40Z

Hi @alessandroniciforo, 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.

orlangur self-assigned this Aug 2, 2017

orlangur requested changes Aug 2, 2017

View reviewed changes

ishakhsuvarov added this to the August 2017 milestone Aug 2, 2017

ishakhsuvarov added the Release Line: 2.1 label Aug 2, 2017

orlangur changed the base branch from 2.1-develop to develop August 2, 2017 10:01

alessandroniciforo and others added 2 commits August 2, 2017 12:11

magento#10402 fix theme templates overrides in environment emulation …

030b6c9

…cross-area Row 141 and 145 are obviously returning a theme id, so we'd better load the theme by id. I'm not sure if execution will fall into row 143 what to expect.

magento#10402 fix theme templates overrides in environment emulation…

72ed28e

… cross-area Load theme by full path only in case we received a 'theme' in $params

alessandroniciforo force-pushed the 2.1-develop branch from 2c83345 to 72ed28e Compare August 2, 2017 10:28

orlangur added develop and removed Release Line: 2.1 labels Aug 2, 2017

magento#10402 fix unit tests to cover different cases for updateDesig…

14f1ded

…nParams

fix unit test code style

8144d6e

alessandroniciforo added 3 commits August 3, 2017 18:22

magento#10402 ensure if the theme is loaded by id that it belongs to …

4c0b4a1

…the requested area

magento#10402 reduce npath complexity

7e966b2

magento#10402 refactoring to reduce method complexity and to increase…

e601d4f

… readability

magento#10402 change implementation to detect automatically whether t…

7c6388a

…he theme param returned by getConfigurationDesignTheme is an id or a path

orlangur added the Progress: accept label Aug 4, 2017

orlangur requested changes Aug 4, 2017

View reviewed changes

magento#10402 cleanup

070caf2

orlangur added Progress: needs update and removed Progress: accept labels Aug 9, 2017

magento-engcom-team modified the milestones: August 2017, September 2017 Sep 18, 2017

okorshenko modified the milestones: September 2017, October 2017 Oct 5, 2017

magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:33

okorshenko closed this Oct 31, 2017

diamondavocado mentioned this pull request Jun 4, 2019

Admin order email: Custom theme template modifications not emulated #8884

Closed

orlangur reopened this Jun 4, 2019

ghost unassigned orlangur Jun 4, 2019

orlangur self-assigned this Jun 5, 2019

sidolov closed this Aug 16, 2019

#10402 fix theme templates overrides in environment emulation cross-area #10403

#10402 fix theme templates overrides in environment emulation cross-area #10403

Uh oh!

Conversation

alessandroniciforo commented Aug 2, 2017 • edited by orlangur Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Fixed Issues (if relevant)

Uh oh!

orlangur commented Aug 2, 2017

Uh oh!

orlangur commented Aug 2, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

alessandroniciforo commented Aug 3, 2017

Uh oh!

orlangur commented Aug 3, 2017

Uh oh!

alessandroniciforo commented Aug 3, 2017

Uh oh!

alessandroniciforo commented Aug 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

orlangur commented Aug 4, 2017

Uh oh!

orlangur commented Aug 9, 2017

Uh oh!

ishakhsuvarov commented Aug 30, 2017

Uh oh!

orlangur commented Aug 30, 2017

Uh oh!

okorshenko commented Oct 31, 2017

Uh oh!

diamondavocado commented Jun 4, 2019

Uh oh!

orlangur commented Jun 4, 2019

Uh oh!

m2-assistant bot commented Jun 4, 2019

Uh oh!

magento-cicd2 commented Jun 4, 2019

Uh oh!

alessandroniciforo commented Jun 11, 2019

Uh oh!

sidolov commented Aug 16, 2019

Uh oh!

m2-assistant bot commented Aug 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

alessandroniciforo commented Aug 2, 2017 •

edited by orlangur

Loading

alessandroniciforo commented Aug 4, 2017 •

edited

Loading