Move template_preprocess, _template_preprocess_default_variables into services

_theme() was removed in #2346937: Implement a Renderer service; reduces drupal_render / _theme service container calls. I think the other functions are not *as* important to move, as they're not called directly by modules.

I was wondering if we can't just inline template_preprocess into the ThemeManager when I was looking at that. We're adding that for every single theme function to the registry, seems pretty pointless. $this->defaultPreproces() should be a lot faster...

There is no way to change or override that anyway unless you remove it manually from the registry for a given entry I think.

I'm working on this.

Would this be ok?

I'm open to criticism. This is my first substantial core patch.

Looks pretty good at first glance A suggestion, chuck changes to this in an "ignore" issue to help weed out the remaining errors(unless of course you know the problems are fixable with one change, or tested all the failures manually).

+++ b/core/lib/Drupal/Core/Render/theme.api.php
@@ -1178,19 +1179,20 @@ function hook_theme_registry_alter(&$theme_registry) {
+ * //TODO This would be an API change. Should we stick with 'template_preprocess'?

Try to avoid API changes to help this issue get in at this late in the game or it may get bumped to 8.1 or 9.

Let's see if this fixes anything.

Don't want to give false hopes on this getting in either. It would be good to get a committers opinion as this is not a bug it needs a beta evaluation stating its case and normal tasks don't usually.

Check it out now you funk soul bot.

Great effort. if this gets in, part of #2493989: template_preprocess() and other preprocess functions are called even for theme hooks implemented as functions already gets fixed as well. I believe I fixed a couple of issues I spotted so template_preprocess isn't looked for and the variables only get added for functions.

Can we do this in 8.1 or is this a 9 task?

I'm guessing, since we are in RC for 8.0.x

Can happen in a minor version I think,

Here's a rerolled patch. Nothing on the code level was changed in core from 8.0.x, just some comments and a test file was moved in different directory. I did some cleanup. In my opinion this should be ready after Remaining tasks are finished. I couldn't make a clean interdiff because the core code had changed around the last patch.

So the question is: Should we deprecate the template_preprocess() or just remove it?

And here's the forgotten patch!

Here's a rerolled patch. Nothing on the code level was changed in core from 8.0.x, just some comments and a test file was moved in different directory. I did some cleanup. In my opinion this should be ready after Remaining tasks are finished. I couldn't make a clean interdiff because the core code had changed around the last patch.

So the question is: Should we deprecate the template_preprocess() or just remove it?

theme_preprocess() should be removed. I think nobody should use theme_preprocess() anywhere in contrib.

Ummm.... what?

Removing preprocess is kind of a huge deal. Is there some issue somewhere where the reasoning for that is discussed? There's a lot of important variable preparation work that happens in preprocess. Would like to hear more explanation about this.

Nobody is talking about removing the concept of preprocess. Just removing the old function, that is now unused, after the logic was moved to those new methods.

That said, I'm not sure if that is possible. Yes, they shouldn't be called directly, but who knows. Maybe leave them as empty functions that are deprecated? Or simply don't touch them at all, it's just a bit of dead code laying around.

1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -462,4 +457,53 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +  /**
   +   * {@inheritdoc}
   +   */
   +  public function addDefaultTemplateVariables(&$variables) {
   +    $variables += $this->getDefaultTemplateVariables();
   +  }
   

   This method doesn't seem useful anymore? Can't we just inline this where it is called?

2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -462,4 +457,53 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +    static $drupal_static_fast;
   +    if (!isset($drupal_static_fast)) {
   +      $drupal_static_fast['default_variables'] = &drupal_static('getDefaultTemplateVariables');
   +    }
   +    $default_variables = &$drupal_static_fast['default_variables'];
   

   this should be changed to a normal protected property.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -462,4 +457,53 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +      \Drupal::moduleHandler()->alter('template_preprocess_default_variables', $default_variables);
   

   should use the injected module handler.

4. +++ b/core/lib/Drupal/Core/Theme/ThemeManagerInterface.php
   @@ -140,4 +140,27 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL);
   +  public function addDefaultTemplateVariables(&$variables);
   +
   +  /**
   +   * Returns template variables that are set for every template by default.
   +   */
   +  public function getDefaultTemplateVariables();
   +
   +  /**
   +   * When theming an element, this function merges its #attributes into
   +   * $variables['attributes']. This will happen only if the $render_element_type
   +   * with key #attributes is found from the $variables.
   +   */
   +  public function addElementAttributeVariables(&$variables, $render_element_type);
   

   as mentioned above, I think the first should be removed completely.

   Maybe the second could be public, but adding public methods to an interface is always a bit tricky in minor releases, probably should be protected instead and not on the interface. And same for the third.

5. +++ b/core/modules/user/user.module
   @@ -561,7 +561,7 @@ function user_user_login($account) {
      // Reset static cache of default variables in template_preprocess() to reflect
      // the new user.
   -  drupal_static_reset('template_preprocess');
   +  drupal_static_reset('getDefaultTemplateVariables');
   

   if we no longer use drupal_static(), then we need a new public method on the Registry instead. So looks like we need a new public method there after all?

6. +++ b/core/themes/stable/templates/form/dropbutton-wrapper.html.twig
   @@ -6,7 +6,7 @@
     *
   - * @see template_preprocess()
   + * @see Drupal/Core/Theme/ThemeManagerInterface::addDefaultTemplateVariables()
     */
   

   not sure why a bunch of templates have this reference. This is called for all, why are those special? maybe it should just be removed, if those default properties are not already documented in a global place, then we should make sure that happens now.

@Berdir, thanks for a great review!

1. You are correct! I removed the method entirely.
2. I left this public to use inside deprecated template_preprocess().
3. I don't understand your suggestion. Can you clarify?
4. I removed first and third. I left the getDefaultTemplateVariables() as mentioned in 2.
5. We are using drupal_static(). I haven't changed that. I just changed the id string. What do you mean?
6. This issue is not about removing the references from template files. But we don't have a function replacement for template_preprocess() anymore so I think the only reasonable option is to remove the references. So I did.

I added template_preprocess() back and marked it as deprecated. Then I refactored my patch a bit for using the ThemeManager code inside that. If we decide to accept this direction we should consider removing the getDefaultTemplateVariables() method later. In that case I could make a follow up issue for Drupal 9.

Submitting a new patch for review.

Thanks for adding it back and marking it deprecated, that should help this get into 8.x branch with the BC layer

Another try.

We are using drupal_static(). I haven't changed that. I just changed the id string. What do you mean?

My point is that we should change it. drupal_static() is for functions, it should not be used for services (for example because it is a function and prevents unit testing). Instead, just make $this->something, it will be much simpler. The problem you need to solve then is invalidation, for those special cases like log out and log in.

We already have reset(), but that invalidates the caches, we don't want that evey time someone logs in. So we need a new method, something like resetDefaultTemplateVariables(), that you need to call instead of a drupal_static_reset().

Hm, of course that would be an API change if someone relies on that reset right now. So we have two options: keep it, add a @todo with a follow-up issue for 9.x and change it then. Or add a BC layer to drupal_static_reset(), if the key is this, then call the new method.

+1 for #31, new method to reset the static cache, because the function name is changing.

Trying something with how the element merges the defaults because that is sure to mess up the cache anyway and seems to fix things. #28 breaks on like the extend page /admin/modules.

1 fail! Unacceptable testbot! @jiv_e could you review and see if my changes are going in the right direction and if so maybe track down what's wrong with that one failed test (which looks related to my change in some way)

Here you go!

Thanks for helping out!

@jiv_e thanks for fixing the render element error. One thing to note I removed the parameters to the getDefaultTemplateVariables method so that argument doesn't do anything, is that going to fly with you?

Yes, flies fine.

It seems that deprecated template_preprocess still needed some attention. I run some theme related SimpleTests on my dev machine through the template_preprocess to make sure that the deprecation works. Should we still write some tests for the deprecation to make sure it doesn't break?

How I used template_preprocess()

    // Set default variables before preprocess hooks.
    template_preprocess($variables, $hook, $info);
    //$variables += $this->getDefaultTemplateVariables();

    // Merge element #attributes into $default_variables['attributes'] if they
    // exist in render_element.
    //$render_element_type = isset($info['render element']) ? $info['render element'] : NULL;
    //if (isset($variables[$render_element_type]['#attributes'])) {
    //  $variables['attributes'] = NestedArray::mergeDeep(
    //   $variables['attributes'],
    //    $variables[$render_element_type]['#attributes']
    //  );
    //}

Results

$ php core/scripts/run-tests.sh Theme Template Render Twig

Drupal test run
---------------

Tests to be run:
  - Drupal\KernelTests\Core\Render\Element\TableTest
  - Drupal\KernelTests\Core\Theme\ImageTest
  - Drupal\KernelTests\Core\Theme\MessageTest
  - Drupal\KernelTests\Core\Theme\RegistryTest
  - Drupal\KernelTests\Core\Theme\StableLibraryOverrideTest
  - Drupal\KernelTests\Core\Theme\StableTemplateOverrideTest
  - Drupal\KernelTests\Core\Theme\StableThemeTest
  - Drupal\KernelTests\Core\Theme\ThemeRenderAndAutoescapeTest
  - Drupal\KernelTests\Core\Theme\ThemeSettingsTest
  - Drupal\KernelTests\Core\Theme\TwigMarkupInterfaceTest
  - Drupal\KernelTests\Core\Theme\TwigWhiteListTest
  - Drupal\system\Tests\Theme\EngineNyanCatTest
  - Drupal\system\Tests\Theme\EngineTwigTest
  - Drupal\system\Tests\Theme\EntityFilteringThemeTest
  - Drupal\system\Tests\Theme\FastTest
  - Drupal\system\Tests\Theme\FunctionsTest
  - Drupal\system\Tests\Theme\HtmlAttributesTest
  - Drupal\system\Tests\Theme\ThemeEarlyInitializationTest
  - Drupal\system\Tests\Theme\ThemeInfoTest
  - Drupal\system\Tests\Theme\ThemeSuggestionsAlterTest
  - Drupal\system\Tests\Theme\ThemeTest
  - Drupal\system\Tests\Theme\ThemeTokenTest
  - Drupal\system\Tests\Theme\TwigDebugMarkupTest
  - Drupal\system\Tests\Theme\TwigExtensionTest
  - Drupal\system\Tests\Theme\TwigFilterTest
  - Drupal\system\Tests\Theme\TwigLoaderTest
  - Drupal\system\Tests\Theme\TwigNamespaceTest
  - Drupal\system\Tests\Theme\TwigRawTest
  - Drupal\system\Tests\Theme\TwigRegistryLoaderTest
  - Drupal\system\Tests\Theme\TwigSettingsTest
  - Drupal\system\Tests\Theme\TwigTransTest
  - Drupal\Tests\Core\Theme\AjaxBasePageNegotiatorTest
  - Drupal\Tests\Core\Theme\RegistryTest
  - Drupal\Tests\Core\Theme\ThemeNegotiatorTest
  - Drupal\Tests\system\Kernel\Render\ClassyTest

Test run started:
  Tuesday, September 6, 2016 - 02:45

Test summary
------------

Drupal\KernelTests\Core\Render\Element\TableTest              13 passes                                      
Drupal\KernelTests\Core\Theme\ImageTest                        4 passes                                      
Drupal\KernelTests\Core\Theme\MessageTest                      1 passes                                      
Drupal\KernelTests\Core\Theme\RegistryTest                     4 passes                                      
Drupal\KernelTests\Core\Theme\StableLibraryOverrideTest        1 passes                                      
Drupal\KernelTests\Core\Theme\StableTemplateOverrideTest       1 passes                                      
Drupal\KernelTests\Core\Theme\StableThemeTest                  2 passes                                      
Drupal\KernelTests\Core\Theme\ThemeRenderAndAutoescapeTest    16 passes                                      
Drupal\KernelTests\Core\Theme\ThemeSettingsTest                2 passes                                      
Drupal\KernelTests\Core\Theme\TwigMarkupInterfaceTest         10 passes                                      
Drupal\KernelTests\Core\Theme\TwigWhiteListTest                1 passes                                      
Drupal\system\Tests\Theme\EngineNyanCatTest                    4 passes                                      
Drupal\system\Tests\Theme\EngineTwigTest                      47 passes                                      
Drupal\system\Tests\Theme\EntityFilteringThemeTest            88 passes                                      
Drupal\system\Tests\Theme\FastTest                            13 passes                                      
Drupal\system\Tests\Theme\FunctionsTest                       45 passes                                      
Drupal\system\Tests\Theme\HtmlAttributesTest                   5 passes                                      
Drupal\system\Tests\Theme\ThemeEarlyInitializationTest         5 passes                                      
Drupal\system\Tests\Theme\ThemeInfoTest                       14 passes                                      
Drupal\system\Tests\Theme\ThemeSuggestionsAlterTest           63 passes                                      
Drupal\system\Tests\Theme\ThemeTest                           89 passes                                      
Drupal\system\Tests\Theme\ThemeTokenTest                      17 passes                                      
Drupal\system\Tests\Theme\TwigDebugMarkupTest                 11 passes                                      
Drupal\system\Tests\Theme\TwigExtensionTest                   19 passes                                      
Drupal\system\Tests\Theme\TwigFilterTest                      18 passes                                      
Drupal\system\Tests\Theme\TwigLoaderTest                       3 passes                                      
Drupal\system\Tests\Theme\TwigNamespaceTest                    6 passes                                      
Drupal\system\Tests\Theme\TwigRawTest                          4 passes                                      
Drupal\system\Tests\Theme\TwigRegistryLoaderTest              19 passes                                      
Drupal\system\Tests\Theme\TwigSettingsTest                    20 passes                                      
Drupal\system\Tests\Theme\TwigTransTest                      105 passes                                      
Drupal\Tests\Core\Theme\AjaxBasePageNegotiatorTest             7 passes                                      
Drupal\Tests\Core\Theme\RegistryTest                           1 passes                                      
Drupal\Tests\Core\Theme\ThemeNegotiatorTest                    4 passes                                      
Drupal\Tests\system\Kernel\Render\ClassyTest                   1 passes                                      

Test run duration: 12 min 16 sec

If possible it would be great if we could write some tests for the deprecated defaults + render element combo. I think this is looking great, may want to get framework manager signoff before the tests though, to make sure we can get this past a sniff test. I've tagged as such.

1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -462,4 +465,34 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL) {
   +    static $drupal_static_fast;
   

   We should just use a class property here, no need for drupal_static().

2. +++ b/core/lib/Drupal/Core/Theme/ThemeManagerInterface.php
   @@ -140,4 +140,22 @@ public function alter($type, &$data, &$context1 = NULL, &$context2 = NULL);
   +  public function getDefaultTemplateVariables();
   

   Adding a method to an interface is an API change, we only allow that in very specific circumstances. Why does this have to be public?

   I can see the bc-layer calls this, but the bc-layer could also just duplicate the code.

Also I left a comment on #2550249: [meta] Document @internal APIs both explicitly in phpdoc and implicitly in d.o documentation prompted by this issue. There are no Drupal 7 modules on drupalcontrib.org that call this: http://www.drupalcontrib.org/api/drupal/drupal%21includes%21theme.inc/fu... so I'm wary about adding an actual bc layer for something that should never have been used in the first place. Feels like the current documentation is ambiguous on procedural functions.

@catch, thank you for the feedback. Great questions that really made us(@jiv_e and I) think.

1. #39.1 Yes, probably but if we do any existing code attempting to clear the static cache will fail (consider we are probably going to have to change that back to a key 'template_preprocess' for that reason)
2. #39.2 It's an API addition which I'm sure is distinct from an API change, or at least should be. Regardless, the public really only serves as help for the BC layer to avoid code duplication, which could cause fragmentation if they are changed independently after the this goes in. Though I'd prefer to make it private/protected, it would be better to flat out remove the function IMO, but that can be quite tricky for BC and that drupal_static() gets called in some interesting circumstances (even in core) and is more of the API break than removing the function I believe.

As an added note, I downloaded all the D7 modules (via https://www.drupal.org/sandbox/greggles/1481160) to see how it's being used in contrib, these are the only calls:

allmodules/dcco/includes/theme.inc
1195:      template_preprocess($default_template_variables, $hook);

allmodules/devel_themer/devel_themer.module
613:      template_preprocess($default_template_variables, $hook);

allmodules/rdfa/rdfa.module
145:  template_preprocess($variables, 'panels_flexible');

allmodules/rebuild/tests/fixtures/drupal_sites/dev/includes/theme.inc
1192:      template_preprocess($default_template_variables, $hook);

allmodules/rebuild/tests/fixtures/drupal_sites/prod/includes/theme.inc
1192:      template_preprocess($default_template_variables, $hook);

And nobody in contrib is actually resetting with static cache at all in D7. And the only other use is a marker when people are re-ordering preprocess functions, they put theirs first just after template_preprocess, which was my bigger concern anyway.

@joelpittet don't forget custom code, who knows what they are doing. The only use case for a reset would be people implementing the alter hook to add more default variables which would need to be reset, But I think that's pretty far-fetched, those we actually do somthing like that more likely just add another global preprocess implementation.

1: As suggested in #31, we could add a BC layer directly in drupal_static_reset(), special case that key and call the new method. keeping it and chaning the key is definitely pointless, would only make sense to keep it if we keep the old string. But I think my other idea would be quite nice, as we can keep BC out of the new code and it is a pattern that we could easily use for other drupal_static()'s.

2. Adding a method to an interface breaks custom implementations. Those are unlikely to exist, but this is still a change technically, not an additional. And not adding it but still keeping it public will still break sites with a custom implementation, just in a more confusing way. We usually only allow this if we have a base class that people are expected to implement.

This issue is badly needed, but I think we could do a one up and make this a goal of #2869859: [PP-1] Refactor theme hooks/registry into plugin managers (which would ultimately replace ThemeManager).

This would allow themes (base themes) the ability to finally and properly hook into these "default variables".

Tempted to mark this as a dup or won't fix. Or maybe the title can just be repurposed and moved as a child of #2869859: [PP-1] Refactor theme hooks/registry into plugin managers.

I think this remains a good idea but we're going to need to update the way code is deprecated to trigger E_USER_DEPRECATED errors and not remove functions. Also this will need to be done in 10.1.x.

We also shouldn't be using drupal_static_fast in OO code... plus we'll need a BC layer in drupal_static to ensure whatever we've set on the theme manager is cleared when that's called.

per #55 there are some changes needed.

Also was tagged for needs tests

Converted the 8(!) year old patch to a merge requests. Rebase was messy (surprise!) and I will definitely still need to do another pass to clean up remaining references to template_preprocess(), update deprecation and drupal_static() stuff, but tests *are* passing, that part was easier than expected, so setting to needs review for now.

Starting to bringt this into shape.

Unsure on the BC layer on template_preprocess(). I checked https://git.drupalcode.org/search?group_id=2&scope=blobs&search=%22templ... and not a single one of those is for D8+, lots of copied core files and weird stuff.

I removed the method from ThemeManagerInterface and just add a method exists and marked it as internal.

For now, I didn't add a new public method to reset the variables, I just added it to resetActiveTheme(). Default variables acually depend on the active theme, and for the use case in user logout/login, it doesn't seem like bad idea to reset the active theme, since the active theme can depend on permission.

And per #41, that is literally the only use case. There are like 5 implementations of template_preprocess_default_variables_alter() hook in contrib, most notably the mgv project. And not one of them bothers with the reset. I'm not sure that hook should exist at all. There are so few use cases, you can do your own preprocess if you really want, provide twig functions and what not. But separate issue.

I added the @trigger_error() to template_preprocess(), but I'd suggest we just don't bother with drupal_static_reset()? I also didn't do a change record yet, same story. Do we really need to bother with a change record for something that has zero known usages? IMHO that just adds noise.

Change record for something like this is probably still useful, I haven't reviewed the code yet, but this should is a great clean up.

I reviewed this, one minor comment.

At first I questioned it being on ThemeManager or a helper, but I think it makes sense internal.

I removed the method from ThemeManagerInterface and just add a method exists and marked it as internal.

Makes sense to me.

For now, I didn't add a new public method to reset the variables, I just added it to resetActiveTheme(). Default variables acually depend on the active theme, and for the use case in user logout/login, it doesn't seem like bad idea to reset the active theme, since the active theme can depend on permission.

I think this also makes sense, but I'm not 100% sure. I do think it's a good thing to get rid of drupal_static_reset, worst case if others disagree we can create a new method to reset only these variables.

I also confirmed the search for template_preprocess were all previous to drupal 8 or distributions that had core copied.

This will definitely cause a conflict for the preprocess issue, but this look great.

I'll wait on the CR and give others a chance to review.

Created a CR: https://www.drupal.org/node/3500806, applied the suggestion.

This looks like a really good cleanup to me and I think makes sense on it's own - the CR mentions calling drupal_static_reset with a given key

Can we emit a deprecation error for that key inside drupal_static_reset?

This was discussed in slack, and unless I'm mistaken @larowlan confirmed the deprecation is not necessary because there are very few implementations of that hook and it's highly unlikely that it will need to be called mid request like it does in user, none of the implementations in contrib called drupal_reset_static.

Also this is going into a minor and there will be a CR.

Looks good!

Here is a link to the discussion referenced in 66: https://drupal.slack.com/archives/C079NQPQUEN/p1737757941674099
Also had a quick discussion about how this deprecation is a bit weird cause the values are automatically added. Calling the service is to preserve the current behavior even thought it shouldn't be strictly necessary.

This looks fine and I think we can live without the drupal_static_reset() bc layer given there are no known usages at all.

Committed/pushed to 11.x, thanks!

Way to pick this one up! Thanks everybody for playing the long game!

This is an automated message from the drupal.org issue recommendation system:
If you like this, you will love #3501136: Explicitly register template_preprocess callbacks in hook_theme() and #3495943: Handle module preprocess functions as OOP hooks.

Both me and @nicxvan would love feedback on the concept and implementation in those issues!