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
- #2009132: Remove #pre_render and #post_render from drupal_render() as it prevents proper Twig drillability
- Determine base use-cases
- Write tests based on use-cases.
- Determine potential implementations with respect to Twig
User interface changes
None
API changes
Improved Twig syntax from what currently exists (that requires manual prep/rendering).
Related Issues
Comment | File | Size | Author |
---|---|---|---|
#21 | drupal-twig-drillable--2008450-20.patch | 3.8 KB | steveoliver |
#19 | drupal-twig-drillable--2008450-19.patch | 3.74 KB | steveoliver |
#15 | 2008450-15--api.patch | 19.58 KB | steveoliver |
#15 | 2008450-15--api-implementation.patch | 21.22 KB | steveoliver |
#11 | drupal-twig-drillable--2008450-8-11-interdiff.patch | 5.74 KB | steveoliver |
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedJust 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.
Comment #2
thedavidmeister CreditAttribution: thedavidmeister commented+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.
Comment #3
thedavidmeister CreditAttribution: thedavidmeister commentedI'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.
Comment #4
thedavidmeister CreditAttribution: thedavidmeister commented#2009132: Remove #pre_render and #post_render from drupal_render() as it prevents proper Twig drillability
Comment #5
c4rl CreditAttribution: c4rl commentedThe #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.
Comment #6
thedavidmeister CreditAttribution: thedavidmeister commented#5 - That sounds cool too. I guess we need to start rolling a few proof-of-concept patches soon to move the discussion along.
Comment #7
thedavidmeister CreditAttribution: thedavidmeister commentedRelated #1818266: [meta] A secure theme system (with twig)
Comment #7.0
thedavidmeister CreditAttribution: thedavidmeister commentedUpdated issue summary.
Comment #8
steveoliver CreditAttribution: steveoliver commentedIn 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.
Comment #9
steveoliver CreditAttribution: steveoliver commentedWe 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.
Comment #11
steveoliver CreditAttribution: steveoliver commentedCleanup and added one more test for #type link child elements.
Comment #12
tim.plunkettThis 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.
Comment #12.0
tim.plunkettchanged Remaining tasks to write tests after use-cases are decided.
Comment #13
steveoliver CreditAttribution: steveoliver commentedFor reference: The IRC chat between Fabianx and I on 2013/06/18:
Since then..
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.
Comment #14
steveoliver CreditAttribution: steveoliver commentednm, 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?
Comment #15
steveoliver CreditAttribution: steveoliver commentedI see no way to hack drillability out of drupal_render() or theme(). I think we should introduce drillability via new '
#drill_callback
' properties inhook_theme()
andhook_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.Comment #16
steveoliver CreditAttribution: steveoliver commenteda 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()
callsComment #17
thedavidmeister CreditAttribution: thedavidmeister commentedI 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).
Comment #18
c4rl CreditAttribution: c4rl commentedI 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?
Comment #19
steveoliver CreditAttribution: steveoliver commentedYep, 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, viadrupal_render_prepare()
, tellstheme()
to return the prepared$variables
array rather than $output (HTML).It works, and is about as simple as it gets.
Implementation patch to follow.
Comment #21
steveoliver CreditAttribution: steveoliver commentedOK, fine... A patch that applies.
Comment #22
Fabianx CreditAttribution: Fabianx commentedFabianx: 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.
Comment #23
thedavidmeister CreditAttribution: thedavidmeister commentedI 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()?
Comment #24
thedavidmeister CreditAttribution: thedavidmeister commentedAlso, the patch in #21 doesn't seem to deal with #theme_wrappers arrays either?
Comment #25
thedavidmeister CreditAttribution: thedavidmeister commentedI'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 :)
Comment #26
thedavidmeister CreditAttribution: thedavidmeister commentedah, also, I thought about this a bit more... Does this work with the 'render element' property from hook_theme()?
Comment #27
c4rl CreditAttribution: c4rl commentedSo I had a more detailed look at this yesterday. Some observations:
{{ content.field_picture }}
actually works, as does{{ content.field_picture.0 }}
(these are just calls to render). So yay.$variables
passed by reference in preprocessors are really what we're after (and extractable); the old-styletheme_
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.
Comment #28
thedavidmeister CreditAttribution: thedavidmeister commentedRelated, #2044105: Introduce #alters for \Drupal\Core\Render\Renderer::render() would make it easier to drill elements rendered outside #theme.
Comment #29
Fabianx CreditAttribution: Fabianx commentedI created #2047263: Provide inline_template tag within Twig that inlines a template for usage with a render array, 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.
Comment #30
Fabianx CreditAttribution: Fabianx commentedThe 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 that inlines a template for usage with a render array 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.
Comment #31
jenlamptonPossibly related #2009132: Remove #pre_render and #post_render from drupal_render() as it prevents proper Twig drillability
Comment #31.0
jenlamptonminor grammar
Comment #32
sunComment #33
star-szrWe could maybe do some things here during 8.x.x if we can do it in a way that maintains backwards compatibility, but not for 8.0.x so just bumping for now.
Comment #35
steveoliver CreditAttribution: steveoliver commentedUnassigning; not sure the status of the feasability of this and not working on it at the moment.
Comment #49
andypostas parent closed