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

[BUGFIX] Support mixing of fluidpages with other backend layouts #367

Open
wants to merge 2 commits into
base: development
Choose a base branch
from

Conversation

cweiske
Copy link

@cweiske cweiske commented Sep 29, 2017

When mixing both fluidpages and templavoila page layouts,
the page's backend_layout setting needs to be checked to
make sure that fluidpages is responsible for handling the
rendering.

Resolves: #366

@NamelessCoder
Copy link
Member

Hi Christian,

I really appreciate the work you put into this, especially that you added unit tests, and I understand the problem and solution - but I think this is the appropriate time to solve this as well as solve a history of confusion regarding the "backend layout" selection and using fluidpages with XYZ other backend layout.

Here's what I suggest:

  • We move the checking "should we use Fluidpages for this record?" to a separate class/function
  • We then use this as userFunc in displayCond for both of the fluidpages template selection and configuration fields (and possibly cache the decision because all fields cause the same result)
  • We call this user function in the Provider(s) - possibly by overriding trigger() and doing an additional check after the parent's checks (which don't use DB) have run.

Together these parts could be quite flexible - if someone wants to add additional displayConds, that's possible, and if we use the displayCond evauation as wrapper when we determine if fluidpages should handle a page, we make the Provider respect the TCA displayCond also if a user changes it. It's very close to the solution you already wrote but I think the displayCond usage is the right approach. Then literally everything that would cause a template to be read, will hinge on whether or not you selected "Fluidpages" as backend layout for this page or parent of page.

This should as far as I can predict also work to, for example, change to Fluidpages backend layout on just a single page somewhere in the rootline. Which would be very useful indeed. Note to self mostly: this may mean we need to disable the "parent decides" field in more cases, e.g. when page is the first to switch to Fluidpages as backend layout instead of checking if page is root page, as it is now.

Perhaps we could even combine this with preventing the automatic TS loading on pages where Fluidpages BE layout isn't selected - or leave it as is and refer to the "disable automatic TS inclusion" option you get in fluidpages' EM configuration.

What are your thoughts about the above as an alternative? Perhaps I am missing some detail that means this filtering is better done in the PageService?

@cweiske
Copy link
Author

cweiske commented Oct 12, 2017

I moved the code to UserFunction/LayoutSelect.php and adjusted the tests.
Overrides/pages.php is also modified now, and the fluid layout selector is now only visible if fluidpages is selected as backend layout.

I did not modify anything trigger-related. My patching budget is nearly up :)

When mixing both fluidpages and templavoila page layouts,
the page's backend_layout setting needs to be checked to
make sure that fluidpages is responsible for handling the
rendering.

Resolves: FluidTYPO3#366
@NamelessCoder
Copy link
Member

Looking great so far - I've amended the commit (just concerning code/doc style) but overall this works when tested, but obviously you need to configure a different rendering for pages where you don't use the "Fluidpages" layout, because page TS (if included automatically) will attempt to render a template which then doesn't resolve correctly and causes a Fluid template file not resolved error. It might be possible to avoid this by changing the Provider, but I guess you don't use this setup intentionally unless you also configured page TS in a way that doesn't call fluidpages ;)

Perhaps a specific error about this is appropriate, but I won't demand one for this pull request.

BE implementation works exactly like discussed - perfect!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants