Updated: Comment #3

Problem/Motivation

Let's discuss whether we should enable strict_variables in Twig. This defaults to FALSE whereas for example autoescape is enabled by default. So we're not straying from the defaults.

Pros of enabling strict_variables

  • Easier to catch typos

Cons of enabling strict_variables

  • Higher quantity and more verbose if statements may be required (Example from Bolt CMS docs):

    Before:

    {% if content.image != "" %}
        (do something with the image..)
    {% endif %}

    After:

    {% if content.image is defined and content.image != "" %}
        (do something with the image..)
    {% endif %}

Proposed resolution

TBD

Remaining tasks

Discuss

User interface changes

n/a

API changes

n/a

Original report by @Fabianx

Currently accessing a not defined var in strict mode gives a FATAL error message. We might want to have a warning instead and return "null".

This needs discussion.

To check for a var in twig use:

if key in array

or more general like isset()

if array.key is defined

or

if arrayis defined

CommentFileSizeAuthor
#8 1806538-strict-todo-fix-8.patch705 bytesjoelpittet
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

podarok’s picture

do we really need make FATAL error message not FATAL?
Are there any example for using templates with undefided variables?

joelpittet’s picture

Priority: Normal » Major
Issue summary: View changes

Just following up: currently we have strict_variables => FALSE. I'm a bit unsure of what advantages we would gain from turning that to TRUE;

Throwing exceptions at themers for not knowing a variable doesn't exist sounds like a pain.
Uncaught exception 'Twig_Error_Runtime' with message 'Variable "variable" does not exist

Though if we are going to decide to go the explicit route, we'd better decide before beta because turning this on after people start using it is asking for a headache.

star-szr’s picture

Title: Twig: Enable strict variables, but handle exceptions gracefully » Twig: Decide whether to enable strict_variables
Issue summary: View changes

Updated the issue summary after doing a bit of research. I'm not convinced we should enable this.

star-szr’s picture

Issue summary: View changes

Fixing typo and slight reword in the issue summary.

forestmars’s picture

I'm still not seeing what the advantages would be of enabling strict_variables…?

star-szr’s picture

Status: Active » Closed (won't fix)

Thanks @forestmars, I'm still not feeling this either. Going to close this, we have plenty of other stuff to work on :)

star-szr’s picture

Priority: Major » Normal
Status: Closed (won't fix) » Active
Issue tags: +Novice

Actually. We need to remove the @todo lines before we can close this:

// @todo Remove in followup issue
// @see http://drupal.org/node/1806538.
joelpittet’s picture

Status: Active » Needs review
FileSize
705 bytes

Here it be.

star-szr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Novice
star-szr’s picture

Title: Twig: Decide whether to enable strict_variables » Remove @todo about enabling strict_variables in Twig
webchick’s picture

Status: Reviewed & tested by the community » Fixed

Yeah that before/after code snippet is pretty ick, so I definitely support leaving this at the default of FALSE.

On IRC Cottser mentioned that since FALSE is already the default, we don't need that line right below the comments anymore, so I removed that on commit and pushed to 8.x. Thanks!

Status: Fixed » Closed (fixed)

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

mikeker’s picture