Skip to content

Conversation

@Hailong
Copy link
Member

@Hailong Hailong commented Mar 29, 2019

Description (*)

Layout change of the Magento_Catalog::catalog_product_view.xml breaks legacy templates in which uses the $block->getProduct() function calls. Problem is the new introduced block class doesn't support this.

To avoid incompatibility problem. it's better to have the new class extends from the legacy class.

Fixed Issues (if relevant)

  1. $block->getProduct() in catalog product view template failed after upgrading from 2.3.0 to 2.3.1 #22036: $block->getProduct() in catalog product view template failed after upgrading from 2.3.0 to 2.3.1

Manual testing scenarios (*)

  1. Install Magento 2.3.0.
  2. Install Porto Theme
  3. Product page is broken.

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)

@rogyar
Copy link
Contributor

rogyar commented Mar 29, 2019

Agree. The existing templates that invoke getProduct() of the block might be broken.
Thank you!

@ghost ghost assigned rogyar Mar 29, 2019
@magento-engcom-team
Copy link
Contributor

Hi @rogyar, thank you for the review.
ENGCOM-4605 has been created to process this Pull Request

@magento-engcom-team
Copy link
Contributor

@Hailong thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

@soleksii
Copy link

soleksii commented Apr 5, 2019

✔️ QA Passed

Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @Hailong @rogyar I have a concern about the backward incompatibility of this backward compatibility fix.

The backward incompatible difference between 2.3.1 and 2.3.2 version of the class is going to be 1 introduced a public method and more importantly 2 required constructor parameters.

@rogyar
Copy link
Contributor

rogyar commented Apr 11, 2019

@sivaschenko. Agree. We need to choose between two evils here:

  • Fix backward incompatible changes introduced in 2.3.1 (fix is fair in case of 2.3.0->2.3.1 upgrade)
  • Leave backward incompatible changes introduced in 2.3.1 and don't introduce new potentially backward incompatible changes in 2.3.2.

After some consideration, I prefer the second option.

@sivaschenko
Copy link
Member

Closing the pull request considering the discussion above. Feel free to contact me in case of any questions or comments

@m2-assistant
Copy link

m2-assistant bot commented Apr 13, 2019

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

@Hailong
Copy link
Member Author

Hailong commented Apr 14, 2019

Agree! Just for information of any others who are using Porto theme with Magento 2.3.1, Porto theme has published a newer version which has a fix for this problem.

@Seb33300
Copy link

@Hailong Where did you find the fixed version of Porto? I am encountering the same issue with the latest version 3.1.8 downloaded on themeforest

@Hailong
Copy link
Member Author

Hailong commented Apr 30, 2019

@Seb33300 My theme was also 3.1.8 downloaded from ThemeForest. I just downloaded it again and checked it for you. The fixed file is in zip named Patch for Magento 2.3.1 And Above.zip, and its name is details.phtml

@Hailong
Copy link
Member Author

Hailong commented Apr 30, 2019

@Seb33300 BTW, don't forget to unzip all the 'Patch...' zip files, and merge them into the theme. I don't like this either, but this is how Porto releases their new versions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants