Incorrect order and duplicate theme hook suggestions

#77 wasn't getting applied to 10.1.0, #83 also. I've make re-roll

#86 doesn't work well for me, admin menu bar suggestion comment:

INVALID FILE NAME SUGGESTIONS:
   See https://api.drupal.org/api/drupal/core!lib!Drupal!Core!Render!theme.api.php/function/hook_theme_suggestions_alter
   menu__admin
   menu

I also experienced the same error from this patch. I think the patch has a couple different issues. First, it looks like a lot of the derived suggestion logic was pulled from the twig.engine debug code, but the problem with that is that the debug output is presented in reverse order.

    $derived_suggestion = $original_hook;
    $derived_suggestions[] = $derived_suggestion;
    while ($pos = strrpos($derived_suggestion, '__')) {
      $derived_suggestion = substr($derived_suggestion, 0, $pos);
      $derived_suggestions[] = $derived_suggestion;
    }

This would generate hooks in the order of

base_hook__specific__really_specific
base_hook__specific
base_hook

When the later code merged this array with the suggestions array coming from the theme system (which is in the opposite order) it would create odd sorting under some conditions.

    $derived_suggestions_excluding_base_hook = array_slice($derived_suggestions, 0, -1);
    if (isset($info['base hook']) && !in_array($hook, $derived_suggestions_excluding_base_hook)) {
       $suggestions[] = $hook;
     }
    $suggestions = array_merge($derived_suggestions_excluding_base_hook, $suggestions);

Second, the patch had this check:

    $derived_base_hook = array_slice($derived_suggestions, -1);
    if (!in_array($derived_base_hook, $suggestions)) {
      $suggestions = array_merge($derived_base_hook, $suggestions);
    }

The above code would always fire since array_slice returns an array not a scaler value, which could sometimes cause duplicates.

Third, part of the updates removed some variable initialization in twig.engine

-      
-       // Get the value of the base hook (last derived suggestion) and append it
-      // to the end of all theme suggestions.
-      $base_hook = array_pop($derived_suggestions);

Later in the twig.engine file, the base_hook variable is used to determine whether a suggestion is valid or not

      $base_hook = $base_hook ?? $variables['theme_hook_original'];
      foreach ($suggestions as $key => &$suggestion) {
        // Valid suggestions are $base_hook, $base_hook__*, and contain no hyphens.
        if (($suggestion !== $base_hook && !str_starts_with($suggestion, $base_hook . '__')) || str_contains($suggestion, '-')) {
          $invalid_suggestions[] = $suggestion;
          unset($suggestions[$key]);
          continue;
        }
        $template = strtr($suggestion, '_', '-') . $extension;
        $prefix = ($template == $current_template) ? 'x' : '*';
        $suggestion = $prefix . ' ' . $template;
      }

That's the reason folks started getting the "INVALID FILE NAME SUGGESTIONS:" error after applying the patch. I made a new patch that addresses those 3 issues.

Seems to have failures and is missing test coverage. Moving to NW for that.

Attempting to fix the failures. I assume we'll want to wait until it's confirmed this is the approach people want before we write test cases though.

Fixing failing unit test.

Actually, still needs tests.

Yup, I just wanted to fix the issues first. I'll try to get a unit test going, but I have very little experience with it so it might take a bit.

This should probably move to an MR, the core team is using MRs for development now and the test suite is significantly faster on gitlab if you're writing tests.

I've converted the patch in 91 as closely as I can to 11.x, there was a refactor in https://www.drupal.org/project/drupal/issues/2953921 📌 Refactor out theme hook suggestion building from ThemeManager::render() into a separate function. Fixed that may have affected how this works. I have not added any additional tests, but I will review the MR now to see if it is working.

Hiding patches.

Can someone confirm this is still an issue on 11.x

I was unable to reproduce this, if you can update the steps to reproduce I'm happy to continue testing.

@nicxvan, the WxT distribution has been using patch #1 for several years.

With that said, we're using the bootstrap 3x theme which may not behave exactly as the core themes. With that said, it would be good to try the latest version of this patch and give it some serious looks.

Unfortunately I don't have any spare cycles at this moment.

There was some refactoring in the same area so I think that might have resolved this issue, but I'd like another confirmation.

Thanks for the updated patch. Here is an updated patch file for use with Drupal 10.2 based on the merge request usable with composer.json

phplint saw an undefined variable, replaced with what looked to be obviously the correct variable.

I can confirm this is still happening with Drupal 10.2.5 - WITH - the patch from #102 applied.

<!-- FILE NAME SUGGESTIONS:
   x page--node--general-page.html.twig
   * page--node--2.html.twig
   * page--node--%.html.twig
   * page--node.html.twig
   * page.html.twig
-->

I would expect that "page--node--2.html.twig" would be at the top of the list.

----

When I add some code I've used in past sites, to my .theme file

function themename_theme_suggestions_page_alter(array &$suggestions, array $variables) {
  // Add content type suggestions.
  if ($node = \Drupal::request()->attributes->get('node')) {
    array_splice($suggestions, 1, 0, 'page__node__' . $node->getType());
  }
}

It puts the template suggestion, in the right spot, but the TOP one is still there, which is wrong.

<!-- FILE NAME SUGGESTIONS:
   x page--node--general-page.html.twig
   * page--node--2.html.twig
   * page--node--%.html.twig
   x page--node--general-page.html.twig
   * page--node.html.twig
   * page.html.twig
-->

This is a tweak on the patch from comment #102. We have had this problem for a while and we've always had to, in the render function at the last point where the suggestions array is defined, apply an array_unique() to it. This has always fixed the problem for us, but I haven't tested thoroughly to see if this in fact fixes it everywhere. We are running Drupal 10.2.6 and PHP 8.1.27. Hope this helps!

I seem to have messed up the naming convention for the patch files. This is my first time uploading a patch file. Sorry about that! Hope I did it right this time. Below is from my previous post.

This is a tweak on the patch from comment #102. We have had this problem for a while and we've always had to, in the render function at the last point where the suggestions array is defined, apply an array_unique() to it. This has always fixed the problem for us, but I haven't tested thoroughly to see if this in fact fixes it everywhere. We are running Drupal 10.2.6 and PHP 8.1.27. Hope this helps!

Sorry folks, third time's the charm, or something like that. I uploaded the wrong patch file with my other two comments. This is the patch I am using for Drupal 10.2.6 and PHP 8.1.27. It is identical to the patch in comment #102 but with the addition of an array_unique() in the second hunk when setting the value of the $suggestions array in the render function. This fixes the issue for us. Hope it helps!

Can you please add those changes to the merge request? People can then generate local patches as needed and the core team uses the actual mr for testing and merging.

Looks like the issue summary needs an update? Drupal 11 with default Olivero theme, no changes from clean install other than turning on twig.config debug to true now results in this which appears correct:

The selected customisation comes from olivero.theme:

function olivero_theme_suggestions_menu_alter(&$suggestions, array $variables) {
  if (isset($variables['attributes']['region'])) {
    $suggestions[] = 'menu__' . $variables['attributes']['region'];
  }
}

For the region 'primary_menu'. Is there a step I am missing to reproduce?

I'm going to post this as it might help someone who stumbles over this issue (as I did). This is using Drupal 10.2.6 with Bootstrap5 subtheme.

When placing a menu block in a region (pretty normal thing to do) I get the following suggestions, prioritized first to last (as suggestions are array_reversed)

1. menu__region-name
2. menu__menu-name_region-name
3. menu__menu-name

This is counterintuitive because (in my mind at least) theme suggestions should be ordered from most specific to least specific i.e.

1. menu__menu-name_region-name (that menu when it appears in that region)
2. menu__menu-name (all instances of that menu)
3. menu__region-name (all menus in that region)

Can someone perhaps confirm if this is happening on other installs of Drupal? Or is it just me :D

Re #110 you're right and that's what this issue is about.

We're still experiencing this with Drupal 10.3.0, so I don't think this is fixed without this patch.

Is there a fix for this?