Specific preprocess functions for theme hook suggestions are not invoked

The D8 solution broke Display Suite, so please ping me before this gets committed..

This is my best attempt to reroll #939462-62: Specific preprocess functions for theme hook suggestions are not invoked, which was from early 2012. I did not try to backport what was committed to D8.

Hello,

That system is already implemented in Atomium (see specific commit)

It's based on Bootstrap theme, but I improved it a bit to have a proper cascading execution of those functions.

So, you are now able to have preprocess and process functions for custom theme() calls per default.
We have included this feature in the theme, but a patch would be more than welcome.
This is, according to me, one of the best feature of the theming layer that we are missing in Drupal 7.

Examples:

theme('link__variant1__variant2', array(...));

The original hook is link and the preprocess functions that will be triggered, if they exists are:

1. HOOK_preprocess_link()
2. HOOK_preprocess_link__variant1()
3. HOOK_preprocess_link__variant1__variant2()

Another example:

theme('link__variant1__variant2__variant3', array(...));

1. HOOK_preprocess_link()
2. HOOK_preprocess_link__variant1()
3. HOOK_preprocess_link__variant1__variant2()
4. HOOK_preprocess_link__variant1__variant2__variant3()

It looks like there are a lot of problems with this patch/reroll
1) code to properly call templates with paths was deleted, thus it is setting template paths to be docroot if they weren't explicitly specified.

-        // Prepend the current theming path when none is set.
-        if (!isset($info['path'])) {
-          $result[$hook]['template'] = $path . '/' . $info['template'];
-        }

later this becomes doubly compounded when the template is actually called:

     $template_file = $info['template'] . $extension;
-    if (isset($info['path'])) {
-      $template_file = $info['path'] . '/' . $template_file;
-    }
     if (variable_get('theme_debug', FALSE)) {
       $output = _theme_render_template_debug($render_function, $template_file, $variables, $extension);
     }

these blocks help with 2 cases: 1) the theme element has a template named, and a path provided, 2) the theme element has a template with the path listed in it. (for example modules/toolbar/toolbar.tpl.php)

2) it is looking in the defined functions namespace and attempting to derive available pre/process function names. This is problematic because many theme elements include their preprocess functions in separate include files (for example bootstrap theme's bootstrap_preprocess_html() located in bootstrap/includes/html.vars.php. If this is to work correctly, all include files for all theme elements would need to be included before this line could run:

+function _theme_post_process_registry(&$cache, $theme, $base_theme, $theme_engine) {
+
+  // Get all user defined functions.
+  list(, $user_func) = array_values(get_defined_functions());
+  $user_func = array_combine($user_func, $user_func);
+
+  // Gather prefixes. This will be used to limit the found functions to the

I think this needs a harder look to ensure it's being correctly handled.

I think the whole registry generation should be revisited.

Lately, I've spend some hours on Atomium and I've updated the way the registry is altered.

I ended up rewriting each hooks... here's how I did:

/**
 * Implements hook_theme_registry_alter().
 */
function atomium_theme_registry_alter(&$registry) {
  // Retrieve the active theme names.
  $themes = _atomium_get_base_themes(NULL, TRUE);

  // Return the theme registry unaltered if it is not Atomium based.
  if (!in_array('atomium', $themes)) {
    return;
  }

  // Get all themes.
  $all_themes = list_themes();
  $current_theme_engine = $all_themes[$GLOBALS['theme']]->engine . '_engine';

  // Process registered hooks in the theme registry.
  _atomium_process_theme_registry($registry, $themes);

  // Process registered hooks in the theme registry to add necessary theme hook
  // suggestion phased function invocations. This must be run after separately
  // and after all includes have been loaded.
  _atomium_process_theme_registry_suggestions($registry, $themes);

  // Compile a list of prefixes.
  // The order of this is very important, see the doc at:
  // https://api.drupal.org/api/drupal/includes!theme.inc/function/theme/7.x
  $prefixes = array('template' => 'template')
    + module_list()
    + array($current_theme_engine => $current_theme_engine)
    + array_combine($themes, $themes);

  // Alter each hook and compile a complete list of preprocess/process functions
  // in the right order.
  foreach ($registry as $hook => &$info) {
    $variable_process_phases = [
      'preprocess functions' => 'preprocess',
      'process functions' => 'process',
    ];

    $stack = array($hook);
    while ($pos = strrpos(current($stack), '__')) {
      $stack[] = drupal_substr(current($stack), 0, $pos);
      next($stack);
    }

    foreach ($variable_process_phases as $phase_key => $phase) {
      $info[$phase_key] = array();
      foreach ($prefixes as $prefix) {
        $info[$phase_key][] = $prefix . '_' . $phase;

        // This is the code that ensure preprocess/process inheritance.
        array_map(function ($hook) use (&$info, $phase, $phase_key, $prefix) {
          $info[$phase_key][] = $prefix . '_' . $phase . '_' . $hook;
        }, array_reverse($stack));
      }

      // Ensure uniqueness of functions.
      $info[$phase_key] = array_unique($info[$phase_key]);

      // Filter out functions that does not exist.
      $info[$phase_key] = array_filter($info[$phase_key], function ($function) {
        return function_exists($function);
      });

      // Sadly we have to remove preprocess and process for hooks that provides
      // a theme function. (like the date module with date_display_single).
      // We could have this in Atomium but some modules badly implementing
      // attributes handling would fail.
      if (isset($info['function']) && function_exists($info['function'])) {
        $info[$phase_key] = array_filter($info[$phase_key], function ($function) use ($phase, $hook) {
          return (FALSE !== strpos($function, $phase . '_' . $hook));
        });
      }
    }

    $info['includes'] = array_unique(
      array_merge(
        (array) $registry[atomium_get_base_hook($hook)]['includes'],
        (array) $info['includes']
      )
    );

    // Ensure "theme path" is set.
    $info += array(
      'theme path' => $GLOBALS['theme_path'],
    );

    // Remove this member so each hook is independent and doesn't depend or
    // inherit of it's parent hook.
    // This prevent many situations where the preprocess/process calls orders
    // are not triggered in the right order.
    unset($info['base hook']);
  }
}

I worked on a new patch.

It's very simple and I'd like to have feedback.

I wonder why is this a bug report, and not a feature request or task?

I am not sure what is advertised in the documentation.
But in practice, theme processor variants are not supported in current version of Drupal 7. For any theme hook with a 'base hook', we only ever execute processors of the base hook.

I did run a egrep -R "^function .*_(pre|)process_.*__" on a test sites with a bunch of modules, and found nothing.

Running variant-specific processors would be an interesting feature to have, but it would be a change of behavior, which we need to design and define before we implement it. It is an API change, and possibly could break things on existing sites,

If some module or theme has a function like MYMODULE_preprocess_node__page(), which is currently not called, but would be called after a core update, this could cause unexpected behavior.
Why would such a function exist? Perhaps the module had its own way of calling this function, and now it would be called twice.

For designing this new feature, of course we should look at Drupal 8, where this is already working apparently.
In D8 it was implemented as a bug fix. I would say this was a miscategorization, it should have been a feature request there as well.

Proposed modification to this issue

Change issue title to something like "Support theme processor variants" or "Support theme processors specific to theme hook variants, like in D8", or "Support variant suffix for theme processors".

Change issue type to "Feature request".

In the issue summary, describe current behavior and intended future behavior, showing which additional processors would be executed.

@markcarver

It's a bug because, as the IS states: "there was similar functionality to this in Drupal 6, or at least in Views"

Historically you may be right.
But current versions of D7 contrib modules do not care much what used to work in Drupal 6. They assume the current behavior of Drupal 7 to be the "correct" behavior, as long as it is consistent in itself. Or, to say it differently, they are developed and tested to work correctly with the current behavior, and might not work correctly with a modified behavior, even if this restores something we used to have in D6.

The current behavior in D7 is limited, but it is internally consistent (in this aspect at least). Theme processors with variant suffix consistently do not work in current D7. This may have been an accident, but it is the situation we have now, since a long time, and that everybody got used to.

I would also suspect that the behavior in D6 was not perfect either. I read your little history, apparently it was changed a few times. (And assuming that something used to be strange and fragile is usually a good guess.)

From the perspective of a D7 developer without D6 history, this issue is a proposed API change. We are adding a new feature to a mostly consistent API, with possible side effects.
I don't even care so much about the formal issue category, as long as we understand that this is what we are doing.

This is a backport issue, this issue needs to mimic what was done in 8.x.

I will study how this works in D8, and then write more.
Whatever was done in D8: If it does cause hook variant processors to be called, then backporting it to D7 will be an API change.
We might not even want to port the complete behavior to D7, maybe just a subset of it, depending on possible side effects. I hope I will soon have something more substantial to say.

E.g. one thing we need to preserve is modifications to the theme registry from hook_theme_registry_alter(). Currently e.g. display suite adds 'ds_entity_variables()' to 'entity', and 'node' etc, and these added processors are also called for variants like 'node__page'.

I had a quick look at D8.
Relevant places in code:
- \Drupal\Core\Theme\Registry
- \Drupal\Core\Theme\ThemeManager::render()

Conclusion: Backporting this as-is would indeed prevent custom entries like 'ds_entity_variables' from being executed in variants / suggestions like 'node__page' or 'node__page__full'. Therefore it is a no-no.

How exactly?
- Core would register separate theme hooks for 'node' and 'node__page'.
- DIsplay Suite would add 'ds_entity_variables' to $registry['node']['preprocess functions'], but not to $registry['node__page']['preprocess functions'].
- theme() would only run the preprocess functions for 'node__page'.

I'm almost half templated to say that this issue should really just be marked as "Closed (won't fix)".

Or, as I suggested:
- Treat the current behavior as the "correct" or "expected" behavior, until we purposefully change it. Add tests and docs to make this explicit.
- Treat this issue as a feature request / proposed API change. Define intended future behavior compared to current behavior, for various edge cases. Write tests for intended behavior.
- Identify and discuss possible side effects.
- Decide whether to wontfix this for its side effects.

At least then we have a better idea and documentation of what we are doing or discarding.

Drupal Bootstrap already does quite a bit of "heavy" theme registry alterations (to fix this very issue BTW), see bootstrap_theme_registry_alter() and _bootstrap_process_theme_registry_suggestions().

Funny thing, Pol and I are/were working on a module which also aims to fix this, similar to bootstrap and atomium themes.
https://github.com/drupol/registryonsteroids (don't expect any miracles from this yet)
I still think we need a more clear definition what the module intends to do, and how the behavior is different with and without the module. This is how I came to this core issue, to better understand the current behavior in D7 core, and how we are trying to change it.

In this module we also have this question of whether custom entries like 'ds_entity_variables' should be propagated from a base hook to variants. If so, this is only possible after hook_theme_registry_alter() has run, because how else would we know about those entries.

I wonder how bootstrap handles this.

The other reason I am interested in this issue is #519940: Performance: optimize building theme registry by using get_defined_functions() instead of function_exists(), which is sadly blocked.

Backporting this now would just mean that I would have to spend a lot of time reversing what has already been done in 7.x contrib.

Yeah, which is a clear sign that this is an API change, if other modules and themes need to adjust to it.

Instead of trying to continue "fixing" the existing theme system in 7.x I've lately been attempting to solve a way to entirely replace it with 8.x code using https://www.drupal.org/project/backport. It's very promising.

If we are already in contrib, there are many options. And we don't need a consensus, because we can each publish our own solution, and the community will decide which works best.

I am personally not thrilled to have D8 code in my D7 site, it sounds like asking for trouble. But I have not tried or evaluated it yet, so I can't really say.

No, core doesn't register separate base theme hooks for theme suggestions. Theme hook suggestions are determined during runtime via the new theme suggestions hooks.

Yes, the suggestions are determined at runtime in ThemeManager->render().
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Theme%21T...

It then picks the first (last) suggestion that has an entry in the theme registry.
Any suggestion that does not exist in the registry is ignored.

Well, almost.
It is still added to $variables['theme_hook_suggestions'] later, but this is considered legacy behavior and does not influence which template or theme function is picked.

  foreach (array_reverse($suggestions) as $suggestion) {
    if ($theme_registry
      ->has($suggestion)) {
      $info = $theme_registry
        ->get($suggestion);
      break;
    }
  }

It will then run the preprocess functions registered for this variant/suggestion, not for the base hook.

By default, the variant info contains all the preprocess functions of its parents, including the base hook.
However, if a module uses hook_theme_registry_alter() to register an additional preprocess function for the base hook, this preprocess function will not be added to the variant.

Currently in D7, if a module like 'ds' registers a preprocess function like 'ds_entity_variables' for 'node', then this will also apply to 'node__page' or 'node__page__full'.
However, in D8 this would not work, unless the module explicitly adds the preprocess function to all variants.

- Treat this issue as a feature request / proposed API change. Define intended future behavior compared to current behavior, for various edge cases. Write tests for intended behavior.

Drupal 7 is, likely, nearing EOL. Adding a "new feature" (of this magnitude) is probably not the wisest solution or the best use of everyone's time now. Like I said, a lot has changed in the past 3 years since this backport issue was originally created. I no longer think it makes sense to continue down this path. Instead, we should just leave this "fix" to contrib base themes (or modules).

So again the correct category would be a "Feature request" with status = wontfix.

I think I agree with the wontfix. It would still be interesting to better understand and document what we were trying to do here, and which side effects or complications we are afraid of. I think I understand those side effects more or less at this time. But if I look at this issue one year in the future, I will no longer remember.

For the EOL, I don't know. There are still many D7 sites out there, and treating it as a dead software would do those projects a huge disservice. But this is a separate discussion.
By default it would be EOL when D9 comes out, which I think is not going to happen any time soon.

- Identify and discuss possible side effects.

There would likely be many. We couldn't even begin to think up how many or what kind, mainly due to the fact that this would be affecting hundreds of thousands of existing sites. Who knows what people have done to overcome this bug.

I think the biggest (but not the only) issue is the chicken-and-egg problem of theme hook suggestions vs process / preprocess functions.

• In D7, theme hook suggestions are registered from process / preprocess functions. This is not something we can change.
• In this issue, we want to determine process / preprocess functions based on theme hook suggestions.

We could try to determine process / preprocess functions based on the original hook argument, or based on theme hook suggestions passed into the $variables array when theme() is called. But only very few modules or themes currently use this functionality. Usually they will call the base hook, and then let preprocess functions build up the suggestions. This means, this version would require extra work from contrib modules.