Problem/Motivation

As detailed in the description for #1843798: [meta] Refactor Render API to be OO, we currently do not have a drillable, accessible way of getting to variables that a themer may anticipate being available to a parent template. This is because variables in a sub-element may be invoked by a preprocessor that runs later.

Example from a node template:

  <!-- First image, with manual tag creation -->
  {{ hide(content.field_image.0) }}
  <img class="banner" src="{{ content.field_image.0.attrs.src }}" alt="{{ content.field_image.0.attrs.alt }}" />
  <!-- Remaining content -->
  {{ content }}
  <!-- Links -->
  {{ links }}
  <!-- Comments -->
  {{ comments }}

Before the larger, long-term goals of #1843798: [meta] Refactor Render API to be OO are achieved, we'd like to get a stopgap for better drillability support in Drupal 8, at least for the Twig engine.

Proposed resolution

Provide a way for Twig to understand which variables need to be available to a template and adjust how they are prepared beforehand. Twig's compilation steps may offer us some insight into implementation.

Remaining tasks

User interface changes

None

API changes

Improved Twig syntax from what currently exists (that requires manual prep/rendering).

Files: 
CommentFileSizeAuthor
#21 drupal-twig-drillable--2008450-20.patch3.8 KBsteveoliver
PASSED: [[SimpleTest]]: [MySQL] 56,567 pass(es).
[ View ]
#19 drupal-twig-drillable--2008450-19.patch3.74 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-twig-drillable--2008450-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#15 2008450-15--api.patch19.58 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 56,393 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#15 2008450-15--api-implementation.patch21.22 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 56,731 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#11 drupal-twig-drillable--2008450-8-11-interdiff.patch5.74 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-twig-drillable--2008450-8-11-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#11 drupal-twig-drillable--2008450-11.patch15.37 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 56,393 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#8 drupal-twig-drillable--2008450-8.patch12.03 KBsteveoliver
FAILED: [[SimpleTest]]: [MySQL] 57,733 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Comments

Just one idea:

Background: render elements can already have #pre_render callbacks. These are called by drupal_render() prior to it calling theme(), whereas preprocess functions (and likely their hook equivalents proposed in #2004872: [meta] Theme system architecture changes) are only called from within theme().

Idea: we make it so that drilling within Twig results in #pre_render being called on each drilled element. And, wherever we currently have child render elements being made/altered within preprocess functions, we move that into #pre_render callbacks, reserving preprocess functions (and everything being proposed in #2004872: [meta] Theme system architecture changes) to only the code that's needed for rendering the element by its own template.

Example from the issue summary: because the .src of an image is wanted during drilling from a parent element's template, we should not be setting it in theme_image() or template_preprocess_image(), but instead, move that to a #pre_render of an image render array.

Potential problem with this idea: is it clear what's needed during drilling vs. what isn't? If not, then potentially everything done in preprocess would need to move to #pre_render, and if that's the case, we should rethink whether it even makes sense to keep those as separate concepts.

+1 to #1. It's pretty much exactly the summary of some discussions I've been having in IRC with a few other people as it certainly looks like the path of least resistance for now (can avoid full OO or other sweeping API changes).

drupal_render() should be responsible for making renderable arrays drillable simply because not all renderable arrays are passed to theme() but if/when #2006152: [meta] Don't call theme() directly anywhere outside drupal_render() is cleaned up all renderable arrays will be passed to drupal_render().

I'd also like to bring up (and I'll make an issue for this) that as great as #pre_render (or equivalent is) for helping drillability is how great the current #post_render is at preventing drillability so it needs to be removed.

I'd also like to provide a quick high level summary of the minimum prerequisites for "Twig drillability" (as I understand it) so that hopefully more people can get involved in the discussion.

Basically, if you have user input ($input) and you want to get a string back ($output) there's at least two things that need to happen:

1. Modules need the ability to modify $input -> sanitising bits of it, altering the content, etc... to create a mid-way point between $input and $output ($variables)
2. The modified input needs to be arranged in some way to create a string with the right structure for HTML -> theme functions/templates

If Twig is to "drill" something then it needs to perform step 1 on some part of some structured data and end up with the $variables (the processed structured data) and not to also end up with the result of step 2 as Twig will want to do it's own conversion of what it receives into a string as part of processing the template.

The problem, as @effulgentsia pointed out is that we don't currently have just one phase for processing, we have like 3 or 4 and one even comes after the input is a string (as I said in my previous comment) so we have to start thinking about building new structures -> #2004872: [meta] Theme system architecture changes and/or start redefining "best practice" for our existing frameworks.

The #pre_render idea sounds potentially promising.

Another idea I had:

This issue, in my view, minimally just has to pertain to the Twig engine, so perhaps there are ways of extending Twig such that it would be able to handle this better.

One idea I had is that the "." character in Twig syntax seems to represent potentially some accessor method on a Twig data object, and if this mapped to a __call() or __get() that we could extend. This could potentially lookup the theme registry of the child element if it is a render array sub-component and run the preprocessor callbacks.

So, in writing {{ content.field_image.src }}, we would check to see that field_image is a render array, run any relevant pre-processors that it potentially invokes, and pull these $vars to be potentially accessed by its own accessor.

This is just theory, I don't actually know if we can extend magic methods on Twig's objects in this way or what challenges this poses.

#5 - That sounds cool too. I guess we need to start rolling a few proof-of-concept patches soon to move the discussion along.

Issue summary:View changes

Updated issue summary.

Status:Active» Needs review
Issue tags:+Twig
StatusFileSize
new12.03 KB
FAILED: [[SimpleTest]]: [MySQL] 57,733 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

In our Hangout last week we discussed an option basically along the lines of c4rl's #4, where:

a. Given child render elements in a parent template which have not yet been prepared,
b. When trying to access variables of those children which don't exist because _preprocess+ has not yet run,
c. Twig, in TwigTemplate::getAttribute(), should run that item through it's "preparation" stage(s),
d. Making it's variables available to the parent template.

Regardless of implementation (I will let Fabianx chime in on this), the attached patch includes a test for the use case we have been describing.

+++ b/core/themes/test_drillable/node.html.twig
@@ -0,0 +1,134 @@
+    {#
+      Test that we can drill into variables of child elements.
+    #}
+    {% if content.field_image.0 is not empty %}
+
+      {#{{ dump(content.field_image.0) }}#}
+      {#{{ hide(content.field_image.0) }}#}
+
+      {#
+        When we access attributes from a field instance below, Twig should:
+          1. Realize the requested attributes do not exist.
+          2. Realize the field instance is still a render array.
+          3. Pre-render the field instance, returning the prepared variables.
+        When we print {{ content }}, field instances should be rendered with
+          render() as usual.
+      #}
+
+      <img class="layout-first-image" src="{{ content.field_image.0.attributes.src }}" />

We may likely end up needing to drill through the field instances themselves, which include wrapper divs, etc.

Implementation may need to be something more like:

content.field_image.0.item.attributes.src

...where this variable preparation magic happens twice--once on the field and a second time on the field item.

Status:Needs review» Needs work

The last submitted patch, drupal-twig-drillable--2008450-8.patch, failed testing.

StatusFileSize
new15.37 KB
FAILED: [[SimpleTest]]: [MySQL] 56,393 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-twig-drillable--2008450-8-11-interdiff.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Cleanup and added one more test for #type link child elements.

Priority:Critical» Major

This is really really important, but not utterly critical. Also, per https://groups.drupal.org/node/298298 Twig isn't supposed to have critical follow-ups.
Demoting per discussion in IRC.

Issue summary:View changes

changed Remaining tasks to write tests after use-cases are decided.

For reference: The IRC chat between Fabianx and I on 2013/06/18:

...
Fabianx: steveoliver: So implementation is:
Fabianx: ( proof-of-concept)
Fabianx: * Add a new parameter to render and theme.
Fabianx: Call it $render = TRUE
Fabianx: If $render == FALSE, call theme(..., $render), but do not run the actual theme function but return the $variables array.
Fabianx: Add some code in the d_render() for the case that $render == FALSE and replace the internal structure of the processed element with $variables and save the original in #original.
Fabianx: Do not call post_process or render_children if $render == FALSE.
Fabianx: or drupal_wrapper
Fabianx: or #prefix or #suffix.
Fabianx: That is all ignored for drillability.
steveoliver: …sure
Fabianx: Then when you are able to get a structure from render()
Fabianx: Like:
steveoliver: i get that part..
steveoliver: so up to the determination point for $render = TRUE/FALSE...
steveoliver: you still see that happening on TwigTemplate::getAttribute()
steveoliver: ?
Fabianx: $vars = render( array( '#theme' => 'x', ...), FALSE);
steveoliver: but why not create a separate function, i..e dont_render()...
steveoliver: and leave render alone...
Fabianx: steveoliver: That would be rather slow and its really only needed for render arrays.
Fabianx: steveoliver: That is fine.
steveoliver: sure
Fabianx: steveoliver: Just for theme() it probably makes sense (for a POC) to not copy all code ...
steveoliver: i'm cool with that stuff...
Fabianx: steveoliver: For #types I would say they should define a "drillable_function" as part of their type definition.
Fabianx: For themes use the $variables.
steveoliver: *thinks on that....*
steveoliver: well wouldn't the drillable function just be preprocess?
Fabianx: steveoliver: ( would at least solve it for links for now )
Fabianx: steveoliver: yes
Fabianx: steveoliver: but _especially_ the types we want drillable.
Fabianx: link as the prime example.
Fabianx: steveoliver: So the thing to pass the tests then is to use the TwigReference::__get function.
Fabianx: because all render arrays (at least for printing) are passing through there.
Fabianx: So that is ideal to intercept there.
steveoliver: ok.  jumping back a second: re: *suggestions, *suggestions_alter, *prepare, *prepare_alter…, i doubt that will happen… what do you think?
steveoliver: i wondered about it because calling prepare and prepare_alter would seem the only things needed in our 'dont_render' function ...
Jennifer Lampton: steveoliver: suggestions + suggestions alter can happen pretty easily
Jennifer Lampton: prepare might have to remain preprocess for D8
steveoliver: jenlampton: cool
Jennifer Lampton: sadly 
Jennifer Lampton: I think we'd need a new meta that was "split preprocess into prepare and prepare alter" with a critical follow up to remove preprocess
Jennifer Lampton: we just need to determine how crucial that is for everything else we need to do
Jennifer Lampton: but since there's been no momentum there yet
Jennifer Lampton: *shrugs*
steveoliver: yeah, but the bug needs fixed
steveoliver: Le Bug
steveoliver: specific preprocess functions for theme_suggestions are not called...

Since then..

  1. I implemented TwigReference::__get(), but it was never called for the unprepared properties of the first link (test) at /twig-theme-test/drillable-elements.
  2. I looked in TwigTemplate::getContextReference() for an opportunity to check for unprepared properties, but came to the conclusion this probably is not the place.
  3. So far, I've been testing with this dummy return array, just trying to find and confirm the proper trigger point:

  public function __get($name) {
    return array(
      'title' => 'Title for ' . $name,
      'attributes' => array(
        'href' => 'HREF',
        'class' => 'foo bar'
      )
    );
  }

c4rl, Fabianx, others: If you could have a look around, you might see something I'm missing. I'll keep digging around in the meantime.

Assigned:Unassigned» steveoliver

nm, got it. __get() needed __isset() implemented.

1. Via __isset(), how do we determine which unavailable properties we'll 'prepare' for?
2. Via __get(), how do we 'prepare' and return the variables?

Status:Needs work» Needs review
StatusFileSize
new21.22 KB
FAILED: [[SimpleTest]]: [MySQL] 56,731 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new19.58 KB
FAILED: [[SimpleTest]]: [MySQL] 56,393 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

I see no way to hack drillability out of drupal_render() or theme(). I think we should introduce drillability via new '#drill_callback' properties in hook_theme() and hook_element_info().

The api-implementation.patch provides drillability into the image field(s) of /core/themes/test_drillable/node.html.twig.

The tests are now broken, I suspect because $node->get('field_image', 0)->entity->uri does not return a string but an Object. Something in the Entity/Field system must have changed recently, but I couldn't find a change notice that helped.

a few more notes:
- support theme hook suggestions/fallbacks in drupal_drill_element_callback()
- maybe put this logic in a theme_hook_is_implemented()
- we may prefer theme registry as a service instead of these theme_get_registry() calls

Status:Needs review» Needs work

I have a couple of comments on #15:

- Theme suggestions will fail to be discovered in drupal_drill_element_callback() and array syntax theme suggestions will probably even throw errors.
- I think it is important to support lists of multiple #drill_callback functions rather than only allow a single callback per element - even the current #pre_render takes an array of functions to run
- #type and #theme are very commonly set at the same time for render arrays, so if element_info() and hook_theme() both return a #drill_callback we shouldn't ignore one of them. Rather than elseif, maybe we should merge #type and #theme callbacks into a single list of callbacks to run (see the point above about supporting this).

I don't see how #drill_callback scales. Example: You have a module that implements a preprocess (i.e. the new "hook_theme_prepare" described in #2004872: [meta] Theme system architecture changes) that adds new parameters to the variables array. How would the original #drill_callback accommodate these?

Status:Needs work» Needs review
StatusFileSize
new3.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-twig-drillable--2008450-19.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Yep, since all it is, or should be, is implementations of preprocess, I think we can just run those preprocess phases for elements that implement them; If preprocess is not implemented, variables are not available.

The attached patch adds a third parameter to theme() that Twig, via drupal_render_prepare(), tells theme() to return the prepared $variables array rather than $output (HTML).

It works, and is about as simple as it gets.

Implementation patch to follow.

Status:Needs review» Needs work

The last submitted patch, drupal-twig-drillable--2008450-19.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new3.8 KB
PASSED: [[SimpleTest]]: [MySQL] 56,567 pass(es).
[ View ]

OK, fine... A patch that applies.

Fabianx: steveoliver: nice
Fabianx: steveoliver: the rendered output should be cached "somewhere".
Fabianx: steveoliver: in general the best would probably be to:
Fabianx: a) replace the contents of "this" ( using exchangeArray ) with the new variables and save the original via ->getReference() in #original.
Fabianx: IIRC, #theme should still be set, so then adding a key like: #variables_prepared = TRUE, #theme_hook = 'x' (all data prepared by theme) and then having a check in theme() if things have been already prepared is all that is left here.

Status:Needs review» Needs work

+  if (isset($element['#type']) && !isset($element['#theme'])) {
+    $element += element_info($element['#type']);
+  }

I don't understand why we aren't loading the defaults for type if #theme is set. This is new behaviour that I haven't seen elsewhere.

I don't immediately see how this patch handles drillability for elements that follow a #type -> #pre_render -> #markup approach that do important sanitisation work in #pre_render and never hit theme()?

Also, the patch in #21 doesn't seem to deal with #theme_wrappers arrays either?

I'd also love to see some more discussion over at #2009132: Remove #pre_render and #post_render from drupal_render() as it prevents proper Twig drillability and I thought I'd link to #2025259: drupal_render() should preserve the original render element in #original so pre and post render functions can access "raw" input as it would be useful for people actually trying to write #drill_callback functions :)

ah, also, I thought about this a bit more... Does this work with the 'render element' property from hook_theme()?

Priority:Major» Normal

So I had a more detailed look at this yesterday. Some observations:

  • This isn't anything that we *had* in D7, so it's not a regression from a DX perspective. I needed to remind myself of this.
  • {{ content.field_picture }} actually works, as does {{ content.field_picture.0 }} (these are just calls to render). So yay.
  • This highly depends on all the patches for #1757550: [META-63] Convert core theme functions to Twig templates getting in, since $variables passed by reference in preprocessors are really what we're after (and extractable); the old-style theme_ functions' local variables aren't anything we'd have access to. Therefore, whatever patch comes out of this issue will depend explicitly on default 'variables' parameter in hook_theme() and subsequent (optional) preprocessor.

Because of these things, I'm going to make the priority normal since there are so many other things we'd like to have in D8. Given the current API freeze, we may end up just focusing on #1843798: [meta] Refactor Render API to be OO as the long-term solution after addressing other majors/criticals.

Related, #2044105: Introduce #alters for drupal_render() would make it easier to drill elements rendered outside #theme.

I created #2047263: Provide inline_template tag within Twig, which will take care of the following use-case:

* Drillabillity with support for #post_render, #theme_wrappers and #cache.
* Essentially inline templates.
* It is actionable now and not even that complicated.

The use case for this issue then remains to "just" access the variables and print them out.

The same approach from https://drupal.org/node/2047263 (using #render_function) can be used here to provide a nice twig function:

{% set render_vars = get_render_vars(render_array) %}

Once #2047263: Provide inline_template tag within Twig is in this is no longer even complicated, though it is not yet automatic drillability, it is another step forward and could be theoretically even called from phptemplate templates.

Issue summary:View changes

minor grammar