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