Problem/Motivation
TwigReference Removal Problem/Motivation
- Blocks turning "auto-escape" on by default #1825952: Turn on twig autoescape by default because the 'raw' filter doesn't produce results.
- Breaks Twig debug's dump command for
{{ dump(_context|keys) }}
because an empty array is always produced. - Prints child elements twice on second renderable array print and 4 times on third print.
example
{{ data }} {{ data }} {{ data }}
Expected:value value value
Actual:value valuevalue valuevaluevaluevalue
@see fail test in #2114563-67: Remove TwigReference with the help of 'without' filter replacing 'show'/'hide' functions. - Causes a minor decrease in performance by tracking and passing references around.
Show/Hide Removal Motivation
The show
and hide
twig functions map to the equivalent Drupal PHP functions. The hide function prevents the specified child element of a renderable array from being printed along with the rest of the children by internally setting #printed => TRUE
. The show function sets #printed => FALSE
to allow printing or re-printing of the specified element.
While this sounds useful and has some practical use-cases, its overhead is quite large. The show and hide functions require TwigReference
to wrap all arrays sent to Twig. Because of TwigNodeVisitor
, TwigReference inadvertently breaks the twig raw
filter. The broken raw filter prevents us from turning autoescape on by default #1825952: Turn on twig autoescape by default.
{% hide(content.links) %}
{{ content }}
{% show(content.links) %}
<nav>{{ content.links }}</nav>
Why both in one issue:
These are not easy to separate because TwigRefence's main reason for being is to support the show/hide feature of renderable arrays. So committing the removal of it would likely break show/hide functionality. And as you can see by the test fail in #67 , committing the without
filter would fail the tests without TwigReference removal as well.
Proposed resolution
The show
function is not used anywhere in core and is not needed. If we need to print a renderable array, we can just {{ print }}
it twice.
@webchick gave me a hand here. This is all of the calls to show
in contrib:
./atom_nzgovt/views_plugin_row_node_atom.inc: show($node->content['links']);
./dcco/includes/common.inc: show($element);
./oembed/oembed.module: return show($element);
./rotoslider/theme/theme.inc: show($element[$key]);
The hide
function can be useful to exclude children from a renderable array.
The solution here is to remove show()
and replace the hide
function with a filter. This is an improvement because a filter always applies to an array with children. A filter is also more explicit and in context to what is happening.
This issue only removes show() and hide() from the context of Twig templates, the functions are still available in the PHP layer and preprocess functions (which are the only usage of show() above).
Example:
Before
{% hide(content.links) %}
{% hide(content.actions) %}
<article>{{ content }}<article>
...
<nav>{{ content.links }}</nav>
<nav>{{ content.actions }}</nav>
After
<article>{{ content|without('links', 'actions') }}<article>
...
<nav>{{ content.links }}</nav>
<nav>{{ content.actions }}</nav>
Remaining tasks
Get feedback of use-cases that aren't solvable without show/hide in the template files.Remove 'show' and 'hide' twig functions.Replace 'hide' twig function calls in templates with the equivalent 'without' filter call.Remove TwigReference/TwigTemplate, their tests, and their reference passing from TwigNodeVisitor.Write tests for the 'without' filter.
API changes
Remove 'show' and 'hide' twig functions from inside templates.
The hide() function is replaced with a filter called 'without' which can be applied to a renderable array.
Comment | File | Size | Author |
---|---|---|---|
#95 | interdiff-89-94.txt | 956 bytes | martin107 |
#95 | 2114563-94-remove-twigreference-without-filter.patch | 38.48 KB | martin107 |
#94 | interdiff.txt | 956 bytes | star-szr |
#94 | 2114563-94.patch | 38.48 KB | star-szr |
Comments
Comment #1
star-szrThanks for posting this @joelpittet, I really like this idea.
Just discussed this with @Mark Carver on the Twig call.
I think we can solve this really cleanly with set tags. http://twig.sensiolabs.org/doc/tags/set.html
So new code for example 1 without unset, (untested):
Comment #2
markhalliwellI definitely agree with removing hide/show. The above code in #1 I think makes a lot of sense. +1
Comment #3
star-szrAdding related issues. Removing show()/hide() would actually vastly improve template inspection since we could remove TwigReference objects.
Comment #4
star-szrI think this qualifies as cleanup and an API change.
Comment #5
joelpittetYay delicious murder! :)
Should we just remove it from twig (easier) or remove them from preprocess/prepare layer as well?
Comment #6
joelpittetQuick note: show() is not used in core, only hide(). So still think unset() would be nice to have.
Comment #7
joelpittetJust a test to see what we're dealing with.
Comment #8
joelpittetComment #10
star-szr@joelpittet - Yeah, we could just use a set tag and never use the resulting variable but that's wasted rendering time I think.
Maybe we create a custom Twig filter/function to just remove items from an array by key along the lines of your unset(content.links) example.
Comment #11
tim.plunkettComment #12
joelpittet7: 2114563-kill-show-hide-in-twig.patch queued for re-testing.
Comment #14
joelpittetIf I can get away with it, I'd prefer not to introduce unset like twig was meant to be... so if we can do it in preprocess... all the better. This is more sane errors too. but the way nodes get variables in content is so weird.
Comment #15
tim.plunkettThis seems like a pretty unfortunate DX (TX?) loss. Are we sure its worth it?
EDIT: See http://drupalcode.org/project/drupal.git/commitdiff/1c54559 for when we added this template. It removed the left/right logic from the form.
Comment #17
joelpittetSo in an effort to try and make more happy, I left hide as a filter to only 'exclude' children from printing. This seems to be a bit cleaner and may still allow the removal or replace of TwigReference.
Comment #19
joelpittet#17 totally kills show from twig, turns hide into a filter and it doesn't call hide() php function so it doesn't get passed as a reference. The exception/fails are probably easy to deal with. And *crosses fingers* it should help us kill TwigReference which is the ultimate goal here. Still gives people who use hide() the feature still in a slightly cleaner way. Also core doesn't use show() at all at the moment in twig files, so I doubt it will be missed. Still consider changing the name of hide to something more intuitive like, exclude() or something. Let me know your thoughts.
Comment #20
joelpittetThat was easy to fix;)
Caveat, this fix doesn't currently go two deep... though it could if deemed necessary.
Comment #21
star-szrNice green patch. @joelpittet two levels within the render array?
Comment #22
tim.plunkettThis should likely get some profiling numbers.
If you're going to start fixing other things, this needs a docblock. Or just remove this hunk from the patch...
I personally think this is a TX improvement!
Missing space after foreach, and {} around if. Also the func_get_args part should be documented like
@param ...
in the docblock, see FormBuilderInterface::getForm() for an example.When you don't need the return value of array_shift, use unset($args[0]); (This is reliable because func_get_args returns a numerically indexed array
Comment #23
star-szrThanks @tim.plunkett! I agree on #1, I think we should just remove the hunk. I also agree the syntax is an improvement :D
Comment #24
joelpittetThank you @tim.plunkett and @Cottser
@Cottser yeah one level of children. If we need to hide deeper children we can accomplish that in other ways... though not sure if it's needed?
I tried to clean up docs and remove twig_show()
Comment #26
markhalliwellComment #27
star-szrExample of possibly related functionality from another CMS using Twig:
http://buildwithcraft.com/docs/templating/filters#without
Comment #28
joelpittet24: 2114563-24-kill-show-hide.patch queued for re-testing.
Comment #31
joelpittet24: 2114563-24-kill-show-hide.patch queued for re-testing.
Comment #34
joelpittetAdded the getName back in because it's part of the Twig_ExtensionInterface interface.
Comment #35
star-szrNice! I think we should remove the unrelated code style/docblock cleanup from this patch.
Comment #36
tstoecklerI know this was already this way before, but why not type-hint $element directly?
Why do we support the case of not supplying any args?
Comment #37
joelpittetGood points @tstoeckler, thanks for the review! I'm just used to catching edge cases in my code. Here's without.
Comment #38
tstoecklerAwesome, thanks!
One minor nitpick: AFAIK TwigReference is in the global namespace, so it should be prefixed with a backslash.
Comment #39
star-szrTwigReference is in \Drupal\Core\Template, it's
use
'd in twig.engine, so I think the function parameters are fine. The docblock should probably use the full namespace though.Comment #40
joelpittetComment #41
tstoecklerOops sorry. Thanks for clearing that up!
Comment #42
joelpittetComment #43
joelpittet@todo: Split out the cleanup into a follow.
Comment #44
joelpittetMind if I bump this up to major? We really need to get rid of TwigReference and possibly TwigNodeVisitor because they are blocking. #1825952: Turn on twig autoescape by default
Comment #45
chx CreditAttribution: chx commentedThis is great and everyone working on Twig agrees but if I see this correctly, we lost all comments regarding the usage of hide? Like this:
Comment #46
chx CreditAttribution: chx commentedAlso, renaming to without is a great idea.
Comment #47
yched CreditAttribution: yched commented"without" ++
Comment #48
joelpittetThanks @chx and @yched. I am +1 for 'without' as well. So here is that plus getting those docs back in there and removing the class cleanup to be added to a followup.
Comment #49
joelpittetWrong interdiff, sorry forgot to commit.
Comment #50
yched CreditAttribution: yched commentedNitpick: "temporarily suppress the printing of a given child element" is a bit vague.
"Will it come back if I wait long enough ?" ;-)
Maybe: "exclude a given child element from the output" ?
Comment #52
joelpittet49: 2114563-kill-show-hide-48.patch queued for re-testing.
Comment #54
joelpittet49: 2114563-kill-show-hide-48.patch queued for re-testing.
Comment #55
star-szrIf we can make the 'without' filter work for classes/attribute objects I can see that being a big win for being able to easily strip out unwanted classes, that can be follow-up though.
Comment #56
star-szrAlso, NW because this needs tests.
Comment #57
joelpittetComment #58
joelpittetThis is just a rough of where I think the tests should go... maybe you can tell me if I'm on the right track...? I don't write many tests.
Comment #59
joelpittetAssigning. Less murderous title. Attached is an experiment in allowing attributes and attributes.class to be used with this filter.
I can do this array_flip trick for OffsetGet or I can set the AttributeArray keys to the values on OffsetSet. This allows for easy removal of classes, although individual classes don't have printed states.
Comment #60
joelpittetI'm going to need to revert my type hinting because I'm trying to "take out" TwigReference because it's evil:
{{ debug(_context|keys) }}
from working.I'll put the Attributes to use |without as a follow up as I'm sure the attributes can be done, and maybe even individual classes as well if I play my cards right!
And remove type hinting because I'll need to accept Attributes and why not any array/object that has a printed property
Comment #61
dawehnerCan't we just put that code directly in the test and safe the full http request around it in the test? the TwigNamespaceTest does the same
Comment #62
joelpittet@dawehner that's a great suggestion, I'll give that a try if I can do it:)
Comment #63
star-szrAdding focus tag.
Comment #64
star-szrJust fixing the status since this still needs tests at least.
Comment #65
joelpittetI moved the filter tests into the twig_theme_test to lighten the load a bit and made theme nicer to look at.
Also, I've put in the removal of TwigReference and TwigTemplate and it's kin. The reason is because in doing the tests if you print a render array more than once on a page, it would duplicate it's children each time.
Try the referenced patch. Note the actual tests aren't written yet... sorry.
drush en twig_theme_test -y
then go to:
/twig-theme-test/filter
Sorry tired when I wrote this... should move TwigReference/TwigTemplate killings to a new critical patch... will do tomorrow, with some test to prove it's critical.
Comment #66
joelpittetStarting cleanup the issue summary.
Comment #67
joelpittetNow with tests. Hopefully my verbose patch names help.
Comment #68
joelpittetComment #71
joelpittetNeed some help with the issue summary clean-up.
Due to:
I'm bumping this to Critical.
These are not easy to separate because TwigRefence's main reason for being is to support the show/hide feature of renderable arrays. So committing the removal of it would likely break show/hide functionality. And as you can see by the test fail above, commiting the without filter would fail the tests without TwigReference removal.
Also a side benefit is we can use the without on attributes to allow explicit out of order rendering of attributes:
Currently
class="{{ attributes.class }}"{{ attributes }}
will magically not print the class attribute twice due to Attribute's printed properties and{{ attributes }} class="{{ attributes.class }}"
would print two class attributes with the second being empty which is not likely expected.Though with this without filter we can likely improve DX with explicit
{{ attributes|without('class') }} class="{{ attributes.class }}"
to let the template do exactly as we ask it.Comment #72
joelpittetComment #73
joelpittetComment #74
joelpittetUnassigning, I gave a stab at the issue summary. Please have another look through the code and issue summary for any minor detail I may have missed.
Comment #75
RainbowArrayThis probably needs to be changed to "Use following code to exclude the printing of a given child element:"
Maybe change to:
"Prevents child elements in a renderable array from being printed."
Above are just a couple comments that could be made clearer.
As much as I can understand what's going on, it seems to make sense. Seems easy enough to use for themers and site builders. I'll take a look at the issue summary.
Comment #76
RainbowArrayTried to clean up a bit of the language in the issue summary.
Comment #77
RainbowArrayFor twig_without docblock, maybe something like this:
Creates a copy of the renderable array without the child elements specified, so that the copy can be printed without these elements. The original renderable array is still available and can be used to print child elements on their own later on in the template.
Comment #78
star-szrThanks @mdrummond! Adding one line to the issue summary and making a small tweak but it's looking pretty good to me. Nice work @joelpittet and @mdrummond. Going to see about reviewing the patch but may not get to posting a review right away.
Comment #79
star-szrOn top of @mdrummond's suggestions, here's my review:
Twig_Function_Function is deprecated, use Twig_SimpleFunction instead. Probably might as well convert 'url' as well since that's the only other one we're keeping here:
…should do it.
Similarly here, this is deprecated and Twig_SimpleFilter is the new way to do it. Not sure we should convert all these, that could be done in a separate non-critical issue.
"Use the following code" would read a bit better here IMO. (Two instances)
Method name and docblock need updating here.
Remove extra blank line at the start of this function please :)
I don't think the string[] thing matches up with https://drupal.org/node/1354#param. I have a feeling we don't document this as a @param at all when we're grabbing the extra arguments, I found a couple examples in views.module like the following but not sure that is part of core standards.
Comment #80
joelpittetThis is only doc cleanup, I think the
@param string[] $args, ...
should be in the coding standards though I did add a space it does describe a lot better what is going in.Thank you very much @Cottser and @mdrummond for the reviews!
We can grab the interdiff here and post it as a follow-up if that makes the patch easier to read?
Comment #81
joelpittetHere's a profiling. It's not a huge improvement, but still one.
Scenario
jQuery(":checkbox").prop('checked', true);
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53180b3960796&...
http://www.lionsad.de/xhprof-kit/xhprof/xhprof_html/?run1=53180b3960796&...
Comment #82
star-szrI really think the unrelated docs/coding standards cleanups should be moved to a separate, non-critical issue to expedite this patch. I can volunteer to do that, just let me know what you'd prefer @joelpittet!
Comment #83
star-szrFrom what I can see the changes look great by the way :)
Comment #84
joelpittetAdded a docs clean-up follow-up non-critical;)
#2212309: Drupal\Core\Template and twig.engine docs, coding standards, and unused code cleanup
These are just more important bits from #80 applied to #67
Comment #85
star-szrThanks @joelpittet, that makes it much easier! Some more minor points, I can't find any more faults, this is very close IMO.
Since this added comment is duplicating the docblock, let's either remove this change or make the change you had before changing the docblock to {@inheritdoc}. No sense doing this in between change :)
If we're going to define the $line variable, let's use it! Replace the last $node->getLine() with $line.
Minor point but the "We hide links..." comments here might not make as much sense when we're using |without. Maybe something as simple as "Render the content without the links."? Or maybe it's a lot more self-explanatory now and we don't need to comment on it at all? :)
(4 instances)
I think this is the only case where we're losing valuable docs, this should be consistent with the node.html.twig in node module IMO where we retain the docs on how to hide/exclude sub-elements.
Comment #86
joelpittetDid mostly the drop/remove options from #85;) and thanks for the
$line
variable catch!Comment #87
star-szrTestbot go!
Comment #88
star-szrI don't think this array needs keys, just
new \Twig_SimpleFunction(…),
.Comment #89
joelpittetThanks, good catch wouldn't do anything but totally unnecessary with that syntax.
Comment #90
star-szrI think that's the winner. To sum up:
{{ this|without('that') }}
Comment #91
star-szrWoke up and realized we'd need a draft change record: https://drupal.org/node/2212845
Comment #92
Fabianx CreditAttribution: Fabianx commentedWhat a genius idea!
+1 to **RTBC** from me.
I reviewed the code and that works very well.
My only nitpick is:
This file is now orphaned. Poor TwigNodeExpressionNameReference is no longer needed :).
Not sure that this is a merge blocker though.
Thanks!
Comment #93
catchLet's remove that while we're here.
Comment #94
star-szrThanks @Fabianx! Makes the diffstat even tastier.
Comment #95
martin107 CreditAttribution: martin107 commentedRemoved TwigNodeExpressionNameReference.
Comment #97
star-szrPatches are identical so I've cancelled the testing of mine, #95 is good to go pending testbot's approval :)
Comment #98
martin107 CreditAttribution: martin107 commentedOpps now that I have been caught playing snap with Cottser ... I will have to stand in the corner.
Comment #99
catchCommitted/pushed to 8.x, thanks!
Comment #100
joelpittetChange record published @ https://drupal.org/node/2212845
Thanks everybody!
Comment #101
star-szrThis means we can now remove a line from twig_render_template() as well, small Novice follow-up:
#2218849: Remove unused line from twig_render_template()
Comment #102
forestmars CreditAttribution: forestmars commented