Preprocess functions from base hooks not triggered for theme hooks not using the __ pattern

// part of ds_entity_view_alter
    if (!empty($field_value)) {
      $build[$key] = array(
        '#theme' => 'field',
        '#field_type' => 'ds',
        '#title' => $field['title'],
        '#weight' => isset($field_values[$key]['weight']) ? $field_values[$key]['weight'] : 0,
        '#label_display' => isset($field_values[$key]['label']) ? $field_values[$key]['label'] : 'inline',
        '#field_name' => $key,
        '#bundle' => $bundle,
        '#object' => $entity,
        '#entity_type' => $entity_type,
        '#view_mode' => '_custom',
        '#ds_view_mode' => $view_mode,
        '#items' => array((object) array('_attributes' => array())),
        '#is_multiple' => FALSE,
        '#access' => ($field_permissions && function_exists('ds_extras_ds_field_access')) ? ds_extras_ds_field_access($key, $entity_type) : TRUE,
        0 => array(
          $field_value,
        ),
      );
    }

// Part of ds_theme
$theme_functions[$plugin['theme']] = array(
  'render element' => 'elements',
  'function' => $plugin['theme'],
  'base hook' => 'field',
  'path' => drupal_get_path('module', $plugin['provider']),
);

This broke a lot of the DS functionality.
DS uses a plugin system to "field" templates.
Due to this change the preprocess field hooks aren't called anymore.

If we use '#theme' => 'field' the field preprocess function should be called if 'base theme' is equal to 'field'.

I maybe have a workaround for DS, but we clearly need a CR for this...
I'll post a link to the issue once it is fixed.

https://www.drupal.org/node/2575865
Fixed it for us, I wonder if we still need the base_hook stuff than...

Sorry @aspilicious, there are so many (too many IMO) ways to do theme suggestions, I agree we need more tests.

Added tests for this issue and better coverage for postProcessExtension in general. The patch fixes this issue as well as some more the tests came up with and rewrites completeSuggestion to be more readable.

lauriii: It does try to fix the issue that aspilicious references and that caused the creation of this issue. Correct me if I'm wrong but looked like it was caused by a hook_theme entry with a base hook but not utilizing the suggestion patterns at all.

The test called testManualBasehook() attempts to test that situation. The function mergePreprocessFunctions() that I added is called for hooks that aren't using the suggestion pattern.

Of course this touches on things other than that since I ended up fixing more than just this... I did consider making a separate issue for all the other changes but after the refactoring this one would be like a 2-3 line change with all the work already done... could still go with that if you prefer.

@Antti J. Salminen that would likely be better to fix just the one small issue and move the other changes to follow-up as it will be easier to review and commit I'd bet.

@joelpittet Sure, when claiming it would be tiny I thought it wouldn't be possible quite as nicely as it actually was in the end. The attached patch is now reduced in scope to just the parts directly relevant to this issue.

Fixed a couple of things based on lauriii's review/feedback and reduced mergePreprocessFunctions to only merge in the preprocess functions. Doing more will require specific handling of keys and seems out of scope for this issue.

Rerolling. It's been a while so maybe we can move forward anyway. Even in the somewhat unlikely case that aspilicious' issue it turned out to be a different one, this patch still does fix an issue and provides tests for it.

Got no time to verify if this works.
I trust the hero's above.

I assume it should still be set as needs review even if normal.

We now have a valid use case in core for this, it's currently being worked around.
See #2834019: Remove the "layout__" prefixing of theme hooks.

We could roll that into this issue if desired.

In the meantime, here is an updated patch fixing the indentation and some other issues.

Patch ran against 8.2.x, hm.

Getting sloppy at the end of the week.

Cleaned up the test methods to use @dataProvider

Looks ready to go, only minor nit.

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -632,6 +632,30 @@ protected function completeSuggestion($hook, array &$cache) {
+   * Merges the preceding hook's preprocess functions into the current hook's.

s/hook's./hook.

Not sure about the comment in #27.
This is blocking DS for having a working test suite on the 8.x-3.x branch. (https://www.drupal.org/node/2821201#comment-11821081)

The comment means "Merges the preceding hook's preprocess functions into the current hook's [preprocess functions]", and is valid grammar.

Updated the issue summary

1. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -632,6 +632,30 @@ protected function completeSuggestion($hook, array &$cache) {
   +   * Merges the preceding hook's preprocess functions into the current hook's.
   ...
   +   * @param string $complete_hook
   +   *   The name of the hook to merge preprocess functions from.
   ...
   +  protected function mergePreprocessFunctions($hook, $complete_hook, array &$cache) {
   

   Preceding or complete? I'm not sure either makes much sense. How about mergeBaseHookPreprocessFunctions() and $base_hook

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -710,7 +734,15 @@ protected function postProcessExtension(array &$cache, ActiveTheme $theme) {
   +        if (strpos($hook, '__')) {
   +          $this->completeSuggestion($hook, $cache);
   +        }
   +        else {
   +          $this->mergePreprocessFunctions($hook, $info['base hook'], $cache);
   +        }
   

   Is it not possible to have both __ and have $info['base hook'] set too? Or should we throw an exception if this is the case?

3. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -83,6 +90,14 @@ protected function setUp() {
      /**
   +   * {@inheritdoc}
   +   */
   +  protected function tearDown() {
   +    parent::tearDown();
   +    static::$functions = [];
   +  }
   +
   

   Nice test coverage and nice to see this being cleaned up.

After thinking about this more, I decided this approach was incomplete (hah!) and wrote some more test coverage.
There are 2 places that completeSuggestion() is called from, and each needs base hook support.

This rewrite makes #31.1 obsolete

#31.2, we should handle both! This does so, and adds coverage for it.

Restoring some more the original approach, I think this is a happy medium.
The do-not-test patch is the changes to Registry without the new variable names and with -w, just to show how little is changing

Okay great, that passes as it should.
This clarifies the logic (second check is redundant) and docs for the new code.

+++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
@@ -159,6 +174,171 @@ public function testGetRegistryForModule() {
+    $data['manual_suggestion'] = [
...
+      'hooks' => [
...
+        'test_hook__suggestion__with_base_hook' => [
+          'base hook' => 'unrelated_hook',
...
+      'expected' => [
...
+        'test_hook__suggestion__with_base_hook' => [
+          'preprocess functions' => [
...
+            'cat',
+            'mouse',
+            'bread',
+            'test_preprocess_test_hook__suggestion',
...
+          ],
...
+          'template' => 'test-hook',

This doesn't look like desired behavior. If the base hook is 'unrelated_hook', then we shouldn't be expecting the template to be set to 'test-hook' or to be accumulating preprocess functions from 'test_hook' and 'test_hook__suggestion'.

Possibly related, this patch makes some nice improvements to completeSuggestion(), but postProcessExtension() still has regex and string parsing on '__' that doesn't seem to account for 'base hook'.

The question boils down to "Should the base hook be in addition or instead of suggestions?"

When I was rewriting this I took a step back and tried to think of what would be least surprising.

Right now the base hook is ignored and the suggestions are used. The goal of the issue is to allow preprocess functions from the base hook to fire. But does that really mean we want to prevent hooks from suggestions from firing? That seems like a BC break, and not what I'd expect to happen.

Setting to NR for now. Let's discuss!

From the docs for hook_theme() in theme.api.php

* The base hook's files will be included and the base hook's preprocess
* functions will be called in place of any suggestion's preprocess
* functions.

So you're absolutely correct. But since that's not currently true, is it safe to switch to that behavior?

@effulgentsia and I stepped through this on a call, and came to the conclusion that the patch is correct, and that the docs (which neither agree with HEAD or the patch) should be updated.

I've also included an addition to the unit test that we came up with while discussing.

Thanks random failures

1. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -159,6 +174,189 @@ public function testGetRegistryForModule() {
   +      'defined_functions' => ['cat', 'mouse', 'cheese'],
   ...
   +          'preprocess functions' => ['cat', 'mouse'],
   ...
   +          'preprocess functions' => ['cheese'],
   ...
   +          'preprocess functions' => ['foo'],
   

   Unclear why some functions are in 'defined_functions' and others aren't.

2. +++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
   @@ -159,6 +174,189 @@ public function testGetRegistryForModule() {
   +          'base hook' => 'unrelated_hook',
   +          'template' => 'test-hook',
   

   That seems wrong, but maybe out of scope for this issue? This patch doesn't change code that deals with 'template', so perhaps fixing bugs and adding test coverage for that should be a separate issue?

In addition to above, I reworked the tests to be clearer on what they're testing and why. Please review to see if it captures everything we want captured.

+++ b/core/lib/Drupal/Core/Layout/LayoutPluginManager.php
@@ -129,10 +129,7 @@ public function processDefinition(&$definition, $plugin_id) {
-      $definition->setThemeHook('layout__' . strtr($template, '-', '_'));
+      $definition->setThemeHook(strtr($template, '-', '_'));
...
+++ b/core/tests/Drupal/Tests/Core/Theme/RegistryTest.php
@@ -159,6 +174,323 @@ public function testGetRegistryForModule() {
+    // Same as above, but also test that such child hooks can also be extended
+    // with magically named suggestions.
+    /*

If we're doing that first part in this issue, then I think we also need to be able to uncomment that commented-out test and make it work.

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -606,27 +606,56 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
+      $this->mergePreprocessFunctions($hook, $cache[$hook]['base hook'], $cache[$hook], $cache);
...
@@ -606,27 +606,56 @@ protected function processExtension(array &$cache, $name, $type, $theme, $path)
+   * @param array $parent_hook
+   *   The parent hook if it exists. Either an incomplete hook from suggestions
+   *   or a base hook.
...
+  protected function mergePreprocessFunctions($destination_hook_name, $source_hook_name, $parent_hook, array &$cache) {

I think the logic in both mergePreprocessFunctions() and its caller is correct (or at any rate, compatible with what's in HEAD), but the name and docs of $parent_hook is then incorrect, since what we're passing in is $cache[$hook], not $cache[$cache[$hook]['base hook']]. I'm not sure yet what a better name would be. In HEAD, it's pretty convoluted as to what "previous" means, especially when you look closely at the order of the lines of code.

@aspilicious mentioned in IRC that this issue (and I guess the follow-up in to remove the layout prefix for layout templates) is now officially a blocker for using the new field layout module in DS.

The same is true for my Widget project (https://www.drupal.org/project/widget) and I guess everyone who wants to avoid having that prefix, which is especially important if you have existing templates.

This is already major and tagged with contributed project, so not changing anything there :)

I really don't understand #43-45, or what is needed here.
#40 seemed fine to me.

I prefer #40 for now as it is the only patch that is ready to commit as is.
Display Suite needs this patch (for 8.3!) if we want a working Display Suite for the next half year....
I tested the patch yesterday and it worked.

AHA! Couldn't figure out why that didn't work, since the logic looked fine. Turns out we need to check either the source or destination for the incomplete flag, and then everything works.

Ticking some credit boxes.

+++ b/core/lib/Drupal/Core/Theme/Registry.php
@@ -647,7 +647,7 @@ protected function mergePreprocessFunctions($destination_hook_name, $source_hook
-    if (isset($cache[$source_hook_name]) && !isset($cache[$source_hook_name]['incomplete preprocess functions'])) {
+    if (isset($cache[$source_hook_name]) && (!isset($cache[$source_hook_name]['incomplete preprocess functions']) || !isset($cache[$destination_hook_name]['incomplete preprocess functions']))) {

I tried figuring out if this is sensible or not, but wasn't able to convince myself one way or the other. However, it seems to fix #44, and doesn't break any other test, so in my opinion, better to get this into the 8.3 alpha than not. So, pushed to 8.4.x and cherry picked to 8.3.x.

I'm concerned that there might be unexpected bugs caused by it, but partly why we have alphas is to find stuff like that. For both #45 and the difficulty in evaluating the above line of code, I opened #2848242: Improve variable names and inspect and document logic of Registry::completeSuggestion() and Registry::mergePreprocessFunctions() for some brave soul to wade into.

Thnx a lot, just to confirm that one of the failing tests (related to this issue) is now working on the 8.3 branch.

Maybe change record needed to notify about "incomplete process functions" in 8.3?

That was added in #939462: Specific preprocess functions for theme hook suggestions are not invoked, this just used it.

Follow-up: #2886187: Document the regex in Registry::postProcessExtension() used to find preprocess functions