| Issue category | Task |
|---|---|
| Issue priority | Normal |
| Unfrozen changes | Unfrozen because this is either a documentation change or code change to ensure code matches documentation. |
| Prioritized changes | Prioritized as a follow-ups from a recent critical or major change (introduction of Twig). |
| Disruption | None. |
https://www.drupal.org/node/1806538 resolved that strict_variables should not be enabled by default in Twig. This decision has allowed us to get sloppy in core themes. Core themes should reflect best practices and allow subthemes to use strict_variables without throwing exceptions.
Our coding standards require strict variables in JavaScript.
In form-element.html.twig, in the documentation says the description_display variable exists:
...
* - description_display: Description display setting. It can have these values:
* - before: The description is output before the element.
* - after: The description is output after the element. This is the default
* value.
* - invisible: The description is output after the element, hidden visually
* but available to screen readers.
...
(note: not “optional” like the description variable). However template_preprocess_form_element() does not guarantee description_display will be available in $variables. The result is an exception if you enable any core theme and turn strict_variables on.
If a variable is listed as "(optional)" in the template documentation, then the template should verify its existence before using it. Otherwise, the preprocess function should provide the variable – even if NULL – so that templates can be kept as clean as possible.
For example, if the documentation says that either foo or foo.bar is optional, then a template should use:
{{ foo.bar|default }}
or
{% if foo.bar|default is not empty %}
<div class="this-is-a-foo">{{ foo.bar }}</div>
{% endif %}
The more verbose (and potentially more readable to new users) version would be:
{{ foo.bar|default('') }}
or
{% if foo.bar is defined and foo.bar is not empty %}
<div class="this-is-a-foo">{{ foo.bar }}</div>
{% endif %}
But if the documentation says that foo.bar is available, then a template should not need to check for it as the preprocess function should provide, at a minimum, a NULL placeholder.
{{ foo.bar }}
or
{% if foo.bar is not empty %}
<div class="this-is-a-foo">{{ foo.bar }}</div>
{% endif %}
strict_variables on the testbot.None.
None.
| Comment | File | Size | Author |
|---|---|---|---|
| #3 | 2445705-3-twig-strict_variables.patch | 7.45 KB | mikeker |
Comments
Comment #1
mikeker commentedFixed php tag in summary.
Comment #2
mikeker commentedAdded related issue and updated summary.
Comment #3
mikeker commentedInitial stab at it.
Comment #4
star-szr@mikeker first off thanks for opening this as a separate issue! Can you please justify this being a best practice? The issue summary uses that phrase but doesn't back it up. strict_variables is off by default upstream in Twig, and I stand by the decision we made to not support it in core. So at this stage I think we need some convincing :)
Comment #5
mikeker commented@Cottser: I agree, with D8's emphasis on production settings out-of-the-box,
strict_variablesshould be off by default and I don't mean to revisit that decision in this issue. What I would like to see is the option to turn it on when developing a site. Currently no core theme works withstrict_variablesmeaning you can't turnstrict_variableson without adding significant preprocessing and template overriding. That seems a shame.As to the claim of best practice, you raise a fair point and I apologize for using that term without support. My (very) preliminary research didn't find any projects that require
strict_variables. But most projects are coded such that you can usestrict_variablesif you want. The closest I could find was a discussion in the Bolt issue queue regarding this. Bolt will not throw exceptions if your turn it on as they suggest in their documentation.Finally, JavaScript can be written as a loosely typed language but Drupal's coding standards require "use strict" to prevent typos and silent failures. I would like to see the same thing in Twig.
Let me know if that's enough to convince you! :)
Comment #6
fabianx commentedThere had been 2 reasons for not using strict variables in Core:
a) performance - that is the production ready thing, yes for sure.
b) ease of the templates.
So with b) we want to ensure that the templates don't get very complicated and hard to read, i.e. it might be preferably to define an empty var in preprocess instead of writing "is a defined and is a not empty".
Comment #7
mikeker commented@Fabianx: thank you for your comment. Just to clarify, I'm not arguing that we change the default setting for
strict_variablesin core. I'm all for D8 having production settings out of the box. I do think core themes should be compatible withstrict_variablesif one were to turn that on during development.Agreed! In many (most?) cases, it's just a matter of defining an empty entry in the
$variablesarray in a preprocess function. The patch in #3 starts that process but has plenty of work left... That approach avoids a lot of "foo is defined and foo is not empty". Though in many cases it can be reduced to "foo|default" at the cost of readability (IMO).Personally I prefer a more verbose and readable template file (especially since it's compiled to PHP) but that discussion is for the coding standards folks. :)
Comment #8
joelpittetI like the idea, don't get me wrong. But it would be a bit tricky because first we'd have to update the templates for their defaults (not terribly difficult).
The harder part would be supporting this feature because it's hidden(off by default), test coverage on this feature would be quite far reaching and simple single page tests wouldn't cover all the 280+ templates. Any suggestions on how we can do this?
Regarding the patch above:
It would be better to move ALL these to the
hook_theme()'variables' if possible.That provides a definition for what the template can expect and should match the template's variables in the docblock.
If that helps us get closer to this without drastically altering the templates I'm all for it but as for a full support for this feature I think it'll be quite a challenge to support.
@mikeker++ for keeping striving for ideals! It resembles XHTML, it helped the web be a bit more consistent albeit they took it too far and lost support but in the long run it has improved the web. Maybe we can find a reasonable way forward for this?
Comment #9
mikeker commentedAgreed, unless there is a way to turn strict_variables on for the testbot. Existing tests would give us pretty complete coverage if we could do that (Twig throws an exception when undefined vars are used). Really, without that option, maintaining this would be impossible. Alas, I know very little about testbot infrastructure...
re: hook_theme:
I'll look into that. I would like to do as much of this at the preprocess/hook_theme level to avoid adding a bunch of conditional logic to the template files. In poking around over the past couple days, it seems that most of the errors can be fixed by just adding
$variables['foo'] = NULL.No trickier than getting Twig into core in the first place! :P
Comment #10
fgmAny reason not to expose the setting in default.services.yml to an explicit false ? Documenting that it can be changed for development, just like cache or auto_reload, along with a similar explanation on the performance impact ?
The same performance argument holds for cache and yet it is already exposed in that file, so that devs reading the file are aware it can be changed when developing.
One possibly interesting note is how I came to discover this issue : I was developing a module providing Response objects directly, hence not using the theme system as such, but using Twig directly, which is probably typical for D8 code designed to also be used outside D8 in Silex/SF2 projects, so I set strict_variables to debug my template code. And any time I had an exception in Twig for my templates, it came out as an exception outside my templates, reported as a missing variable in Classy, the actual exception being only discoverable by digging in the undisplayed exception stack at the point where it happened, and climbing up 11 levels of stack to access the actual error. This was caused by the theme layer trying to render a page from an exception response, and not having access to its variables. This is not something that should be imposed site/app developers/themers, and which could be avoided if core themes worked with strict_variables: true, just like our coding practices recommend for PHP (error_reporting) and JS (use strict).
Comment #13
mikeker commented@fgm: After some lengthy discussions with @joelpittet on IRC about this, I don't see this happening in Drupal 8.0. I've started a sandbox theme called Strict, which is basically Stark but doesn't throw exceptions, but haven't had the time to get very far with it (upcoming D8 Twig sprint at PNWDS should help with that!)
Until core themes don't barf when
strict_variablesis TRUE, I don't think we should mention it default.services.yml. Because then people like me will turn it on and their site will crap out. If I can get the Strict theme to run through the basic list of pages without throwing exceptions, then I'll promote it to a full project and we can go from there.Thoughts?
(On a side note, I think this will come up again after a year or two of D8 being in the wild. We find more "programming" in Twig templates as Drupal will start exposing more variables to templates and people will start making dumb typos -- I do all the time! -- and someone will suggest we turn on strict like we do elsewhere...)
Comment #14
fgm@mikeker : I suppose that's how it will have to be... interesting idea, this Strict theme. But something can remain an issue even with it if you look at my previous explanation : the problems happened when working on a site /not/ using a theme since it built SF2 responses directly : core would still try to use the theme layer to display exceptions, defaulting to Classy because that's all it had as a base.
So even with a perfectly valid theme, the root issue is in Classy which, although rather minimal, still manages to be non-strict. Wouldn't it be possible to have at least this foundation be strict-compliant, even if Bartik and Seven are not ?
Comment #28
smustgrave commentedThank you for creating this issue to improve Drupal.
We are working to decide if this task is still relevant to a currently supported version of Drupal. There hasn't been any discussion here for over 8 years which suggests that this has either been implemented or is no longer relevant. Your thoughts on this will allow a decision to be made.
Since we need more information to move forward with this issue, the status is now Postponed (maintainer needs more info). If we don't receive additional information to help with the issue, it may be closed after three months.
Thanks!
Comment #29
fgmAFAICS, this is still relevant, especially regarding my suggestion in #15 to just expose that setting and be done with it. That way administrators/developers can decide if it matters to them, defaulting to not.
In the long run, that seems much simpler and likely to remain stable than actively keeping the core themes passing with strict variables.