Add a "context" array variable to all theme hooks and "#context" array property to all elements to provide optional contextual data

This issue may be able to sidestep @fabianx's concern with my other approach! :)

#2465399-18: IGNORE: Patch testing issue: The problem is you remove explicitness and discoverability that way ...

Assigning to fabianx to see if maybe this gels better.

+1 to that, I myself have used that in D7 using hook_theme_registry_alter().

There might be even an open core issue by me to introduce a #theme_context ...

My main usage was #theme => 'user_picture' ...

I propose to use #theme_context instead of just #context, because context is a very overloaded word.

#2761525: Accessing the original entity in field templates is hard provided another usecase for this.

Beta evaluations aren't a thing anymore since the beta ended like two years ago. :)

Not sure if this solution would solve my issue, but Im trying to pass some context to my theme from a render array in a module and its impossible. All I want to do is conditionally add a body class depending on if a template file was used or not, without using JS it doesn't seem possible.

@willeaton even though this feature is missing, it should be possible to achieve what you are trying to do. Are you using Drupal Slack? Feel free to ping me on Slack and we can try to figure out how you could add the class.

Thanks @willeaton for providing more context for your specific requirements! While there's ways to work around this, for example, by passing necessary contextual metadata to the static cache, fixing this with the proposed solutions is a lot cleaner.

Here's initial patch which adds the theme context for html and page level. I guess next step would be to figure out how to spread this information for every template. I figured it wouldn't be as simple as passing information while rendering the tree since we now have '#lazy_builders' which makes the rendering happen in multiple trees.

@lauriii, this is a great solution that works perfectly. When rendering a theme array e.g.

return [
      '#theme' => 'MyTemplateFile',
      '#title' => "Page title",
      '#data' => $data,
      '#theme_context' => ['body_classes' => ['has-description', 'filter-bar-open'], 'some_value' => 1]
    ];

... I am able to access all the information from "theme_context" in my myTheme_preprocess_html() and thus apply body classes based on things I've rendered, or values that have come back from an API call. For example:

function myTheme_preprocess_html(&$variables) {

  // Add body classes sent from render arrays
  if(!empty($variables['theme_context'])) {
    if(!empty($variables['theme_context']['body_classes']) {
      foreach($variables['theme_context']['body_classes'] as $body_class)) {
        $variables['attributes']->addClass($body_class);
      }
    }
  }

}

I will test this further and keep you posted on my findings. As discussed in slack, it may be necessary to send this information down the tree too, i.e. we render something which in turn renders something else further down, we may want to have context of who called us in the sub-element.

This will hopefully fix some of the failing tests

This is a "lite" version of #2121317: Introduce the "context" concept for theme system

Considering that issue never really contained any patches, this comment in the IS doesn't really make any sense. "Lite version" of what exactly? I'll just close that issue as a dup of this one and remove this from the IS.

I propose to use #theme_context instead of just #context, because context is a very overloaded word.

Yes, it is an "overloaded word", but that's the whole point. It's arbitrary and could be used for anything by anything. It's really not specific to themes. We're just, currently, using it for that purpose due to a limitation of the conversion between a render array element to theme hook variables. This could and probably will expand to more use cases over time.

The "context" (in this context, pardon the pun) is a reference to the element itself, not specifically the theme system. It should be provided when additional information/data needs to be passed, regardless of who may use it.

Namspacing this variable/property with "theme_" makes little sense when it is intentionally arbitrary in nature.

Example from the search module in core, where a 'context' variable was manually added to allow for this flexibility (#2495419: Move the 'search-results' class from the render array and into a Classy template)

As you can see from the IS, we're already using #context in core and in contrib, we've been using #context for years):

https://drupal-bootstrap.org/api/bootstrap/src%21Bootstrap.php/function/...
https://drupal-bootstrap.org/api/bootstrap/src%21Plugin%21Alter%21Elemen...
https://drupal-bootstrap.org/api/bootstrap/src%21Plugin%21Alter%21ThemeR...

After discussing this with @lauriii in slack, it seems there is confusion as to what this issue actually is.

The above patch/comments suggest a "global" context for the current render stack. That is not what the original intention of this issue was about (nor do I believe that is very maintainable/manageable).

Context is specific to the render array/theme hook/element itself during the creation of said render array.

It does not "bubble up" to other render arrays/theme hooks/elements, nor should it (e.g. $build['#context']['entity'] = $entity; wouldn't make sense all the way at the top, especially if there are 100s of entities on the page, each overwriting that "entity" context).

The issue raised in #15 is actually an entirely different concept and is, yes, quite difficult to achieve (especially when render caching is involved).

The easiest way to accomplish this will be, in fact, using JavaScript. Doing it via Drupal's theming system, properly, would likely require creating custom cache contexts and config (settings). Although, one may be able to get away with utilizing (abusing, really) drupalSettings to do what you want (bubbling up a flag).

---

Anyway, that all being said, I will attempt to get this issue back on track with a new patch (hopefully) later today.

Another use case is #2923634: [PP-1] Use hook_theme_suggestions in views

Hi Mark

The easiest way to accomplish this will be, in fact, using JavaScript

It might be, but in my site we have so much JavaScript that in this rerole we are trying to do away with client side as much as possible, plus it's not efficient, why should the browser on every page load have to detect that an element exists (which could change classes) and add a class to the body tag, when it can be donde once and cached?

In my situation it's a massive limitation that causes me to have to use global variables to communicate between a module and a theme, and the proposed solution is very valid if used carefully, naming standards etc.

Suppose i have an element that might be loaded on a page based on conditions, if it gets rendered i need a class on the body tag to affect other elements on the page because CSS is cascading so can't affect other elements directly. In my opinion we should empower the developers out there (like me) that look at the Drupal rendering system and don't have the first clue of how it works, and by providing this context array it gives freedom to resolve situations that aren't even imagined when creating Drupal, as is it's beauty.

Drupal 8 was supposedly all about context, but the lack of communication between module and theme for the cool idea of decoupled and API first drupal is also limiting when you're building a website.

Can't it at least be considered, even if it has to be moved to its own case?

plus it's not efficient

This is an over-generalized statement and only true if the JS wasn't properly architected. I understand the logic/desire to have the raw HTML generated with the proper classes prior to it reaching the browser. I'm was simply saying that it's far easier to implement this via JS than custom config/settings/cache contexts is all.

why should the browser on every page load have to detect that an element exists (which could change classes) and add a class to the body tag,

Because, apparently, that is the design of the site in question. Personally, I think the decision to "add classes to the body" is an architectural mistake. If only due to the complexities of Drupal's theming system.

It is actually very rare that you need a conditional top-level class added to a page that is affected by something nested deep within the page. Does this single template really affect the entire page?

Can you not simply add said class to the element's #wrapper_attributes? Or, if it's in a view, to the view itself?

Drupal 8 was supposedly all about context...

It is. You just haven't really looked at existing APIs that much yet is all.

Can't it at least be considered, even if it has to be moved to its own case?

Core already has mechanisms in place to accomplish what you're asking to do. Albeit, it is or rather can be somewhat difficult to achieve natively in Drupal, especially if you don't understand how the render system or cache context works.

The only example I can really point to is the Background Image module I recently created. This module needs to add classes to the body when a background image is "dark" or uses the "full viewport" settings.

The only way to, properly, add these classes is to extract these variables from the current background_image entity config (settings):
https://cgit.drupalcode.org/background_image/tree/background_image.modul...

In D7, that would be enough because we didn't have render cache.

However, in D8 and to ensure that the page doesn't cache the first setting value it comes across, cache contexts need to be added to the page.

This requires custom cache contexts to be created to handle the variations of a background_entity that may be on the page:
https://cgit.drupalcode.org/background_image/tree/src/Cache/Context

The concept of a "runtime bubbling global context" sounds great in theory.

However, in practice, it would be a nightmare to implement and maintain... if only due to render caching, lazy loading, placeholders, etc.

The template, if it was already cached, wouldn't be able to add context via said "runtime bubbling global context". So it'd be empty and the page could potentially render without said context.

I'm not saying we wouldn't investigate making all this easier for developers, but it is an entirely separate issue/feature. One that will likely take a while to implement and be quite tricky so that it "just works".

---

I didn't get to creating a patch yesterday, I'll try later this weekend.

Alrighty. Here's a patch that gets this issue back on track. There's no interdiff since previous patches were around a different concept.

I've also included a tests only patch that should fail. The full patch should pass just fine.

This fixes a couple tests that expected a specific default array which has now changed.

Nice! The changes look good for me. Slightly concerned if this will be breaking some tests in contributed projects but these should be easy enough to fix. This type of changes are allowed according to our BC policy, and it does urge people to take this type of scenarios into account.

Let's start drafting a change record for this. We should also add some documentation to the theme.api.php so that it's easier to find this feature.

Ok, I've done the initial pass of the CR here: https://www.drupal.org/node/2971707

I'll get started on making some documentation changes in the patch.

After discussing this with @lauriii in slack, we realized that there's a potential security implication (leaking sensitive information) if we don't remove context from the variables prior to actually rendering it.

There's also a limitation with module and theme alters that prevent us from passing an additional context parameter for callbacks like theme hook suggestions. So we'll have to keep this context in $variables (for now) until we can have properly typed objects (e.g. Variables) where we can set the context on that object before passing it via alters.

Ok, here's a patch that helps lock down context a bit more.

Unfortunately, due to a lack of proper documentation, we cannot simply "append context" to things like preprocess functions because they've been receiving the same three variables for years: $variables, $hook, $info. To prevent BC breaks (in case someone actually determined what has been sent to the functions for the past 5+ years without looking at the API documentation) we'll have to nest this inside of $variables (which is also necessary due to the alter method parameter limit).

So, I went ahead and updated some of that documentation too (when was contextual_links_view() ever used in core?).

I also moved the theme hook context property into its own top-level property (outside of variables), mainly due to the fact that any data in context aren't actual variables to be printed (which is confusing since it's inside $variables of the functions... I know, see paragraph above).

Suffice it to say, most of this will get cleaned up when we have a better theme system in place (e.g. #2869859: [PP-1] Refactor theme hooks/registry into plugin managers) and proper strong typed objects for some of this stuff.

Might help if I actually set the proper context property names from the documentation...

This got buried in recent issue updates, but it could use a proper review.

1. +++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
   @@ -119,18 +119,41 @@ protected function buildInfo($theme_name) {
   +    // Normalize the element info.
   +    $this->normalizeElementInfo($info);
   ...
   +    // Normalize the element info again after the alterations.
   +    $this->normalizeElementInfo($info);
   

   Maybe it is just me, but for me these comments don't really add any information. Maybe we could explain at least, why we need it again after the altering: "Altering might have added additional values".

   Reading the code though I am wondering why the first normalization isn't moved into the previous foreach loop

2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -226,8 +237,23 @@ public function render($hook, array $variables) {
   +    // @todo Create an immutable array object.
   

   Do we have a follow up issue for that? I am wondering whether it would be possible to start in this issue already. It would allow us to not have to worry about BC for example, but also help with documentation etc. I am wondering whether there should be a immutable and mutable version during construction time.

3. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -226,8 +237,23 @@ public function render($hook, array $variables) {
   +    // Clone the variables so they cannot be modified by suggestion hooks.
   +    // @todo Create an immutable array object.
   +    $variables_clone = $variables;
   

   I am wondering: Is this needed as part of this particular issue?

#40.1: Perhaps the comments should just be removed. The method itself describes what it does:

+++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
@@ -119,18 +119,41 @@ protected function buildInfo($theme_name) {
+   * Normalizes element info, ensuring it has the proper default properties.

#40.2: It doesn't have a specific issue created, yet, but it's semi-related to #2972143: Create \Drupal\Component\Utility\ArrayObject.

#40.3: Yes. This is just matching the code to what is documented in the APIs (variables aren't referenced in suggestions build/alter) and should not be alterable (even if &$variables is used). It's related to the tests that exist to ensure context isn't alterable in preprocess... but maybe there should be another test to ensure it's not alterable from suggestions hooks.

Bump. Would appreciate some theme system maintainer reviews here. This is blocking #2923634: [PP-1] Use hook_theme_suggestions in views.

Hi everyone. Found this draft change record.

I want to know if this context can solve my task.

I have basic setup:
Node type -> entity_reference field called field_image -> media entity -> field_image_file

I want to have image field with image linked to node, like classic image field formatted can do(but with media).
Additionally I wand to save all media markup with contextual links, etc. so it can't be done from node's/media/field preprocess.

To solve my task I need to pass (somehow) additional data from node to media entity and it's image formatter.
Maybe this is possible using #context ? If yes, than i can create new entity reference formatter.

Bump... again

Re-roll and expanded comments to address #40.1.

Setting back to RTBC.

Somehow BC support for $variables['theme_hook_original'] was lost along the way.

Added links to some of the @todos to link to #2972143: Create \Drupal\Component\Utility\ArrayObject temporarily until more solidified issues can be made.

@joelpittet found one minor reference to the old $variables['theme_hook_original'].

It's possible in preprocess functions to add any variable you want, but when building render arrays you're limited to what's defined in hook_theme() and _template_preprocess_default_variables().

As part of #2809683: Make it required to specify variables passed to templates and #3016948: Type check theme variables, I'm proposing stricter rules for defining and validating variables passed to templates. I feel like this is going to the opposite direction since this would allow passing new variables in a render array although only under the context variable.

I'm wondering what are the downsides of requiring developers to define the variables they want to pass to the template? This is already possible with hook_theme_registry_alter. Maybe instead of allowing developers to pass variables to templates without defining them, we should make it easier to define new variables?

I feel like this is going to the opposite direction since this would allow passing new variables in a render array although only under the context variable.

It's not. This data is not a variable, it does not reach the template; on purpose.

Per #33, you and I had already discussed that context should not reach the template level due to sechole concerns; which I wholeheartedly agree with.

The entire point of #context is to allow additional, contextual and intentionally arbitrary, data into the theming pipeline.

The purpose of this is to allow easier communication between the back-end and the front-end; only up to the preprocessing layer (keeping it, technically in the back-end, PHP side, of things).

It is up to the developer how to use context; using it to create/populate/override existing variables as they see fit.

This idea isn't anything new and has been implemented in base themes like Drupal Bootstrap since its inception.

Further, this is a step is to declutter the $variables array from providing unnecessary meta information that is simply contextual in nature; information that really doesn't belong in $variables:

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -210,7 +223,8 @@ public function render($hook, array $variables) {
+    // Supply BC support for previous contextual data.
+    // @todo Remove this deprecated passive feature in 9.0.x.
     $variables += [
       'theme_hook_original' => $original_hook,
     ];

@@ -226,8 +240,25 @@ public function render($hook, array $variables) {
+    // Supply default context about the theme hook. Be sure to do this after
+    // any render array context has been merged in.
+    $context['theme_hook'] = $hook;
+    $context['theme_hook_base'] = $base_theme_hook;
+    $context['theme_hook_original'] = $original_hook;

As part of #2809683: Make it required to specify variables passed to templates and #3016948: Type check theme variables, I'm proposing stricter rules for defining and validating variables passed to templates.

While these issues are, indeed necessary in the long run, this is just as important and just one of the baby steps to get there.

Thanks for the input @markcarver! I was confused by the issue summary and didn't recognize this issue as the same we had discussed earlier. We should update the issue summary to match what we are proposing here. I'm still +1 to what we have discussed and what @markcarver described in #54.

1. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -210,7 +223,8 @@ public function render($hook, array $variables) {
   +    // Supply BC support for previous contextual data.
   +    // @todo Remove this deprecated passive feature in 9.0.x.
   

   I don't think we have a process in place how deprecations for theme variables should happen. Let's file a change record for this and ask for feedback from release managers.

2. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -226,8 +240,25 @@ public function render($hook, array $variables) {
   +    $variables['context'] = $context;
   
   @@ -307,6 +346,10 @@ public function render($hook, array $variables) {
   +    unset($variables['context']);
   

   Should we give some warning for anyone who has 'context' variable defined in hook_theme? How about if someone has created preprocess function that adds 'context' variable?

   I think we should create a separate change record warning people specifically about this. It would be great if we could do something more than that though.

#55.1: I don't think we do. It's also very difficult to "trigger" anything since it's only consumed down the theming pipeline. Perhaps this could be/should be officially deprecated once we have an actual Variables(#2972143: Create \Drupal\Component\Utility\ArrayObject) object that we could intercept a magic get method request for this specific key? If so, maybe then we could create a CR for this?

#55.2: I added a note to the existing CR that mentions this property is removed for security reasons. Ultimately, it shouldn't belong in $variables at all, but we really don't have a way to mark this as @internal either.

This is one of the reasons we're trying to move away from arbitrary arrays like this, ironically by introducing a new one, but only because this is just one of the baby steps for a feature that we've really needed for a very, very long time.

We're a long ways off from having dedicated objects for all this stuff though (probably even well into 9.x).

I don't believe this is a very common variable that makes its way to an actual template. In the Drupal Bootstrap base theme, we create this variable, but it's never used on the template level; only to pass contextual data down from modules.

I'd almost be half tempted to maybe mention in the CR that both the #context element property and the variables context property should both be considered @internal and are subject to change.

If we need to reflect this in the documentation of this patch, I'd be happy to change that as well.

Thanks for updating the CR and the issue summary. I still think it would be useful to have specific mention in a change record that we have moved
theme_hook_original under the 'context' variable.

It seems like this change would comply with the Drupal Core frontend BC policy. Given the potential risks and the fact that the policy is quite permissive, it would be useful to get a review from a release manager before committing this.

preprocess implementations should be written in such a way as to account for the change or be prepared to update

This seems to indicate that variables in preprocess functions are not guaranteed at all, by default.

It's a little unclear if this includes preprocess hook signatures (e.g. array of variables vs. a strong-typed Variables object).

Regardless, this sounds like good news.

I've gone ahead and updated the CR to make the change a little clearer and prominent.

Specific data structures are subject to change and counted as @internal.

I don't think we've done any significant introduction of new patterns for data structures in 8.x though, so it's slightly different to introduce something that generally affects all preprocess hooks. This is still doable in a minor release though. Will have to think a bit more if there's anything extra to worry about.

Changing from array to ArrayAccess/ArrayObject is also theoretically doable, except in this case it's a function parameter that can be type hinted to array so I don't really see a clean way to do it (i.e. it might be necessary to introduce a parallel hook or something). Return values are a bit more flexible and we did some of that in 7.x.

Changing from array to ArrayAccess/ArrayObject is also theoretically doable, except in this case it's a function parameter that can be type hinted to array so I don't really see a clean way to do it (i.e. it might be necessary to introduce a parallel hook or something).

Yes. I'm thinking we should just introduce a new method via #2869859: [PP-1] Refactor theme hooks/registry into plugin managers; which would ultimately deprecate preprocess hooks anyway. We'd still pass the variables as an array to the preprocess hooks for BC reasons though.

We could do the same for the context object.

Removing the release manager review tag for now.

Thanks for the review @catch! I have no more concerns on this, and the latest patch and the change record look good for me. Moving to the RTBC queue.

Note for committing, please only publish this change record.

I have a couple of comments/questions as follows

- what if an existing contrib module/custom code defines a theme hook that requires a variable named context - should we be using something a bit more obscure like _context for the key instead?
- should we be using a class constant for the 'context' key instead of a magic string?

#64.1: No, see #58 and #59.

#64.2: I'm not entirely sure what you mean by "magic string"... but unlikely since this will eventually be moved to a dedicated object.

I think #64.2 is referring to providing the variable key with a constant, and changing 'context' into something more random to avoid collisions. I'm not sure if there would be much value in having the constant by itself, but since this would essentially solve #64.1 as well, it might make sense.

This entire issue is based on years of contrib using this parameter in $variables for this exact purpose.

If there is anyone using this, it's likely because they're already using it by a base theme that already provides this exact functionality.

That isn't this hypothetical "collision" though. Which, I might remind everyone, is still permissible by the Drupal Core frontend BC policy.

This issue is/has always been intended to be an upstream-port of a very useful contrib feature that needs to make its way into core.

So #64.2 is $variables[SomeObject::CONTEXT]? No, I don't think sticking this behind some random class constant is the wisest decision at this point.

Especially given that this is still just the first baby step in this process. Things are bound to change and I'd rather not introduce a constant that will likely be deprecated and replaced with another down the line when things solidify more.

This issue/patch has tests that check for this parameter; I think that's quite sufficient at this point.

I agree with #67 and confirm RTBC. Great and important work, thank you very much!

Patch in #52 doesn't apply to 8.6.x.

For folks needing to apply it with current stable version, here is a rerolled patch for 8.6.7 only.

Hiding patch in #69 from IS, so it doesn't generate unnecessary confusion.

Bump...

Discussed with @larowlan on Slack and we agreed that it wouldn't make sense to provide a constant for this variable, even though it could bring some benefits. It would be a new pattern for the theme system, and given the direction we're trying to take in the theme system, it could end up being the only place where constants are being used for theme variable key names.

I retitled the change record to be even clearer about #context not being available in templates.

1. +++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
   @@ -119,18 +119,42 @@ protected function buildInfo($theme_name) {
   +      // Ensure the element's #type is set.
   +      if (!isset($info[$type]['#type'])) {
   +        $info[$type]['#type'] = $type;
   +      }
   +
   +      // Ensure there is a default #context array.
   +      if (!isset($info[$type]['#context'])) {
   +        $info[$type]['#context'] = [];
   +      }
   

   we could just write this as

   $info[$type] += [
     '#context' => [],
     '#type' => $type,
   ];
   

   and could even replace the foreach with array_map

2. +++ b/core/lib/Drupal/Core/Theme/Registry.php
   @@ -372,6 +372,11 @@ protected function build() {
   +      // Ensure every theme hook as a default context array.
   

   as » has

   Again we can use += here to simplify the code

3. +++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
   @@ -210,7 +223,8 @@ public function render($hook, array $variables) {
   +    // @todo Remove this deprecated passive feature in 9.0.x.
   

   We need an issue for this - as well as a way of signalling to someone that they're using the deprecated path, which I can't see happening without #2972143: Create \Drupal\Component\Utility\ArrayObject so probably just an issue at this stage

4. +++ b/core/themes/engines/twig/twig.engine
   @@ -47,11 +47,22 @@ function twig_init(Extension $theme) {
   +function twig_render_template($template_file, array $variables, array $context = []) {
   

   do we need to trigger a deprecation error here for folks who invoke this without the third argument, so we can enforce the context argument in D9?

5. +++ b/core/themes/engines/twig/twig.engine
   @@ -75,16 +86,16 @@ function twig_render_template($template_file, array $variables) {
   -    $output['debug_prefix'] .= "\n<!-- THEME HOOK: '" . Html::escape($variables['theme_hook_original']) . "' -->";
   +    $output['debug_prefix'] .= "\n<!-- THEME HOOK: '" . Html::escape($context['theme_hook_original']) . "' -->";
   ...
   +    if (strpos($context['theme_hook_original'], '__') !== FALSE) {
   +      $derived_suggestions[] = $hook = $context['theme_hook_original'];
   
   @@ -92,17 +103,17 @@ function twig_render_template($template_file, array $variables) {
   +      $context['suggestions'] = array_merge($derived_suggestions, $context['suggestions']);
   ...
   +      if (strpos($context['theme_hook_original'], '__') === FALSE) {
   

   $context is optional, we can't rely on it here. This therefore points to us needing to trigger an error and construct a psuedo context value from the old $variables if $context is not passed

edit: removed an item I found later in the patch but forgot to remove when I posted

error: patch failed: core/lib/Drupal/Core/Render/theme.api.php:707
error: core/lib/Drupal/Core/Render/theme.api.php: patch does not apply
make: *** [patch88] Error 1

edit: @larowlan, just an FYI, I was able to apply #52 to 8.8.x using both patch -p1 and git apply --index (some slight offset, but still succeeded). I think maybe you were trying to apply #69, which is for 8.6.x?

• #75.1: Done, however unable to use array_map because we need the $type key from the array which array_map doesn't let us do.
• #75.2: Done
• #75.3: Done, created #3051608: Deprecate contextual based variable parameters and #3051609: Remove contextual based variable parameters
• #75.4 and #75.5: Done, created #3051613: Require $context parameter in twig_render_template()

I also went ahead and updated the documentation and example code for hook_preprocess() to match the change record (and vice versa).

Oops, been a while since I've worked on this. Forgot $variables['theme_hook_suggestions'] already existed.

That last patch will fail because TwigWhiteListTest manually invokes twig_render_template().

Here's a new patch that fixes that and simplifies twig_render_template() a little bit more.

Sigh, the last patch will still fail because it doesn't actually check if the parameter was passed, just if it's empty.

This will allow an empty context array, which technically doesn't affect the output of the template... but also won't print any debugging information since that requires contextual information about the theme hook invoked. This can be purposefully omitted if invoked manually and not ran through the theme system (like TwigWhiteListTest is doing).

Ping...

Using this patch (#83) in combination with #2923634: [PP-1] Use hook_theme_suggestions in views for a while on several sites.
It really helps a front-end developer to do his job :)

++

Can't wait to be able to use this.

+++ b/core/lib/Drupal/Core/Render/ElementInfoManager.php
@@ -119,18 +119,37 @@ protected function buildInfo($theme_name) {
+    $this->normalizeElementInfo($info);
...
+    $this->normalizeElementInfo($info);

Why can't we just do this only after the alterations?

Why can't we just do this only after the alterations?

Because alter hooks will need normalized elements to actually alter.

---

@larowlan, can this go back to RTBC?

Bump

Bump.... again..............

I did some research just to make sure that this wouldn't cause regressions for contrib modules or themes. Luckily I found only one instance where context variable is being printed in a template. We should open an issue to fix that in the module.

I also realized that we are using #context for passing variables to inline templates. This patch makes the behavior inconsistent, since in that instance #context is specifically used for passing variables to templates, and here we want to specifically avoid passing that variable to templates.

I also realized that we are using #context for passing variables to inline templates. This patch makes the behavior inconsistent, since in that instance #context is specifically used for passing variables to templates, and here we want to specifically avoid passing that variable to templates.

No, #context in the case of inline templates is prerendered to #markup: InlineTemplate::preRenderInlineTemplate(); it never reaches a preprocess hook/template because it's... inline.

We could rename it to #variables and deprecate #context for inline templates... but at this point, I think just adding a note to the CR will suffice:

Inline templates will continue to follow Twig nomenclature and use #context for passing variables to the inline template. While the render array property is similarly named, it is not affected by this change because inline templates are prerendered into markup and are never preprocessed or passed to an actual template.

edit: added note to CR

I did some research just to make sure that this wouldn't cause regressions for contrib modules or themes.

Again, to remind everyone, this change is permissible per Drupal Core frontend BC policy.

It's not like there isn't a workaround to this change either. The benefits of adding this certainly outweigh the relatively minor cost of anyone having to do a simple variable rename for any contrib/custom code actually affected.

We should open an issue to fix that in the module.

Regardless, I went ahead an opened an issue: #3082665: Use container with theme hook suggestions and attributes instead of custom template and variables

As a side note, ^ is a byproduct of poor design.

@markcarver
Hi

#40.3: Yes. This is just matching the code to what is documented in the APIs (variables aren't referenced in suggestions build/alter) and should not be alterable (even if &$variables is used). It's related to the tests that exist to ensure context isn't alterable in preprocess... but maybe there should be another test to ensure it's not alterable from suggestions hooks.

Could you explain then, how we can add a special suggestion for the element what depends on parent element?
Basically if we pass $variables (not a clone) by reference, then we have a chance add parents' suggestions, etc.
and, now, after applying the patch we don't have that possibility because of:

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -226,8 +237,23 @@ public function render($hook, array $variables) {
+    // Clone the variables so they cannot be modified by suggestion hooks.
+    // @todo Create an immutable array object.
+    $variables_clone = $variables;

Hi,

Any chance this patch could be made available for core version 8.9?

Working on 8.9.0.
Though I don't do testing so not sure if my modifications where right.
Interdiff only shows the .rej modifications on
core/modules/system/tests/modules/theme_test/theme_test.module
and
core/tests/Drupal/Tests/Core/Theme/RegistryTest.php

Not tested, do not apply on production :)

Edit: the patch is for 8.9.x, my bad in setting up test.

#99 can't be applied on Drupal9.0.1, needs reroll

Rerolling patch for 9.1.x

Added patch for 9.1.x

This patch should fix some of the test failures. There is still 1 left. I haven't had the time to check the issue completely:

Drupal\Tests\help_topics\Functional\HelpTopicSearchTest::testHelpSearch is failing because context is no longer added to the template item-list--search-result.html.twig and context.plugin is added as a class to the search result list.

{%
  set classes = [
    'search-results',
    context.plugin ~ '-results',
  ]
%}
{% set attributes = attributes.addClass(classes) %}

Drupal\Tests\help_topics\Functional\HelpTopicSearchTest::assertSearchResultsCount uses context.plugin ~ '-results' to check the nr of results. Since that class is no longer present, this will always fail.

Since patch #83 on Drupal 8.8.x was the last patch to pass all tests, I took that patch and then rebased it onto 9.2.x and confirmed that the differences between my merge conflict resolutions were trivially similar to the patch in #105. So I took the patch in #105 and applied it to the Merge Request branch and then applied my rebased #83 on top of it. You can see any interdiff by examining the commits on the MR.

Lastly, I think I fixed the broken tests. Let's see what the testbot says.

I should note that we really have broken the context.plugin ~ '-results' class added by Classy and its children. However, no core theme is using any class ending in _search-results. The previously broken test was looking for the help_search-results class, but I changed that to the class that still exists, search-results.

If I'm reading Drupal Core frontend BC policy correctly, we'll need to fix that.

The stable base themes (Stable, Classy) are considered public API, with stable templates, markup, and CSS, so themes needing BC support should extend one of those base themes.

So, if I'm understanding this new context correctly, if I wanted to change a CSS class based on a context, I'd need to do it in a preprocess function. And since class names need to live in a .twig file (best practice), I'd need to have the preprocess function take the context and create a new variable to use in a twig file. Right?

Given that's the preferred way to use context, I went ahead and fixed the search plugin context by creating a new variable based on context.plugin.

I also noticed that the item-list.html.twig templates have a context.list_style variable which is broken. I'll fix that in the morning since it's late at night here.

I reverted the changes to the stable9 theme. I believe we're not supposed to change the Twig markup in that theme. Also, I noticed that removing the $variables['context'] in the MR meant we were breaking any item-list.html.twig not in core. I've added the 2 context variables used in those templates to an allow list for the remainder of 9.x.

After thinking about the next steps after this MR, I created #3190462: Replace $hook/$info parameters with render context in preprocess/theme_suggestions hook to discuss whether we should deprecate the $hook and $info parameters in favor of making this new $context the last parameter for preprocess/theme_suggestions. It seems logical to me; please join in that discussion to pipe in.

In the meantime, I'm going to add $theme_info to $context in this MR.

Uploading patch file of the latest merge request against latest stable (9.2).

9.3.0-alpha1 was released. Tthat means feature requests have to be applied to 9.4.x.

Patch #115 failed to applies on 9.3.x.

Re-roll to #105

@phoang since 9.3 is already released, JohnAlbin set the ticket to 9.4.x-dev.

The issue status is reflecting the merge request that's now at 9.4.x-dev. Not a previous stable version.
When this is needed specially for 9.3 it could be back ported... but seeing the ticket history, change and impact; it probably isn't.

I pushed a commit that fixes the merge conflict with 9.4.x's theme.api.php file.

Someone else (besides phoang) asked how to apply this changeset to 9.3.x. The only place where this fails to apply to 9.3.x is the theme.api.php file. But to make this easier for others to use on 9.3.x, I've created a patch file by taking the 9.4.x diff and removing the theme.api.php and test files changes. You can't use this patch file to test and review this issue though. Please use the MR for testing and reviewing.

it's not mergeable to 9.5.x anymore, sorry

This is a patch against current stable version 9.4.0

Adding bug smash tag for targeting!

Thanks for the work on this everybody. This would be super useful. It's one of those things you just assume you can do. Then when you really need to do it and learn you can't, it's quite frustrating.

Re-rolled patch #124 to support Drupal 10 latest version(10.1.1) and below themes twig changes removed because those themes not support for Drupal 10,

bartik, classy, seven, stable.

Could someone perhaps give an update on the plan here?

Since @markhalliwell's Bio says:

I am no longer an active part of this community and not working on any Drupal projects.

there seems to be not much movement here any more and I think for Drupal 11 it would be worth a review and discussion, if this still makes sense as improvement to Core.
If yes, we should take it over the finish line. If not, we should point out, why this plan is deprecated.

Does anyone know?

Patch for 10.2.x based on #129

Patch #131 works fine in Drupal 10.2.2 with PHP 8.2.

Fixed phpstan
Please review

Please note that such changes could cause issue with third-party modules, for example ui_patterns.

This last module define a #context object, which is overridden if we apply the patch. This leads to a fatal error.

So maybe it could be better to override or extends the #context element if it is empty or array, otherwise the variable should be kept as this.

An issue has been filled for the UI Patterns module: https://www.drupal.org/project/ui_patterns/issues/3422997