Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
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.

Problem/Motivation

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.

More details

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.

Proposed resolution

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 %} 
  
{{ foo.bar }}
{% 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 %} 
  
{{ foo.bar }}
{% 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 %}
  
{{ foo.bar }}
{% endif %}

Remaining tasks

  1. Update preprocess functions and Twig template documentation so that variables are correctly accounted for.
  2. Figure out if it's appropriate to turn on strict_variables on the testbot.

User interface changes

None.

API changes

None.

Files: 
CommentFileSizeAuthor
#3 2445705-3-twig-strict_variables.patch7.45 KBmikeker
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2445705-3-twig-strict_variables.patch. Unable to apply patch. See the log in the details link for more information. View

Comments

mikeker’s picture

Issue summary: View changes

Fixed php tag in summary.

mikeker’s picture

mikeker’s picture

Status: Active » Needs review
FileSize
7.45 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch 2445705-3-twig-strict_variables.patch. Unable to apply patch. See the log in the details link for more information. View

Initial stab at it.

Cottser’s picture

@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 :)

mikeker’s picture

@Cottser: I agree, with D8's emphasis on production settings out-of-the-box, strict_variables should 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 with strict_variables meaning you can't turn strict_variables on 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 use strict_variables if 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! :)

Fabianx’s picture

There 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".

mikeker’s picture

@Fabianx: thank you for your comment. Just to clarify, I'm not arguing that we change the default setting for strict_variables in core. I'm all for D8 having production settings out of the box. I do think core themes should be compatible with strict_variables if one were to turn that on during development.

i.e. it might be preferably to define an empty var in preprocess instead of writing "is a defined and is a not empty".

Agreed! In many (most?) cases, it's just a matter of defining an empty entry in the $variables array 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. :)

joelpittet’s picture

I 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:

+++ b/core/includes/form.inc
@@ -432,9 +436,11 @@ function template_preprocess_form_element(&$variables) {
+  $variables['type'] = NULL;
...
+  $variables['name'] = NULL;

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?

mikeker’s picture

The harder part would be supporting this feature because it's hidden(off by default)

Agreed, 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:

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.

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.

But it would be a bit tricky because first we'd have to update the templates for their defaults

No trickier than getting Twig into core in the first place! :P

fgm’s picture

Any 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).

Status: Needs review » Needs work

The last submitted patch, 3: 2445705-3-twig-strict_variables.patch, failed testing.

mikeker’s picture

@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_variables is 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...)

fgm’s picture

@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 ?

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.