backdrop-issues icon indicating copy to clipboard operation
backdrop-issues copied to clipboard

Allow a layout to be selected on additional paths.

Open mikemccaffrey opened this issue 9 years ago • 59 comments

It would be great to set more than one path for which a layout is used instead of the default. For example, while most of the pages on a site may have a sidebar, there could be five or so pages that don't have a sidebar, and it'd be great to define a single layout without a sidebar and specify the five paths for which it should be used. Am I correct in assuming that this isn't something that cannot currently be done without creating five different layouts with five different paths?

Perhaps we could just allow the path field to be left as empty, or filled with a single % wildcard, and then just use the visibility conditions options to determine what paths it should be used instead of the default layout?

mikemccaffrey avatar Jan 15 '16 21:01 mikemccaffrey

or filled with a single % wildcard

Perhaps a special token like <multiple>, similar to <front>? I like the simplicity of % though. In all I think this is a good idea. I've added it to the META as a "long term" solution because I think/thought changing Layouts to accept multiple paths as default would require significant changes, unless if a shortcut version like %+conditions is acceptable and doable.

docwilmot avatar Feb 15 '16 16:02 docwilmot

...@jenlampton's issue title on the other, duplicate seemed more appropriate :wink:

klonos avatar Mar 01 '16 02:03 klonos

...@jenlampton's issue title on the other, duplicate seemed more appropriate :wink:

Don't think so actually, two different suggestions. Jen's suggestion would be an API change I think. Layout json files would need the path key converted to an array, and there'd be changes to how layouts are routed AFAICT.

The OP could be accomplished by just allowing blank paths or paths with a % in them, which wouldnt affect much.

docwilmot avatar Mar 02 '16 19:03 docwilmot

...my bad then.

Don't think so actually, two different suggestions.

Then perhaps that issue shouldn't have been closed as duplicate of this one?

Jen's suggestion would be an API change I think.

Still a thing that we should do at some point if you ask me. If it is an API change, can we do it in 2.x?

klonos avatar Mar 02 '16 22:03 klonos

In https://github.com/backdrop/backdrop-issues/issues/3062, we've run into an issue that requires layouts have multiple paths. In this case, we want node/% and node/%/preview to always use the same layout. It's related to this request for sure, but perhaps slightly different in that this combination of paths would probably not be optional or user-specified. Simply if node/% were the path, node/%/preview would automatically be included in the path matching as well.

@docwilmot suggested a hook:

I propose a new hook_layout_get_layout_by_path_alter(&$layouts, $router_item, $object), invoked in layout module's layout_get_layout_by_path() function, that allows other modules to modify the layouts that are loaded for a given path before the chosen layout is resolved.

Which I think would work okay for this use-case, but I'm nervous about introducing a hook into the primary pathway of the page rendering. It's an opportunity to completely tank the site performance, because finding a matching layout at a given path is tricky to optimize. And because it happens every single page load, inefficient implementation would affect the entire site.

The OP could be accomplished by just allowing blank paths or paths with a % in them, which wouldn't affect much.

Wildcards like this might also be an efficiency issue. In layout_get_path_layout_names(), we'd probably run backdrop_match_path() on every layout's path (or only those that contain *). In order to be compatible with existing wildcard matching, we'd probably end up using * as the path wildcard (as opposed to %, which indicates a context).

As an alternative, instead of altering the path on load, we could add additional paths (either automatically or manually) on layout save instead. Right now, layouts only have $layout->path (a single value). I think we could expand this out to include something like $layout->alternative_paths (an array).

It makes some sense that $layout->path still be a single value, because there needs to be something that roots the available contexts for the overall layout configuration. For example you could have node/% as the primary path and node/%/preview as an alternative; both would have a single Node as their context in position 1. But you could not have node/% as the primary path and taxonomy/term/% as an alternative, because the contexts would be different.

quicksketch avatar Sep 01 '18 20:09 quicksketch

I'm not sure if it is helpful or not, but @Gormartsen made a module that I'm using on a site which allows a layout to be used on multiple "Alternative paths" besides the main contextual path:

https://github.com/backdrop-contrib/layout_wildcard

laryn avatar Sep 12 '18 03:09 laryn

I like the suggestion about additional paths:

As an alternative, instead of altering the path on load, we could add additional paths (either automatically or manually) on layout save instead. Right now, layouts only have $layout->path (a single value). I think we could expand this out to include something like $layout->alternative_paths (an array).

I think calling it $layout->additional_paths is better. There would need to be a way to restrict the additional paths so they can only be in the same context set by the placeholder of the primary path. Maybe the additional paths need to be sub-paths if the primary path includes a context. So with node/% as primary the additional needs to be [node/%] plus /preview.

herbdool avatar May 24 '24 15:05 herbdool

Note that Layout Wildcard module (which has 39 installs) uses $layout->settings['alternative_paths'] for its alternative paths. If it is proposed to replicate some of its functionality in core (which this seems to be), the core changes and contrib module should play nicely together.

bugfolder avatar May 24 '24 16:05 bugfolder

What @bugfolder suggests :+1:

Currently it's a bit tricky to use multiple paths - the "OR" is a missing feature in Layout path handling concept. :wink:

A practical example (as recently discussed in other issues) for login/password/register pages:

  • Create a custom layout for path "user" (no placeholder)
  • Use layout_wildcard to add the three (sub-)paths
  • Alternatively create a custom (minimal) module that implements hook_layout_load_by_router_item_alter()

This works fine, but it can't be done with core UI alone and I do belief, that using the same layout for multiple paths is quite a common use-case.

indigoxela avatar May 25 '24 08:05 indigoxela

What @bugfolder suggests 👍

What everyone here said 👍🏼 😅 ...I believe that 'alternative_paths' (as provided by the contrib layout_wildcard for now), and the 'additional_paths' suggested here to be provided by core would greatly expand the flexibility of how layouts/contexts work, and most importantly how people expect them to work.

klonos avatar Jun 02 '24 08:06 klonos

So we have something to start with, I've ported over the basic alternative_path / additional_path functionality from layout_wildcard and nothing else. https://github.com/backdrop/backdrop/pull/4879

I made a change to allow additional_path to have more placeholders than the primary one. The first placeholder is the one that provides context so as long as the additional paths have the minimum of 1 placeholder it should work. In my understanding this is what is needed to allow node/%/revisions/%/revert to work when using node/%. If I've got that correct, will need to test more. And even if it's necessary.

herbdool avatar Oct 03 '24 14:10 herbdool

With the PR I got this to work. STR:

  1. Create View - User Content that has a path of user/%/content and a contextual filter of Author UID that gets the uid from the URL.
  2. Create Layout - User with a primary path of user/% and an additional path of user/%/content.
  3. Go to /user/1/content and check that it's using the new layout and not default.
  4. Compare with removing user/%/content from the additional path. This should use the default layout.

herbdool avatar Oct 03 '24 14:10 herbdool

Thanks for the PR, @herbdool! I've tested the use-case for the login, password, and register pages mentioned by @indigoxela in https://github.com/backdrop/backdrop-issues/issues/1528#issuecomment-2131112323. It works with the following configuration:

Path: user/login

Additional paths: user/password user/register

olafgrabienski avatar Oct 03 '24 15:10 olafgrabienski

I did some quick testing and I like it a lot!

My testing setup:

  1. Set node_admin_theme to false, so the frontend theme is used for editing
  2. Create some page nodes
  3. Create a layout that looks quite different to the default node layout
  4. Layout's path node/%
  5. Layout condition node nid = X
  6. Additional path node/%/edit :point_left:

It works. :+1: This specific node gets the special layout in display and when editing it. Not the others.

Slight modification:

  • switch Layout condition to node type = page
  • Additional path still set to node/%/edit (unchanged)

Works, too. :+1: The "page" node types get the special layout for view and edit, none of the other types.

indigoxela avatar Oct 04 '24 11:10 indigoxela

What didn't work - because that's not how it works, anyway ... and maybe shouldn't work that way (?): I couldn't add an arbitrary path to a Layout with a path containing a placeholder.

Example: create a layout for node type "post". But then try to add a path to a view as additional path (which has no placeholder).

And with a weird placeholder addition I could break the site. :smiling_imp:

So, my-special-posts-view is forbidden to save, but my-special-posts-view% is a WSOD. Maybe unrelated, but ... a bit ugly.

indigoxela avatar Oct 04 '24 11:10 indigoxela

@indigoxela I've added validation for improper placeholders like my-special-posts-view% which isn't allowed in the primary path either.

herbdool avatar Oct 04 '24 14:10 herbdool

I've added validation for improper placeholders like my-special-posts-view%

Wow, that was quick! :+1: More testing soon.

indigoxela avatar Oct 04 '24 15:10 indigoxela

I belief, some feedback by @bugfolder would be crucial here. Ideally, we'd manage to provide a smooth transition from "layout_wildcard provides everything" to "layout_wildcard provides the whistles and bells". I'm not familiar enough with layout_wildcard to reliably evaluate compatibility.

indigoxela avatar Oct 05 '24 09:10 indigoxela

I [believe that] some feedback by @bugfolder would be crucial here.

I'll try to review this to see the implementation and comment on compatibility, but i'm on the road for two weeks, so it may take a little while to get the right time to do this.

bugfolder avatar Oct 05 '24 09:10 bugfolder

I'll advocate for this.

herbdool avatar Oct 11 '24 17:10 herbdool

This now has an automated test. Ready for fuller testing.

herbdool avatar Oct 11 '24 18:10 herbdool

@herbdool I've left some questions on your PR. And phpcs "requests" some parameter comments and a comma. :wink:

indigoxela avatar Oct 12 '24 12:10 indigoxela

If the primary path and the additional paths are both context paths, I wonder if the fact that the additional paths do not load contexts would be a problem or confusion for users. For example if the primary path was user/% and one of the additional paths was node/%. I imagine that you may expect that node field blocks should work on that layout. Could we put the additional paths fieldset above the context table, and check context on those paths, and also attempt to load contexts for the additional paths?

docwilmot avatar Oct 17 '24 03:10 docwilmot

Good question. I was wondering if instead that it enforces the use of only the context of the primary path, and that any additional paths should have that context. In which case it would have to display a warning. I'd need to look at the originating module to see how it handled contexts. It does check positions and adjusts contexts when loading layouts, but I'm not sure if its allowing different contexts at that stage or not.

herbdool avatar Oct 17 '24 15:10 herbdool

In Layout Wildcard, "Alternative paths and Ancestor matching only consider for overriding layouts whose primary path has the same number of placeholders as the router path of the requested URL (so that contexts can be unambiguously set correctly)."

Dealing with the possibilities of contexts and placeholders was a big factor in throttling further progress in expanding LW to things like aliases and other placeholders (e.g., what if there are two placeholders, but their order is reversed in an alternative path?).

bugfolder avatar Oct 17 '24 16:10 bugfolder

Maybe I should change this PR to go back to just having the same number of placeholders to limit the scope and "so that contexts can be unambiguously set correctly".

herbdool avatar Oct 17 '24 17:10 herbdool

I dont quite understand the same number of placeholders concept; what do we mean by that?

docwilmot avatar Oct 17 '24 17:10 docwilmot

@docwilmot Layout Wildcard currently requires the same number of placeholders for the primary and additional paths. That means having node/%/revisions/%/revert as an additional path isn't allowed when using node/%for the primary, butnode/%/edit` is allowed.

In this PR I was attempting to also allow additional paths to have more placeholders than the primary path. So that node/%/revisions/%/revert would also work when using node/% for the primary. Then perhaps don't need to check where there are "two placeholders, but their order is reversed" as @bugfolder mentioned.

herbdool avatar Oct 17 '24 21:10 herbdool

I'm happy to see progress on this feature. I recently tried to created a layout for 403 and 404 pages, but realized that I would need one layout for each. With this functionality, I believe I could create one layout for both and place specific blocks for each message.

I found a problem with the PR, unless I'm misunderstanding something.

I tried to add the "about" page to the "home" layout and it does not seem to be working. I think this should work, shouldn't it?

image

stpaultim avatar Oct 18 '24 04:10 stpaultim

I tried to add the "about" page to the "home" layout...

@stpaultim I'm assuming, you mean the default page node with the path alias "about". Its path would be "node/%" with a condition on the node's NID. Layouts work with system paths, not with path aliases.

@herbdool Re multiple placeholders, more than one in the additional paths section - I scratched my head, but it seems a complicated concept, prone to errors and misunderstandings.

Ideally, this UI would work for admins not familiar with hook_menu. Not sure, if that's possible.

indigoxela avatar Oct 18 '24 05:10 indigoxela