search

backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

Fixed: Flush the layout cache when switching the node_admin_theme option. #6374

Closed indigoxela closed 6 months ago

indigoxela commented 10 months ago

Description of the bug

It's a rather subtle problem, as the default admin theme and the default theme aren't that different. But it's visible. Even more if the layouts do differ significantly.

Steps To Reproduce

  1. Switch the setting "Use the administration theme when editing or creating content" off and save on admin/appearance
  2. Open a node form, like node/add/post
  3. Inspect the current layout via admin bar dropdown "This page"
  4. Switch the setting "Use the administration theme when editing or creating content" back on and save (do not flush caches manually)
  5. Open a node form
  6. Inspect the current layout via admin bar

Actual behavior

When the default theme is selected for node edit/create, the Default Layout should be in use. When the admin theme is selected, the Default Administrative Layout should be used. But switching doesn't change that, so the wrong Layout is used, until one flushes caches.

This leads to visual glitches. I've seen this before, but didn't actually realize the source of the glitch.

wrong-layout

^^ That's a harmless example, it can look a lot weirder, depending on layout adaptions (blocks).

Expected behavior

The right layout after switching.

Additional information

indigoxela commented 10 months ago

A PR is available for testing and review.

I tried to narrow the flush down to specific items, but all the node/add/NODETYPE have their own entry, and that's potentially a lot. Flushing is easier.

izmeez commented 10 months ago

I spun up a fresh site at backdropcms.org and tried the steps you describe and cannot replicate the problem you describe.

izmeez commented 10 months ago

The tugboat demo is BD 1.26.2

indigoxela commented 10 months ago

I spun up a fresh site at backdropcms.org and tried the steps you describe and cannot replicate the problem you describe.

It's essential to follow the exact steps, in order to put the old value into the cache_layout_path table. That cached, but outdated value will then be used to apply the layout to node/add/THETYPE - the wrong layout.

The visual glitch can be subtle, but with the "This page" dropdown you can verify that the wrong layout's in use.

argiepiano commented 10 months ago

Tested and reviews. RTBC

izmeez commented 10 months ago

I think the PR diff makes good sense.

quicksketch commented 8 months ago

@indigoxela I made a suggestion in the PR to use Layout module's dedicated cache clear function, in the event that other caches are in place (or are in the future). The admin theme setting is so tightly tied to the default admin layout that I think this is a reasonable thing to do.

indigoxela commented 8 months ago

I made a suggestion in the PR to use Layout module's dedicated cache clear function

Merging wasn't such a good idea... :laughing: Syntax error. There was a semicolon missing in your commit. I'll fix it, and rebase.

indigoxela commented 8 months ago

Done, merged-fixed-rebased. Do we need another round of testing? Or is it still RTBC?

quicksketch commented 8 months ago

There was a semicolon missing in your commit. I'll fix it, and rebase.

That's why we have testing :joy: Thanks for fixing that. With the parent function doing the exact same thing as before (plus additional cache clearing) this looks good to me. I'll get it on my next pass through RTBC issues.

quicksketch commented 6 months ago

Thanks @indigoxela, @izmeez, and @argiepiano! I merged https://github.com/backdrop/backdrop/pull/4639 into 1.x and 1.27.x.