Skip to content
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

[EZP-30590] Fix noLayout default value #2656

Closed

Conversation

lolautruche
Copy link
Contributor

@lolautruche lolautruche commented May 23, 2019

Question Answer
JIRA issue EZP-30590
Bug/Improvement yes
New feature no
Target version 6.7 / 6.13 / 7.5
BC breaks no
Tests pass
Doc needed

Prior to v6.0, ViewController::viewLocation() and ViewController::viewContent() had parameter $layout default value to false. The consequence was that noLayout parameter, injected further
down in templates, had true as default value
(the exact opposite).

The goal of such parameter was to ensure that templates used in subrequests always had the information not to use the main pagelayout by default, especially when rendering content from legacy (templates not migrated yet to Twig).

We could also do stuff like the following:

{% extends noLayout == true ? viewbaseLayout : pagelayout %}

Problem with the value injected by NoLayout injector is that the default value of noLayout parameter changed. It currently checks if layout parameter is present (which is not the case by default with the new view system) and if not, defaults to false.
Consequence is that when migrating an instance from eZ Publish 5 to Platform with legacy bridge, main layout is always used. This sounds like an undocumented BC break to me 😉.

Before opening a Jira issue, I wanted to know is that could be an acceptable fix. I don't see any possible regression here since even using Symfony stack, noLayout variable will never have the expected value anyway without this fix.

TODO:

  • Open Jira issue
  • Tests

@lolautruche
Copy link
Contributor Author

Ping @bdunogier @andrerom

@andrerom
Copy link
Contributor

Did you find git blame for this?

I remember there where a change that was discussed around this with @emodric, but that might have been in legacy bridge.

Anyway, maybe a bit late to change it now for 6.7, 6.13 and 7.5? As in I assume this is bc break for everyone on platform already.

@lolautruche
Copy link
Contributor Author

lolautruche commented May 24, 2019

I did git blame a lot, without finding anything relevant.
@emodric : if you have more inputs, please shout out 😄 .

Anyway, maybe a bit late to change it now for 6.7, 6.13 and 7.5? As in I assume this is bc break for everyone on platform already.

AFAICS it won't be a BC break since the default value for layout parameter (on which depends noLayout is always false (hence noLayout should always be true).
Anyway, what I could do is to make this default value configurable, with a dynamic setting.

This is really blocking (even if it doesn't happen so often), so we must be able to solve this IMHO.

@andrerom
Copy link
Contributor

andrerom commented May 24, 2019

Anyway, what I could do is to make this default value configurable, with a dynamic setting.

ehh, rather not 😅

This is really blocking (even if it doesn't happen so often), so we must be able to solve this IMHO.

sure, but someone else will need to review this and input from @emodric would help. In the meantime; JIRA issue, tests?, and call for reviews from someone

@lolautruche
Copy link
Contributor Author

lolautruche commented May 24, 2019

ehh, rather not 😅

Why not?

I'll open the Jira issue for sure. As for tests, I looked for existing tests and there was none, but I can sure create some.

Ping @bdunogier @StephaneDiot @lserwatka

@lolautruche lolautruche changed the title Fix noLayout default value [EZP-30590] Fix noLayout default value May 24, 2019
@adamwojs
Copy link
Member

Closing PR as obsolete. eZ Platform 2.5 has reached EOM, so please reopen PR in ezsystems/ezplatform-kernel (bug fixes) or ibexa/core (features/improvements) if issue is still valid for v3.3 / v4.x and you are willing to work on it.

@adamwojs adamwojs closed this Apr 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
3 participants