bug-fix #16074 - Fix theme change in middle of product page.

[drew7721]

Description (*)

When setting a design exception by user agent for front end, the exception works on all pages except the product page. This was caused by one of the blocks (product_viewed_counter) making a call that would reset the current theme design to the default theme when collecting product data.
See details on how to reproduce in issue #16074 .

The fix does not create any issues and all test continue to pass. There is no need to set the global theme to the default one at that point in the execution.

Solution: remove faulty line.

Fixed Issues (if relevant)

1. User agent exception not setting the correct templates for product pages. #16074 User agent exception not setting the correct templates for product pages.
2. Invalid Template with Custom Theme Applied by User Agent #14932 Invalid Template with Custom Theme Applied by User Agent
3. Category design / theme update not inheriting correctly on product pages #7503 Category design / theme update not inheriting correctly on product pages
4. Custom theme doesn't apply XML layout updates when applied to a product #7710 Custom theme doesn't apply XML layout updates when applied to a product
5. Magento v2.1.2 - Choosing a child theme for product page #8333 Use Design to get theme, because theme is changed after constr…

Manual testing scenarios (*)

1. Set a different theme for iPhone user agent and the changes worked for the whole product page, including the footer.
2. Ran all tests. Including the one for the class containing the changes. Passed.

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 on Travis CI are green)

[magento-engcom-team]

Hi @drew7721. 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-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

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

[drew7721]

@magento-engcom-team give me test instance

[magento-engcom-team]

Hi @drew7721. Thank you for your request. I'm working on Magento instance for you

[magento-engcom-team]

Hi @drew7721, here is your new Magento instance.
Admin access: http://34.228.235.121/pr-19124//admin
Login: admin Password: 123123q

[slavvka]

Could you consider covering the changed functionality with an automated test (preferably integration)?

[drew7721]

Could you consider covering the changed functionality with an automated test (preferably integration)?

There is a Unit test written for that class.. They both pass. Magento\Catalog\Test\Unit\Ui\DataProvider\Product\Listing\Collector\ImageTest

[drew7721]

I guess when @sereban wrote that, he either did not think of the impact changing the theme would have on the rest of the blocks or he just assumed it will only be used trough async calls once the page is loaded. Either way, maybe he can enlighten us on his decision and why setting the theme to the default one is needed at that point, though it seems he has not been that active lately.

Regardless, it's obvious it has an undesired impact on the following blocks that will be rendered on the product page. IMO the image creation should be emulated in the current theme and not the default one and that line should simply be removed, but as per your suggestion @slavvka I think it satisfies all functionality.

[orlangur]

@drew7721,

This was caused by one of the blocks (product_viewed_counter) making a call that would reset the current theme design to the default theme when collecting product data

Why the fix is not about fixing such block then but rather set theme in another class?

Ran all tests. Including the one for the class containing the changes. Passed.

This is bad actually. It means that there is no test currently covering broken scenario.

[drew7721]

@orlangur ,

This was caused by one of the blocks (product_viewed_counter) making a call that would reset the current theme design to the default theme when collecting product data

Why the fix is not about fixing such block then but rather set theme in another class?

It is true that this block does call the data provider(collector) to gather all the data. I don't think the issue lies in the block, but rather in the provider. Blocks should be able to call data providers without fearing that the global theme will be changed.

IMO fixing this in the block is like adding fresh paint over a rotten wall, it will cover it for now but further down the line the issue might/will happen again. I think that fixing the issue at the core is the best approach and will also avoid future issues.

Ran all tests. Including the one for the class containing the changes. Passed.

This is bad actually. It means that there is no test currently covering broken scenario.

I agree, but that should be a different issue, as this PR focuses on fixing the issue at hand, that the front end is broken. That being the main issue that this PR fixes. In my opinion a new issue/PR should be made for the missing tests.

[drew7721]

Also, on another note, this issue is relevant to 2.2.* as well so I'm thinking that I should have done this PR against the 2.2-develop branch instead. Let me know if that would be better.

[drew7721]

@slavvka, thank you for the updates.

[slavvka]

Also, on another note, this issue is relevant to 2.2.* as well so I'm thinking that I should have done this PR against the 2.2-develop branch instead. Let me know if that would be better.

No, that's okay. We want to have all fixes in 2.3 and then in 2.2

[orlangur]

@drew7721,

Blocks should be able to call data providers without fearing that the global theme will be changed.

So, where global theme is actually changed and why we cannot fix it?

a new issue/PR should be made for the missing tests.

This never works. Automated tests are always to be delivered together with corresponding changes, that's the point. But first we should understand where is the correct place for the fix (probably you and Slava already understand this completely and I'm the only one unaware :)).

[drew7721]

@orlangur 😆 np, let me enlighten you.

Issue resides here. See how it sets the default theme? That's a big No No! because we don't know where or when the call to that collector might be placed. So this might randomly change the theme.

Deleting the line might have done the job since the default theme will be used anyhow, but just to make sure it does not break any other functionality on any other branches out there, I agree with @slavvka that the current approach is best : storing the current theme in a var and setting it back when done.

❓ By the way, I really don't see why a "counter" needs that much information about the product. One might think that the entity_id and a $count++ would be enough 🤣

[magento-engcom-team]

Hi @drew7721. Thank you for your contribution.
We will aim to release these changes as part of 2.3.1.
Please check the release notes for final confirmation.

[Ctucker9233]

@magento-engcom-team I'll create a backport for 2.2