Provide a mechanism for deprecation of variables used in twig templates

I may be missing something, but why can't we do all of the deprecation logic when actually compiling the templates and trigger the deprecation notices then?

I may be missing something, but why can't we do all of the deprecation logic when actually compiling the templates and trigger the deprecation notices then?

I had to think for a while to remember the answer from when I coded this :) I think it's because the Twig $context (i.e. the render array) is not available at compile time so there is no way then to inform Twig which variables are deprecated.

Your question did make me consider the use of frontmatter here, as that is the only way the template itself could declare a variable deprecated.

A lot depends on who we see as providing the API here and who is consuming it. Are templates providing an API surface called by render arrays, or are render arrays the API being consumed by templates.

It seems to me that we expect templates to be the locus of customisation, and render arrays make information available to templates without requiring them to make use of any particular piece of it. Therefore it's render arrays that need to be able to deprecate pieces of information that templates can no longer rely on. And this is what this patch offers, which frontmatter could not.

I think we should add test coverage of a template that is not using a deprecated variable but one is being passed to it. Maybe we could use state and alter in a suggestion for the theme_test_deprecate theme so it uses a template that is not using {{ foo }} but is using {{ bar }}.

Just update the MR fixing the code standards, @alexpott, it would be nice to have your review here, and thanks to @jonathanjfshaw for the test. Moving back to NR

My bad here, I putted to NR, but #32 was addressed and the tests seems fine, so moving back to RTBC to see what you think.

A few nitpick comments.

OK, so as well as fixing @longwave's MR comments, I also identified and fixed an edge case: if a deprecated variable is overridden inside a template, then the usage of that variable is no longer the usage of a deprecated variable.

However, I think the tests need expanding and refactoring to cover more cases. I will do that now.

Great job with the tests @jonathanshaw!

Moving back to NW, just some nitpicks and I think you've missed this comment from Dave, otherwise it's RTBC for me.

Just reviewed and the MR looks good, #35 and #39 were addressed, all the tests are ok so moving back to RTBC.

Wanted to commit this, but I think we need to add to core documentation somewhere to show how this can be done.

https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Render%21... might be the only obvious place?

It needs to measure effect of the new node visitor, as kill-switch is not needed (#19) some diff in blackfire can help

Having $variables['deprecations'] could have collisions within custom theme functions so I think it's better to introduce new key deprecated besides variables to be inline with https://github.com/twigphp/Twig/blob/3.x/doc/tags/deprecated.rst

$items['custom_template'] = [
    'variables' => [
      'foo' => 'foo',
      'bar' => 'bar',
    ],
    'deprecated' => [
      'foo' => 'foo is deprecated in version X and is removed from Y. Use "bar" instead.',
    ],
  ];

I think it will be cheaper to catch this variables inside of render and substitute variables with some DeprecatedMarkup object which will throw deprecation if used in templates

@jonathanjfshaw re #31 - front-matter will not work here because template could be overriden and nobody going to use it except help topics, see 📌 Supply front matter metadata to preprocess functions Active

Having $variables['deprecations'] could have collisions within custom theme functions

True. Although a very very very edge case.

so I think it's better to introduce new key deprecated besides variables

items['custom_template'] = [
    'variables' => [
      'foo' => 'foo',
      'bar' => 'bar',
    ],
    'deprecated' => [
      'foo' => 'foo is deprecated in version X and is removed from Y. Use "bar" instead.',
    ],
  ];

We'd still have to combine variables and deprecated into a single context array to pass to Twig, so this doesn't prevent a collision with a variable named 'deprecated'.

I think it will be cheaper to catch this variables inside of render and substitute variables with some DeprecatedMarkup object which will throw deprecation if used in templates

It might be cheaper, but it's not backwards compatible as the DeprecatedMarkup object will not be of the same type as the original variable so it will behave differently in Twig logic e.g. {% if foo is same as(false) %}

Added documentation

#31 definitely explains why we can't do this at the template level, because the template doesn't know what's being passed into it. A big reason for introducing this API is for getting rid of cruft from preprocess.

But re-reading it has me thinking: preprocess hooks only get called at runtime, so the deprecation information can only be picked up at runtime if it's in there.

But... what if we added a new hook, hook_deprecated_preprocess_variables($theme_key), and get the variables from that during template compilation, and compare them against what's in the template?

If we did that, there's zero chance of variable collisions, and no runtime overhead either.

what if we added a new hook, hook_deprecated_preprocess_variables($theme_hook), and get the variables from that during template compilation, and compare them against what's in the template?

The NodeVisitor has access to the environment:

  protected function doLeaveNode(Node $node, Environment $env) {
    // At the end of the template, check the used variables are not deprecated.
    if ($node instanceof ModuleNode) {

The ModuleNode has a getTemplateName() method.

So in theory we should be able to add a getDeprecatedVariables($template_name) method to Drupal's TwigEnvironment and do:

  protected function doLeaveNode(Node $node, Environment $env) {
    // At the end of the template, check the used variables are not deprecated.
    if ($node instanceof ModuleNode) {
      $deprecations = $env->getDeprecatedVariableNames($node->getTemplateName());
      foreach ($this->usedNames as $name) {
        if (isset($deprecations[$name])) {
          @trigger_error($deprecations[$name], E_USER_DEPRECATED);
        }
      }
    }

A problem is that in getDeprecatedVariables($template_name) we may well have to reverse engineer the hook names out of the template name. The variables contain the hook names, but the node visitor doesn't have access to the variables. This feels like it could be a maintenance complication.

An alternative approach to remove the hypothetical collision risk would be to keep the current architecture, but fire hook_deprecated_preprocess_variables($theme_hook) from within the TwigContext at runtime, as the context with the variables and so (I think) the hook names is available there. However, this would compound the performance concerns dramatically.

Lastly, with hook_deprecated_preprocess_variables() we'd be doing the deprecation in a seperate place from where the variable is defined or used, which makes the code base harder to understand.

TLDR; I suspect the harms of hook_deprecated_preprocess_variables() outweigh the gains.

How often does Drupal deprecate Twig variables? Don't you think it's over engineering?

How often does Drupal deprecate Twig variables? Don't you think it's over engineering?

The issue is that Drupal's committment to backwards compatability is very strong these days; and because of this, developers come to expect it. But there is simply no other backwards compatabile way to cease to provide twig variables to a template, so we have no way to remove variables and clean up the "preprocess cruft" @catch refers to which can have a history going back 15 years. The more we can modernise and standardise the variables delivered to twig templates, the better frontend DX we can have.

Something like that would be the argument ...

@catch and I discussed the first idea in #49 about having hook_deprecated_preprocess_variables().

The problem with it is that there are any falavours of preprcess hook that can provide variables, in addition to hook_theme. Which means that all of them need to be able to deprecate variables. But the specificity and overriding of all of this has to be respected; more later hooks should be able to 'undo' deprecations. Just because a module provides a variable in hook_theme() and then deprecates it, doesn't mean that a theme shouldn't be able to provide an undeprecated variable with that same name using THEME_preprocess_HOOK() .

To enhance the fun we also have hook_template_preprocess_default_variables_alter we should be respecting which could 'undo' stuff too.

We might end up:
- needing multiple versions of hook_deprecated_preprocess_variables called in sequence
- passing already discovered deprecations as an argument to hook_deprecated_preprocess_variables() so they can be undone
- trying to hack onto the theme registry's preprocess hook discovery mechanism and repurpose it so that we can call all these hook_deprecated_preprocess_variables_HOOK() in the right sequence.

We agreed this seems too complex to be worthwhile.

This is RTBC once rerolled

Removing credit from myself for the reroll and rebase.

catch → credited quietone → .

I re-reviewed this again and I think it looks good.

Nearly committed it then noticed one thing - I've pushed one additional commit to use \array_key_exists() instead of array_key_exists() which allows PHP to use an opcode implementation instead of a function call. Some background in 📌 Eliminate anti-pattern isset($arr[$key]) || array_key_exists($key, $arr) Fixed .

Assuming we get a green test run with that one character change, I'll commit this soon (to 11.x/10.2.x, probably a bit late for 10.1.x now).

Hopefully we can deprecate lots of twig variables in 10.2.x and onwards :)

Committed/pushed to 11.x, thanks!

Updated and published the change record.

Automatically closed - issue fixed for 2 weeks with no activity.