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

Fix relative template references in individual Magento modules #1895

Merged
merged 6 commits into from Mar 24, 2017

Conversation

@davidalger
Member

davidalger commented Sep 17, 2015

Relative template references work up until 3rd party code attempts to use a DI to change a block type via a preference, thus changing the base for the relative template reference from the original module to their own.

This replacement done globally via following regex on any *.xml files in app/code/Magento:

match: <block class="Magento\\([^\\"]+)\\(.*)template="([^:"]+)"
replace: <block class="Magento\\$1\\$2template="Magento_$1::$3"

David Alger
Fix relative template references in individual Magento modules
Relative template references work up until 3rd party code attempts to use a DI to change a block type via a preference, thus changing the base for the relative template reference from the original module to their own.

This replacement done globally via following regex on any *.xml files in `app/code/Magento`:

match: `<block class="Magento\\([^\\"]+)\\(.*)template="([^:"]+)"`
replace: `<block class="Magento\\$1\\$2template="Magento_$1::$3"`
@davidalger

This comment has been minimized.

Member

davidalger commented Sep 17, 2015

Question: should there be a static code test to enforce these are absolute template references in core layout code?

@mttjohnson

This comment has been minimized.

mttjohnson commented Sep 30, 2015

We keep running into issues with these relative template paths when building a new (3rd party) module to extend some functionality of a block class. In several instances the templates all of the sudden stop showing up because they don't exist in the new module's directory. It seems like some template definitions are set that includes the module name, and other are not.

Our current workaround is to define a layout update in the new module to point every template that uses the block class back to the original module by including the module in the template path.

It seems like having the core modules explicitly state the module in the template path would be a more reasonable thing to standardize on. Either way it is frustrating running into the differences where some templates have the module name and others do not.

@magento-cicd2

This comment has been minimized.

Contributor

magento-cicd2 commented Oct 1, 2015

We have automated a Magento Contributor License Agreement verifier for contributions sent to our GitHub projects.
Please see the CLA agreement in the Pull Request comments below.

David Alger added some commits Oct 20, 2015

David Alger
Merge remote-tracking branch 'upstream/develop' into feature/fix-rela…
…tive-template-paths

# Conflicts:
#	app/code/Magento/Catalog/view/adminhtml/layout/catalog_product_set_edit.xml
#	app/code/Magento/Customer/view/frontend/layout/default.xml
#	app/code/Magento/GroupedProduct/view/adminhtml/layout/catalog_product_grouped.xml
#	app/code/Magento/Sales/view/adminhtml/layout/sales_order_creditmemo_new.xml

@vkublytskyi vkublytskyi added CS and removed PS labels Apr 19, 2016

@vkublytskyi

This comment has been minimized.

Contributor

vkublytskyi commented Apr 19, 2016

Relative templates implementation needs more internal discussion

@davidalger

This comment has been minimized.

Member

davidalger commented Apr 19, 2016

@vkublytskyi Is there a sticky point with this which you could shed some light on? Not having had this right out of the gate is causing major holdups in theme customizations, and it's been > 6 months since I initially submitted the PR. I'd love to see it included, and am willing to help and/or dialog to get us all to the same end goal: making things consistent and usable for developers.

@vkorotun

This comment has been minimized.

Contributor

vkorotun commented May 20, 2016

Let me describe an alternative solution. Please reply with your thoughts.

  • If we resolve template path during layout compilation (based on XML declaration and not after block was instantiated), then doesn't matter if block class was replaced with layout update or DI preference, as well as doesn't matter if prefix was set or skipped
  • Template path could changed only when developer explicitly says that in "referenceBlock" directive:
    • <referenceBlock name="a.block" class="Some\Extension\Block" template="Some_Extension::template.phtml"/>. Changing just block class should not lead to changed path to the template.
  • XSD validation of "referenceBlock" template attribute should allow only path with prefix, so developer will be REQUIRED to explicitly specify either he needs to go with original template or some other template:
    • Validation failed: <referenceBlock name="a.block" template="template.phtml"/>
    • Validation passed: <referenceBlock name="a.block" template="Module_One::template.phtml"/>

@vasiliyseleznev vasiliyseleznev added Area: Frontend and removed CS labels Aug 3, 2016

@vkorotun

This comment has been minimized.

Contributor

vkorotun commented Aug 4, 2016

@davidalger This will be taken soon into progress, would be great if you provide your feedback on alternative proposal described above.

@vkorotun vkorotun removed the TECH label Aug 4, 2016

@mttjohnson

This comment has been minimized.

mttjohnson commented Aug 4, 2016

@vkorotun - Let me know if this is correct. If you resolve the template path earlier, any relative template path would always end up relating to the module where the block was originally defined in a layout file, and relative template paths would just be relegated to being a short hand way of defining the layout path, rather than having them always be fully qualified with the prefix of the module name, it in effect becomes the same thing as if it was fully stated.

If the relative paths are only ever used when originally defining a block in the layout, and it always resolves to the path of the module it was defined in, what would be the purpose of ever using a relative path at that point? Is the intention of relative template paths just supposed to be a shorthand?

Consistency in the core is important, and that was one thing I noticed initially is that it seemed to be inconsistent where relative template paths were used, and where prefixed template paths were used, and that made it a lot more difficult to know what to expect when trying to extend functionality in different ways.

If this change in functionality occurs, then would you state relative paths in all the block declaration layout files that are referencing their own module's template path, or would you put in the module prefix on all of them?

@vkorotun

This comment has been minimized.

Contributor

vkorotun commented Nov 3, 2016

Internal ticket: MAGETWO-60451

@vrann vrann added this to the March 2017 milestone Mar 22, 2017

@vrann vrann self-assigned this Mar 22, 2017

@vrann vrann added this to Response Needed in Community Pull Requests Mar 22, 2017

@vrann

This comment has been minimized.

Contributor

vrann commented Mar 23, 2017

@davidalger I'm ready to go ahead and accept this PR. Will you be willing to help with that? If yes, what I need from you:

  • merge the PR with the latest mainline
  • update any new occurrences of the template references in CE which need to be fixed
  • do you update them by hand or do you have some kind of script which do that? If you have script, please share it, so that I can do the same for EE and B2B
  • we discussed this internally and decided to have the static test which will ensure that there will be no new cases of wrong references delivered to Magento. If you can, please write such test, otherwise, I will do it.

P.S.: I read again the initial comment and my question regarding script is answered.

@davidalger

This comment has been minimized.

Member

davidalger commented Mar 23, 2017

@vrann Awesome! thank you! :)

I've updated the branch with latest from develop, and then used the following one-liner to update new references which came in on the merge:

find . -type f -name \*.xml | xargs -n1 perl -i -pe 's#<block class="Magento\\([^\\"]+)\\(.*)template="([^:"]+)"#<block class="Magento\\$1\\$2template="Magento_$1::$3"#g'

After I did that, I used ack to verify my find/perl combo didn't miss any:

ack '<block class="Magento\\([^\\"]+)\\(.*)template="([^:"]+)"'

This turns up a few relative references in the integration tests, but I'm not certain those should change, so I left them alone. That first command should make EE and B2B repos very happy!

I'll see if I can get to the test next week, I'm under the gun on an end-of-month deadline atm and traveling to a friends wedding over the weekend, so I'm not sure yet.

@vrann

This comment has been minimized.

Contributor

vrann commented Mar 23, 2017

@davidalger perfect, now I'm working on EE, B2B and static tests.

@vrann vrann moved this from Response Needed to Review In Progress in Community Pull Requests Mar 23, 2017

@vrann

This comment has been minimized.

Contributor

vrann commented Mar 23, 2017

@davidalger few occurrences in CE was not fixed, not sure yet why they are not caught. One issue is it seems the script does not take into account the template attribute on the new line. Though this one is weird <block name="checkout.cart.totals.msrp" before="checkout.cart.totals" class="Magento\Msrp\Block\Total" template="cart/totals.phtml"> -- may be because class is not immediately after <block?

Targets
    Occurrences of 'template="([^:"]+)"' in Project with mask '*.xml'
Found Occurrences  (8 usages found)
    Usage in string constants  (8 usages found)
        clear-engcom2ce  (8 usages found)
            app/code/Magento/AdminNotification/view/adminhtml/layout  (1 usage found)
                default.xml  (1 usage found)
                    20template="notification/window.phtml"/>
            app/code/Magento/Catalog/view/adminhtml/layout  (1 usage found)
                catalog_product_set_edit.xml  (1 usage found)
                    14template="catalog/product/attribute/set/main.phtml"/>
            app/code/Magento/Customer/view/frontend/layout  (1 usage found)
                default.xml  (1 usage found)
                    23template="account/link/authorization.phtml"/>
            app/code/Magento/Msrp/view/frontend/layout  (1 usage found)
                checkout_cart_index.xml  (1 usage found)
                    12<block name="checkout.cart.totals.msrp" before="checkout.cart.totals" class="Magento\Msrp\Block\Total" template="cart/totals.phtml">
            app/code/Magento/Theme/view/adminhtml/layout  (1 usage found)
                theme_design_config_edit.xml  (1 usage found)
                    15template="design/config/edit/scope.phtml"/>
            app/code/Magento/Wishlist/view/frontend/layout  (2 usages found)
                wishlist_index_configure.xml  (1 usage found)
                    13template="item/configure/addto/wishlist.phtml" />
                wishlist_index_configure_type_bundle.xml  (1 usage found)
                    24template="item/configure/addto/wishlist.phtml" />
            dev/tests/integration/testsuite/Magento/Framework/View/Layout/Reader/_files  (1 usage found)
                _layout_update_block.xml  (1 usage found)
                    10<block class="Dummy\Class" name="test.block" group="test.group" template="test.phtml" ttl="3">
@vrann

This comment has been minimized.

Contributor

vrann commented Mar 23, 2017

@davidalger fixing those and the failing static test

@vrann

This comment has been minimized.

Contributor

vrann commented Mar 24, 2017

@davidalger ok, fixed everything mentioned before.

The new tricky part if virtual types for the blocks. Here is an instance which failing:

    <virtualType name="Magento\CatalogSearch\Block\SearchResult\ListProduct" type="Magento\Catalog\Block\Product\ListProduct">
        <arguments>
            <argument name="catalogLayer" xsi:type="object">Magento\Catalog\Model\Layer\Search</argument>
        </arguments>
    </virtualType>
<block class="Magento\CatalogSearch\Block\SearchResult\ListProduct" name="search_result_list" template="Magento_CatalogSearch::product/list.phtml" cacheable="false">

See how the reference to the template different from the real instance of the block.

@vrann

This comment has been minimized.

Contributor

vrann commented Mar 24, 2017

@davidalger Luckily, there are just 2 such cases of virtual classes. Fixed those and added the static test for any further occurrence.

@magento-team magento-team merged commit 0ffb532 into magento:develop Mar 24, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
licence/cla Contributor License Agreement is signed.
Details

magento-team pushed a commit that referenced this pull request Mar 24, 2017

MAGETWO-66675: Fix relative template references in individual Magento…
… modules #1895

- fix references
- update static test

magento-team pushed a commit that referenced this pull request Mar 24, 2017

magento-team pushed a commit that referenced this pull request Mar 24, 2017

MAGETWO-66675: Fix relative template references in individual Magento…
… modules #1895

- add static test to ensure that no new broken template references appears in the codebase

magento-team pushed a commit that referenced this pull request Mar 24, 2017

magento-team pushed a commit that referenced this pull request Mar 24, 2017

MAGETWO-66675: Fix relative template references in individual Magento…
… modules #1895

- Corrected templates referencing the module of the Virtual type versus the module of the base
- Add static test for the Blocks virtual types

magento-team pushed a commit that referenced this pull request Mar 24, 2017

Merge pull request #955 from magento-engcom/develop-prs-isolated
[EngCom] Fix relative template references in individual Magento modules #1895
@davidalger

This comment has been minimized.

Member

davidalger commented Mar 25, 2017

@vrann w00t!! And thanks for adding the static test too! :)

@davidalger davidalger deleted the davidalger:feature/fix-relative-template-paths branch Mar 25, 2017

@vrann

This comment has been minimized.

Contributor

vrann commented Mar 27, 2017

@davidalger great PR, excited about it too :)

@vrann vrann moved this from Review In Progress to Done in Community Pull Requests Mar 31, 2017

@southerncomputer

This comment has been minimized.

Contributor

southerncomputer commented Jun 3, 2017

While you are here, mind fixing relative paths in require statements?! I had one quote.js bite me recently due to the code using '../model/quote' - had to change them all to full absolute paths, seems to be all over the code base - but it would be nice to have no more relative paths that can cause error when upgrading from 2.1.2 to 2.1.7 quote.min.js missing!

hostep added a commit to hostep/magento2 that referenced this pull request Feb 7, 2018

Backport for Magento 2.1: Merge pull request magento#955 from magento…
…-engcom/develop-prs-isolated

[EngCom] Fix relative template references in individual Magento modules magento#1895

(cherry picked from commit a434162)

hostep added a commit to hostep/magento2 that referenced this pull request Aug 11, 2018

Backport for Magento 2.1: Merge pull request magento#955 from magento…
…-engcom/develop-prs-isolated

[EngCom] Fix relative template references in individual Magento modules magento#1895

(cherry picked from commit a434162)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment