Problem/Motivation

I've encountered two cases where I needed a render array to be rendered in order to run a twig filter on it, e.g. trim(). Otherwise an error is generated because typically those string functions cannot operate on array. Here is an example to generate a twitter field:https://gist.github.com/scor/9931b6ebf456caa356a6. The workaround I had to use was to descend into the array to find the right value, but that's far from being intuitive and optimal.

Proposed resolution

Introduce a 'render' filter in twig that would render the array into a string, which can then be passed to other Twig filters.

Remaining tasks

User interface changes

N/A

API changes

New filter in twig: render

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task, small functional addition
Issue priority Normal, helps with some use cases
Disruption Not disruptive at all for core or contrib.
Files: 
CommentFileSizeAuthor
#1 add_a_render_filter_to-2447049-1.patch2.55 KBjoelpittet
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,866 pass(es). View

Comments

joelpittet’s picture

Issue summary: View changes
Status: Active » Needs review
Issue tags: +Twig
FileSize
2.55 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,866 pass(es). View
joelpittet’s picture

Assigned: Unassigned » Cottser

What do you think @Cottser?

Cottser’s picture

Assigned: Cottser » Unassigned
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: +TX (Themer Experience), +DX (Developer Experience)
FileSize
1.89 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 87,985 pass(es), 1 fail(s), and 0 exception(s). View

Attached "test only" patch demonstrates the need for this with a test failure.

Patch from #1 is RTBC from me, just a small addition and includes test coverage. Updating beta evaluation in the issue summary.

Related: It would be nice to unify the existing "render_var" function and call that "render" as well so this is available as both a filter and function. But for backwards compatibility maybe we can just add the following line and tests (or modify existing any tests that use render_var), but that should probably happen in a separate issue unless we agree it makes sense to do it all here. I don't see a strong reason to hold this one up though.

new \Twig_SimpleFunction('render', 'twig_render_var'),

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 3: 2447049-3-test.patch, failed testing.

Cottser’s picture

Status: Needs work » Reviewed & tested by the community
Wim Leers’s picture

I think it's confusing you only want |render if you're doing things like |length or |trim. Because rendering of render arrays actually happens automatically, except in case you're directly applying a filter.

Isn't this therefore a TX/DX regression?

scor’s picture

Well, |length and |trim would work well if Drupal was passing objects to Twig all the time. We need this render filter because of the fact that Drupal like to pass arrays to Twig, which natively Twig doesn't support. Should we instead never pass arrays to Twig, convert all our render array to objects with a __toString() method equivalent to our render function?

Wim Leers’s picture

Assigned: Unassigned » Fabianx
Status: Reviewed & tested by the community » Needs review

Can't we make TwigNodeVisitor not wrap the existing expression (and thus make the "outer" call one to twig_render_var()) but instead make it smarter to become the first invocation/filter so that it's the "inner" call?

(I don't know Twig terminology well enough.)

Assigning to Fabian to get his feedback.

Fabianx’s picture

Generally I am +1 to this change as its sometimes needed to render something and we already have a function, so its okay to also define a filter.

On render being smarter, there is a chicken-egg problem:

You might want to apply a filter to the render arrays, like the |exclude() filter, which is the show/hide, so you don't want to render it.

- You want to get a count, then you _do_ want to render it first.
- You also generally would like to not render things twice.

There is a way out by probably extending the API of twigs filters.

If |exclude() could specify that it needs render_arrays or raw values (similar to how they specify which escaping strategy is supported and if the result is safe), then it would be possible to indeed be smarter about the filtering, though some things like |join remain, that are impossible, because:

There is no way to know if something is a render array or not, #type and #theme are indications, but it can just be children with arbitrary keys ...

e.g.

[ 'london', 'paris', 'barcelona' ] is not a valid render array.

[ 'london' => ['#markup' => 'foo'], 'paris' => [...], 'barcelona' => '...' ] is.

This is related in ways to the visibility issue, which I still would love to discuss in IRC ;).

Fabianx’s picture

Assigned: Fabianx » Unassigned
Cottser’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#1843798: [meta] Refactor Render API to be OO

Below is a cross-post, but I fundamentally agree with what @Fabianx said - except it's |without(), not |exclude() ;)


Twig can deal with both arrays and strings. If we try to automatically render a render array every time it goes through a filter, AFAIK we not only kill our own |without filter, but also use cases for built-in array filters: join, first, last, keys, length, and more…

The failing test in #3 isn't because {{ render.array|length }} returns NULL or an exception, it's because |length returns the length of the array or string depending on what it's given - in that case 1.

So I don't think there are any fundamental changes needed here. Of course having objects with __toString methods as @scor said would be ideal but I don't see that happening for D8. And actually I'm not sure how you would distinguish in a Twig template from whether you wanted the "array" part of the object vs. the string part, you'd probably have to drill down to a method call or something to return the array. So DX/TX-wise not sure that is actually an improvement in this particular case.

Wim Leers’s picture

Ok, just wanted to make sure we know what we're signing up for! Looks like we have sound justifications for doing things this way.

Cottser’s picture

Indeed, thanks for the feedback!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 9e13c54 and pushed to 8.0.x. Thanks!

Thanks for adding the beta evaluation.

  • alexpott committed 9e13c54 on 8.0.x
    Issue #2447049 by Cottser, joelpittet: Add a render filter to twig
    
Cottser’s picture

Status: Fixed » Closed (fixed)

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