Skip to content

Add explicit sequence dependency on Magento_Theme to Magento_Customer #10965

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

Conversation

CNanninga
Copy link

Since Magento_Customer references containers defined by Magento_Theme in layout XML, it should explicitly declare Magento_Theme as a sequence dependency.

It is possible (though highly difficult to predict) for a particular combination of custom extensions to result in Magento_Customer coming before Magento_Theme in the module sequence order. This can create unintended side effects on the front-end. If Magento_Customer layout is processed before Magento_Theme, the blocks added to the "content" container in Magento_Customer/view/frontend/layout/default.xml will result in the "content" container being processed and scheduled before the "content.top" container and to precede it in the list of children of "main".

Ultimately, this will result in the children of "content" being output before the children of "content.top" on the final page.

…er to prevent potential layout order issues
@orlangur
Copy link
Contributor

Since Magento_Customer references containers defined by Magento_Theme in layout XML, it should explicitly declare Magento_Theme as a sequence dependency.

Please provide a basis for such statement. With such approach any module referring to sidebar.main container must declare Magento_Theme in sequence, for example, the Magento_Catalog module.

You must specify before/after attributes when order or layout elements is essential.

@orlangur orlangur self-assigned this Sep 19, 2017
@orlangur orlangur added this to the September 2017 milestone Sep 19, 2017
@CNanninga
Copy link
Author

CNanninga commented Sep 19, 2017

@orlangur

The subject isn't related to a custom extension trying to modify layout.

It's the layout order of "content.top" and "content", declared in Magento_Theme/view/frontend/layout/default.xml, that is important. "content.top" should come before "content". Because of Magento_Customer's reference to the "content" container, it can disrupt this order simply by being present before Magento_Theme in app/etc/config.php.

Short of providing the code of every custom extension being used in the install where I ran into this, I can't put together an exact reproduction scenario, because this is simply down to the order that modules ultimately end up in due to sequence declarations. In my case, the module that triggered this scenario was doing only something as benign as referencing Magento_Theme in its own sequence. Said module does not even have layout updates related to these blocks at all (nor was even enabled at the time).

If the sequence declaration is not the appropriate solution, then before/after attributes should be added to the layout declarations of "content" and "content.top" to make their order explicit.

@orlangur
Copy link
Contributor

@CNanninga

Yeah, let's identify a problem we are trying to solve first.

<referenceContainer name="main">
            <container name="content.top" label="Main Content Top"/>
            <container name="content" label="Main Content Area"/>
            <container name="content.aside" label="Main Content Aside"/>
            <container name="content.bottom" label="Main Content Bottom"/>
        </referenceContainer>

Are you saying that if Magento_Customer is loaded earlier (we can emulate it by specifying an opposite to this PR sequence in Magento_Theme) then content container will be rendered before content.top as it is referenced earlier?

@CNanninga
Copy link
Author

@orlangur
Correct

@orlangur
Copy link
Contributor

If the sequence declaration is not the appropriate solution, then before/after attributes should be added to the layout declarations of "content" and "content.top" to make their order explicit.

That seems like a Plan B to me, can we change implementation in such a way that order of declarations is preserved, no matter when they are referenced for the first time?

@korostii
Copy link
Contributor

korostii commented Oct 1, 2017

can we change implementation in such a way that order of declarations is preserved, no matter when they are referenced for the first time?

Not universally. You will still have corner cases where the order is reversed in a different module, for example. Thus it'll only depend on the module load order if you don't add before/after attributes anyway and will remain fragile.

@orlangur
Copy link
Contributor

orlangur commented Oct 2, 2017

You will still have corner cases where the order is reversed in a different module, for example

Before/after will be needed if order is essential for layout elements declared in different modules.

What I suggest to fix here is: if we declared ElementTop, ElementContent, ElementBottom in ONE module, there is no way they will change order besides <move directive.

@magento-engcom-team magento-engcom-team changed the base branch from develop to 2.3-develop October 20, 2017 15:33
@okorshenko okorshenko modified the milestones: October 2017, November 2017 Nov 1, 2017
@vkublytskyi
Copy link

@CNanninga Are you still going to work on this PR?

@orlangur
Copy link
Contributor

@vkublytskyi #10965 (comment) needs to be checked first and then decided whether we can fix it in Layout Framework: #10965 (comment)

@CNanninga
Copy link
Author

@vkublytskyi - Though I understand am in agreement with the reason the original PR is not an appropriate solution, I unfortunately don't have the capacity to dive into the more complex solution.

@okorshenko okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018
@okorshenko okorshenko self-assigned this Jan 10, 2018
@okorshenko
Copy link
Contributor

Thank you all for collaboration. Unfortunately, the proposed solution is not acceptable.

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.

5 participants