Theme registry doesn't find templates in subfolders for self-defined theme hooks

Trying to update the title to reflect what we figured out

This is reproducible without the amp module, removed that from the steps to reproduce.

The drupal_find_theme_templates() uses the cache built up from all the modules implementation of hook_theme() but twig_theme() gets called before the theme's implementations of hook_theme()

There is some unrelated code removal in here for $template_candidates, but I'm curious that that shouldn't break anything because that should never have a filename in the 'theme path' key from what I saw stepping through.

So the patch just moves the theme engines' hook_theme() calls last

It would be nice to have test coverage to prove this use case is working so that we don't break it in the future

Here's a test that demos the problem.

The fix looks correct to me based on the assumptions that we make in core. I'm not sure if there are any themes out there that are expecting that theme hook for the theme engines has been called before themes. This could be possible but mostly very unlikely.

I looked back on the issue where this bit of code was originally introduced #137211: Move discovery of theme information to .info files (and improve theme inheritance) and couldn't find any notes about the order. Because of that, I am okay with this change.

+++ b/core/modules/system/tests/themes/test_theme/test_theme.theme
@@ -6,6 +6,19 @@
+    'skip_rope' => [

Feel free to rename the theme hook name to tell what we are testing here. It is hard to remember in future what it's testing without looking at the test class.

Changed the theme hook name.

Thanks @joelpittet. Still RTBC!

I guess now we know render element is default for hooks. Adding variables key back in passes tests.

Back to RTBC there isn't anything different in the patch besides the lack of that key.

+++ b/core/includes/theme.inc
@@ -240,8 +240,7 @@ function drupal_find_theme_templates($cache, $extension, $path) {
-        $template_candidates = array($info['template'], str_replace($info['theme path'] . '/templates/', '', $info['template']));
-        if (in_array($template, $template_candidates)) {
+        if ($template == $info['template']) {

Per @joelpittet, this hunk should actually be removed from this patch.

@lauriii, @joelpittet, @Cottser, @alexpott, and I discussed the issue at length and decided to defer triage on this. Per @lauriii, this is a contributed project blocker for work on theme components. It might be more properly a major task.

@Cottser had some concerns about the risk of the change, so @alexpott, @Cottser, and I agreed that at a minimum this change would need to be minor-only. We might also consider another solution to reduce the risk (e.g. adding a new hook). Assigning to @Cottser for further committer review.

Removed the theme.inc change and moving that to a separate issue because it doesn't relate to this fix, just a by product of this fix.
#2821420: drupal_find_theme_templates() searches candidates that are never have templates and are duplicates

I'm working on addressing concerns that @Cottser might have by adding a new post_theme hook for the theme engines for this purpose.

Let's see if this works

@lauriii, that looks like it makes the scope of this change bigger, could you explain your changes a bit more please.

Before it was just changing the order from:

1. modules
2. engines
3. themes

to

1. modules
2. themes
3. engines

What are the other changes and why are they needed?

Reposting my patch from #17 because of what I mentioned in ##2 and #19 introduced an API change with changing twig_theme to twig_theme_post_build

@lauriii please explain the changes further before reposting #19

Two reasons why I think we should take the approach from #19:

1. It makes conceptually sense to first process theme engine vs. theme. The loading order goes from higher level extensions to lower level extensions. Themes can depend on theme engines but not the other way around. This is the same approach as we've taken with hooks as well. The fix for this shouldn't be making an exception for the loading order because we want to do conceptually something new; we want the theme engine to be able to be able to make modifications in the post-process phase (when all theme implementations have been added).
   • modules
   • engines
   • themes
2. The even more worrysome issue is that approach taken on #17 would break BC. If anyone has built custom theme engine, and a theme that depends on the logic in the theme engine, we wouldn't only break this, but make it impossible to tackle, since there wouldn't be any hook available for the theme engine to make modifications to theme hooks before the theme.

The point you brought up in #23 is quite interesting and I haven't thought about it before. The interesting part is that twig_theme is a hook, and therefore it shouldn't be called directly. However, it looks like we don't have anything about this in the Drupal 8 BC policy.

Re-rolled against 8.5.x.

Confirming that the description in #7 is spot-on. I'd like to add that this also breaks the use of theme suggestions for hooks that are defined in a theme, causing suggestions to always use the base hook template (even though preprocessing for suggestions will run, if implemented).

Re-roll of the patch for D9.