Provide a mechanism for deprecation of variables used in twig templates

I have a working PoC, coming later today.

OK, this is causing functional tests to fail with:
Fatal error: Can't use method return value in write context in /app/vendor/twig/twig/src/Environment.php(497) : eval()'d code on line 62

I had missed this in my local testing with inline templates. I think I can fix it easily enough using a new type of node instead of a twig function.

I've added an example to the IS.

We're able to trigger_error in preprocess in #3176673: Deprecate non-standard display of title and other base fields because of the complex preparatory work we've done to wrap the deprecated variables in conditionals; some of that preparatory work might not have been necessary if we'd had this. However, the node base field issues are particularly complex and high impact, so I'm not sure how much this would have helped them and it probably won't help to evaluate that more here.

The good news is that the performance impact of this is absolutely negligible. Twig itself is using node visiotrs and custom nodes to do things much more complex than we are adding here; the runtime impact when there are no deprecations is simply testing a single isset($context['deprecations']) statement for every template being rendered.

It's performance impact is trivial AFAICS, as the twig templates get compiled and cached and it's just a simple function call.

But yes, we can make it opt-in in services.yml or settings.php. However, we don't really have any precedent for switching something on for tests. And if we're not confident that developers will switch it on, then we can't be confident the deprecation error will get triggered so it's all a waste of time.

I'm not sure what's best here, but happy to implement if someone can advise.

To elaborate a little more on the performance implications:
for every variable used in a template, TwigExtension::checkDeprecations() gets called once, and if there are no deprecations all that function will need to do is make a single if (isset($context['deprecations'])) {} call.

If 1000 named variables are used in all the templates for a single uncached page, that seems like a drop in the ocean of all the function calls necessary to render that page. With active caches, this seems like a complete non-issue.

On balance, I think the maintainability considerations for having this feature, and not having the complexity of an option for it, are compelling.

Technically I think this can be called RTBC now.

I leave as questions for the committer:

1) Should this get eyes on it from a front-end manager, as it's their world we are trying to provide this functionality for.

2) Has this been adequately reviewed by people who understand twig node visitors? It's a bit of an esoteric subject.

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.

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.

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

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?

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