Declaring a preprocess function for a theme hook suggestion forces base template to be used at render time

Confirmed on 8.3.x. Doing some debugging.

I see the problem here but I don't think it has an easy answer.

We have two different things falling back to the same template.

When you implement hook_preprocess_field__comment__comment_body but do not provide a template, the following happens while the theme registry is being built:

It looks for the template field__comment__comment_body.html.twig.

When that is not found, it looks for a less specific template by slicing off the name at the double underscores. Therefore, it looks for field__comment.html.twig.

That does exist so it attempts to use it to render the body field but that's not what it is designed for. It's purpose is rendering the comment field of the node, i.e., the whole comment thread. It doesn't print anything for the body field.

I doubt if anything can really be done at this point given the naming rules. I guess the workaround is to copy field--test-long.html.twig to field--comment--comment-body.html.

Yeah, I agree it's a bug. Someone a little deeper into the theming system might have some ideas.

If I'm understanding this correctly, it's a pity that the field type for "comments", which is defined in the comment module, has an id of "comment". It seems like it would have been better to call that "comments" to avoid conflict with the entity id of "comment".

The "conflict" happens in Registry::completeSuggestion but I don't see a way to avoid it.

But wait ... maybe the comment module could implement a hook to modify the registry to avoid this. I'll look into that.

I think this will fix it. It's a bit of a bandaid but I think my logic is correct in that it will only catch this situation.

I think renaming the field would be very disruptive at this point.

Just removing an extra space.

Just noticed the docblock was wrong.

I'm a bit unsure if this is a bug or not. You could argue, that if you implement the hook, you should also provide a suitable template suggestion. I have called for subsystem maintainer, so we can get feedback from some one who knows the code a bit better. I would like get a green light before we start implementing a solution as (s)he might have some good ideas for a solution.

If we do want to go ahead and make a solution, I have a few comments for the patch itself.

1. We need a test case which which should fail without and proposed fix and pass with the fix applied. You can do this by creating a test only patch.

2. hook_theme_registry_alter seems like the wrong approach here. It will make some assumptions about the template being used, and while in some cases it will do a good job at it, I can image a few cases where the behavior wont be what we want it to be. If we can't get it right for all cases, it will be better not to try to fix it IMO, as it will be easier to developers to track down, than this hook changing the theme registry in case they actually want to use `field__comment.html.twig` for the field (which might be the case in custom themes).

3. When implementing hooks, documentation should be Implement hook_foo(). (you are missing (). in your patch.)

Thanks googletorp. I agree with your comments. It's a bit of a weird "bug" so let's wait for feedback from the maintainer.

I'm a bit unsure if this is a bug or not.

It's most definitely a bug. One that should have been squashed by #939462: Specific preprocess functions for theme hook suggestions are not invoked. So not sure if something else has been committed that caused a regression or there just wasn't enough tests in the first place. But I have noticed a similar issue lately while working on other things.

hook_theme_registry_alter seems like the wrong approach here

Indeed. This is not a problem that is unique to just field comments. Any "fix" will likely need to be in the theme registry itself or ThemeManager::render. For this reason, I'm setting this issue to CNW.

I don't really have the time to take an in-depth look at this as it will likely require a lot of step-through debugging.

I'll leave the title for now as I'm not sure what a more accurate one would be ATM.

#2886187: Document the regex in Registry::postProcessExtension() used to find preprocess functions may be the possible culprit.

Updating the issue title to be more general -- this affects more than just comments.

We just spent several hours trying to track down why one of the 16 fields in our content type (e.g. "my_type") is using base field.html.twig template while the rest of the fields are using the correct theme template of field--node--my-type.html.twig, despite the fact that our code does not force the template on this field in any way. The only thing different about this field was that it has a pre-process function in module code to fix-up the URL that's used for a taxonomy link.

After stepping through theme function suggestion resolution code in \Drupal\Core\Theme\ThemeManager::render(), I could see that there is a theme function in the theme registry that corresponds with the pre-process function, but the entry is forcing the use of the base theme implementation (field.html.twig) instead of the more-specific template. If we remove the pre-process hook and rebuild registries, the correct template gets used. This is def. a bug in core.

This issue actually looks worse than I thought... it looks like Drupal binds the template for the pre-process hook to one from Seven/Classy instead of even using the field template in our theme or its base theme (Radix). (Below, the template should be "field--node--my_type" not "field").

Luckily, there is a workaround:

/**
 * Implements hook_theme().
 */
function my_theme_theme($existing, $type, $theme, $path) {
  return [
    // BUGBUG: This is a workaround for DDO-2866208. We have to force Drupal to
    // use the correct theme template because it ignores template suggestions
    // when a pre-process hook is present.
    'field__node__field_my_type_collections__my_type' => [
      'template'  => 'field/field--node--my_type',
      'base hook' => 'field',
    ],
  ];
}

As described, the bad guy is Registry::postProcessExtension($cache, $this->theme).
but I'm not able to write a bugfix :-(
Here is one more example which shows, that this bug is really annoying:

1. add Nodetype "blog"
2. add viewmode "hero"
3. render a blog with viewmode hero
4. use template node--blog--full.html.twig -> works fine
5. use template node--hero.html.twig -> works fine
6. add HOOK_preprocess_node__blog()
7. now Registry::postProcessExtension() adds $cache['node__blog'] with values:
   • 'theme path' => 'core/themes/stable' // this is the base theme - not my theme !
   • 'template' => 'node' // typically it would be 'node--blog'
8. Instead of node--hero.html.twig this tpl is now chosen: node.html.twig from base theme
   Why? Because the theme-suggestion "node--blog" comes before "node--hero" and "node--blog" has the bad entry from above...