Visibility condition options incomplete for layouts with non-string contexts

[bugfolder]

On the "Configure Layout" page, when creating a layout with a path with a placeholder and a non-string context (like Node) for the placeholder, the visibility conditions options don't include all possible options.

To reproduce:

1. Go to admin/structure/layouts.
2. Select "Add layout".
3. Enter a layout name and select a template.
4. For the Path, enter "foobaz/%".
5. Under Contexts, select Type = Node.
6. Click "Add visibility condition".
7. Click the "Select" dropdown to see the available options.

Expected result: all possible conditions, including "Node: NID" and "Node: Type".

Actual result: a shorter list, shown here:

Now, we can get the longer list if we save the layout and then go back and configure it.

1. Cancel out of the "Add visibility condition" dialog.
2. Click "Create Layout" button.
3. Click "Configure Layout" tab.
4. Click "Add visibility condition".
5. Click the "Select" dropdown to see the available options.

This time, we see all possible conditions, as here:

In stepping through the code, I find that a place where things go awry is in layout.admin.inc, function layout_condition_add_form(), line 1817, which is:

$layout_has_context = $layout && $layout->hasContexts($required_contexts);

At this point, $required_contexts contains a node context, but the relevant context in $layout is a LayoutStringContext, rather than an EntityLayoutContext (which is what it should be if Type = Node). So I infer that the ajax called when we change the context Type is not updating the tempstore Layout as it should.

We can encounter the same issue by creating and saving the layout with a String pass-through context, then going back and editing the layout to change the context to a Node type.

So, the workaround is simple: just save the layout, then go back and re-edit it to get the full list of possible visibility conditions. But it would be nice to have them show up without having to save-and-re-edit.

This seems related to https://github.com/backdrop/backdrop-issues/issues/5065. Attempting to save the layout with that added visibility condition results in the problems of https://github.com/backdrop/backdrop-issues/issues/5065.

Doing testing on 1.x.

[jayelless]

The problem is that when the link to add a new visibility condition is ticked, the newly selected context is not part of the layout settings (yet). However, the new context value is available in the returned user input data when the ajax popup form is being built. The solution then is to check whether any required context for each visibility condition is either already available in the layout, or will be added as a new context when the layout is saved.

[jayelless]

Issue https://github.com/backdrop/backdrop-issues/issues/5145 is a duplicate of this issue, and has now been closed.

[stpaultim]

@jayelless - Is this Pull Request ready for testing and review? If so, you should go ahead and add those two labels as well.

[klonos]

There's still 100s of test failures, so I'm assuming not ready for testing/review yet.

[jayelless]

I see some test failures which I am currently investigating and fixing :(

[jayelless]

The problem I faced was that I had used the PHP "array_column" function, which was not introduced until PHP5.5, so the 5.3 tersts were failing. Now replaced with equivalent code.

[bugfolder]

LGTM.

[stpaultim]

I confirmed the problem on an existing site, then logged into the PR sandbox and tested with the listed steps to reproduce. With the PR I was able to see the longer list of 8 possible visibility conditions from the start.

I also am adding this to the 1.19.4 milestone

[quicksketch]

I left a review where I found a few issues: https://github.com/backdrop/backdrop/pull/3696#pullrequestreview-775772283

Overall though, I'm not totally sure this is the right approach. If arbitrary paths can be specified that create contexts from thin air, I'm not sure how we can enforce access control. For example if you made a layout at foobaz/%, the access permissions on that path are only going to be controlled by the Layout UI. It would effectively bypass all node (or other entity) access control, allowing users to view content regardless of system-wide permissions.

A safer route might be to use hook_layout_context_info() to add additional paths to existing contexts, but you would also need to implement hook_menu() to create the menu entries that set proper access control on that new path. I'm not sure this approach is feasible either because we don't yet have a hook_layout_context_info_alter() which I would think would be necessary.

[jayelless]

@quicksketch I note the concerns you have expressed about this method being able to bypass existing access control, so will need to consider that when looking at a update to the original patch prepared.

[jayelless]

@quicksketch Can you please expand on your suggestion that allowing access to entities via a non-standard path (eg nodes via path "/foo/{nid}" would allow access controls to be bypassed. I am unable to find how this might work, as access does not appear to be tied to the path used to access the entity.

[klonos]

@quicksketch can I please get your attention to this issue here? There are also some comments in the PR that could use your input. Thanks.