Problem/Motivation

HOOK_theme() offers an option 'override preprocess functions'. Here's the relevant documentation. See https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21...

 *  - override preprocess functions: Set to TRUE when a theme does NOT want
 *     the standard preprocess functions to run. This can be used to give a
 *     theme FULL control over how variables are set. For example, if a theme
 *     wants total control over how certain variables in the page.html.twig are
 *     set, this can be set to true. Please keep in mind that when this is used
 *     by a theme, that theme becomes responsible for making sure necessary
 *     variables are set.

This is not implemented at all.

Proposed resolution

I have two propositions.

  1. Remove the feature.
    1. Remove documentation.
    2. Remove the code that mentions this parameter. It doesn't actually do anything relevant. See below.
  2. Fix the feature.

Here's the only place where this parameter is used in the code:

\Drupal\Core\Theme\Registry

        // Check for the override flag and prevent the cached variable
        // preprocessors from being used. This allows themes or theme engines
        // to remove variable preprocessors set earlier in the registry build.
        if (!empty($info['override preprocess functions'])) {
          // Flag not needed inside the registry.
          unset($result[$hook]['override preprocess functions']);
        }
        elseif (isset($cache[$hook]['preprocess functions']) && is_array($cache[$hook]['preprocess functions'])) {
          $info['preprocess functions'] = array_merge($cache[$hook]['preprocess functions'], $info['preprocess functions']);
        }

Seems that this code doesn't do it's job anymore.

Steps to reproduce

For example set the 'override preprocess functions' for the html hook like this:

core/includes/theme.inc

/**
 * Provides theme registration for themes across .inc files.
 */
function drupal_common_theme() {
  return array(
    // From theme.inc.
    'html' => array(
      'render element' => 'html',
      'override preprocess functions' => TRUE,
    ),
...

Rebuild the cache and put {{ dump() }} inside core/themes/classy/templates/layout/html.html.twig. See how all the variables are added from the template_preprocess() and template_preprocess_HOOK() functions.

Remaining tasks

Let's first discuss how we should resolve this and then write the decided tasks here.

User interface changes

None.

API changes

Data model changes

Files: 
CommentFileSizeAuthor
#11 2561343-11.patch2.33 KBmarvin_B8

Comments

jiv_e created an issue. See original summary.

jiv_e’s picture

Issue summary: View changes
jiv_e’s picture

Issue summary: View changes

Fix typos.

lauriii’s picture

Priority: Normal » Major

Bumping to major

jiv_e’s picture

Issue summary: View changes

The following issue introduces the code https://www.drupal.org/node/166681.
See also @merlinofchaos's and @Dries's comments. https://www.drupal.org/node/166681#comment-592091

At least I understood that setting the parameter like this would disable all the preprocess functions.

/**
 * Provides theme registration for themes across .inc files.
 */
function drupal_common_theme() {
  return array(
    // From theme.inc.
    'html' => array(
      'render element' => 'html',
      'override preprocess functions' => TRUE,
    ),
...

But it doesn't. All the preprocess functions are run regardless of the 'override preprocess functions'. Or then I don't understand what the documentation says. Which is not so big of a problem if everyone else does. Do you?

I have to take my earlier claim in the issue summary back a little bit. I wrote about the current implementation "This (code) doesn't actually do anything because the unset array element is not used or referred anywhere." This is not necessarily the case because there's an elseif structure. I edit the issue summary to match my current understanding. Still it seems to be broken on Drupal 8.

Based on @dvessel's last comment on the pre-mentioned issue I think this could be removed. If I understand correctly we have more control on this kind of stuff on Drupal 8. Couldn't a theme even provide a custom ThemeManager service if it has these exotic needs?

valthebald’s picture

What's the consensus? Remove documentation about non-working feature?

Cottser’s picture

Title: 'Override preprocess functions' is not implemented » 'override preprocess functions' is not implemented, remove its documentation
Issue tags: +Needs subsystem maintainer review, +documentation

I think we need theme system maintainers to weigh in. Since I'm one of them I'll start:

Considering this no longer works we should probably just remove the documentation. Unless I'm missing something this can be accomplished if there is really a use case (it seems like an edge case) via hook_theme_registry_alter().

Digging deeper…

This was added before hook_theme_registry_alter() existed, and before that hook could be called from themes. See #166681-17: Overriding template preprocess functions not possible..

We can also grep contrib to see how often this was used in D7.

Cottser’s picture

Issue tags: +Twig
lauriii’s picture

This has been a way to do micro optimization for performance in the past. There is no performance gain anymore on run time for using 'override preprocess functions' => TRUE, since preprocess functions are cached in the theme registry. I can't think of the use cases where this is needed. I would consider just removing docs and any remaining pieces of the functionality.

Cottser’s picture

Issue tags: +Novice

Yup if it's broken it's not doing anyone any good. I'd also like to add that there is also the 'preprocess functions' key that could be used for this type of use case and I verified recently that it works :)

marvin_B8’s picture

Status: Active » Needs review
FileSize
2.33 KB

is that right ?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community
jiv_e’s picture

Issue summary: View changes
alexpott’s picture

Status: Reviewed & tested by the community » Needs review

Is this actually broken? It does not look broken to me. Given that removing this is API change and is probably only eligible for Drupal 9. It does look untested :)

Cottser’s picture

Title: 'override preprocess functions' is not implemented, remove its documentation » Add tests for 'override preprocess functions' functionality
Category: Bug report » Task
Priority: Major » Normal
Status: Needs review » Active
Issue tags: -documentation, -Novice +Needs tests
Related issues: +#166681: Overriding template preprocess functions not possible.

Did some digging on this today and I agree with @alexpott I think it does still work.

I ran git log -S "override preprocess functions" and only saw docs and the relevant code being moved around but not being functionally changed/broken. I'm adding what I think is the most relevant related issue that actually changed the behaviour here. The issue summary from this related issue isn't accurate because the code snippet there is showing an array of "override preprocess functions" rather than a boolean but looking past that this part is pretty relevant:

What this patch does is allow *any* invocation of HOOK_theme() to override all preprocess functions that were set *before* it not after.

So changing 'html' in drupal_common_theme() will have no effect since everything else will be running after that. You'd have to do it from later, not from the hook_theme that is defining the theme hook (we need better names for these things).

Based on that and @alexpott's comment I'm changing this to a normal task to add some test coverage for this feature because we definitely don't have any.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.