Themes always get an extra chance at the end of \Drupal\Core\Theme\Registry::processExtension() to add preprocess to everything, and then also the extra-extra processing for theme suggestions (which I think we an clean up and combine if we extend the current preprocess-collected data and combine it, but that's for D12).
Theme engines currently also looks for theme prefixed functions, Modules don't do that. This check does not support OOP and it specifically does not support LegacyHook, so it might add preprocess twice. It also doesn't deal with base themes.
Adding a breakpoint within the second theme specific loop shows that theme preprocess functions are added twice, the array_unique() in \Drupal\Core\Theme\Registry::postProcessExtension() will then make it unique again.
There is a condition and if comment above that says "Check only if not registered by the theme or engine.", but the second part of that statement is untrue, as engines are processed in a separate call to processExtension(), so $result will never contain templates returned by engines.
This has wrong/unnecessary the since before 2013 when the theme registry discovery was converted to a service.
Remove the theme prefix when processing theme extension templates.
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes, plain diff MR !13613
Comments
Comment #3
berdirI had one fail in a test that first looked related in ThemeTest about switching the default theme, but it was an assert on a config value and must have been a random fail/race condition?
Comment #4
berdirComment #5
nicxvan commentedI suspected this was true when we did the theme oop issue but it's easier to confirm here without the noise.
Had very a lengthy discussion in slack about whether there is anything else to consider, but the issue summary sums up our conclusions rather nicely.
Comment #9
catchGood find.
Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!
Comment #12
longwaveWe could refactor away that foreach now it only has one iteration, will open a followup.
Comment #13
longwaveLet's not bother as this is in a deprecated path anyway.
Comment #14
longwaveComment #15
catchPer slack discussion we probably need to revert this due to early installer weirdness. Details to be filled out later (before or after the actual revery, but not doing it immediately).
Comment #16
nicxvan commentedThank you, after further investigation I think we found the root cause, we can address that separately here: #3562319: [regression] The installer does not properly load the theme
Putting this back to fixed for now.
Comment #18
dwwRestoring more metadata for posterity.
Comment #19
nicxvan commentedHate to ping pong a bit, but version should remain 11.x
Comment #20
dwwSee #9. 🤓 I think core committers move the version to the earliest real version a change was committed to.
Comment #21
catchYes this is true, it has got a bit messy with the 11.x branch because all the commits that go into 11.x before 11.3.x was opened don't get moved, but we still try to do that when whichever lower branch gets committed to is open.