Allow and test for NULL and integer 0 values in Twig templates.

tagging.

I was about to RTBC this, then I noticed that is_null also gives back TRUE if the array index does not exist. Therefore this fix is not right as the following script proves:

<?php

function gC_a($context, $item) {
  if (!isset($context[$item])) {
    echo "Error: $item does not exist";
  }
  return $context[$item];
}

function gC_b($context, $item) {
  if (!isset($context[$item]) && !is_null($context[$item])) {
    echo "Error: $item does not exist";
  }
  return $context[$item];
}



$context = array(
  'page_top' => NULL,
/*  'page_undefined' => NULL,*/
);

var_dump(gC_a($context, 'page_top'));
var_dump(gC_b($context, 'page_top'));

var_dump(gC_a($context, 'page_undefined'));
var_dump(gC_b($context, 'page_undefined'));

Expected result: 3 error messages; Actual result: 2 error messages.

Proposed Fix

I checked what Twig uses and they use a much slower:

if (!array_key_exists($item, $context)) {

So as a compromise and because isset() is really fast, I suggest we do:

if (!isset(...) && !array_key_exists($item, $context)) {

The new tests should still pass.

Gave this a run through and it worked great. Thanks @thedavidmeister

Also thanks for adding the test in there too.

There was a new line whitespace issue in the patch, I think it is in "core/modules/system/tests/themes/test_theme_twig/theme_test.template_test.html.twig" but it got corrected when I applied it the patch or my editor autofixed it because it wasn't there on opening it... :S

Whitespace fix.

I think the one I am seeing is line 65 of the patch, EOF whitespace. Maybe it's a patch format discrepancy but thanks @chx for finding a real whitespace issue:)

I'll post the patch file, that git diff is providing me. No interdiff because the extra line ending is fixed automatically when the patch is applied.

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTestTwig.phpundefined
@@ -69,4 +69,15 @@ function testFindThemeTemplates() {
+   * Tests that Twig templates handle PHP various data types correctly.

I think this should be 'various PHP data types', right?

+++ b/core/modules/system/lib/Drupal/system/Tests/Theme/ThemeTestTwig.phpundefined
@@ -69,4 +69,15 @@ function testFindThemeTemplates() {
+   * Tests that Twig templates handle PHP various data types correctly.

Also... is this really various types... is not just testing whether or not twig handles nulls in the expected fashion?

RTBC per #14.

I second this RTBC also:

* Fixes a bug
* Has tests to show the bug
* Is performance optimized

=> RTBC (plus #14 RTBC)

I don't think we should be adding @todo's at this stage. Let's test all the php scalar variable types.

This will fail because at the moment...

    'twig_int_0' => array(
      'value' => 0,
      'expected' => '0',
    ),

Will result in <li>twig_int_0: </li> whereas I think we should expect <li>twig_int_0: 0</li>

This is fixed here: #1970960: Twig implementation does not print int(0)

So maybe wait till that is committed, then commit this here?

@thedavidmeister since we're now in code slush each commit should make us closer to release and therefore we should avoid adding @todo especially if the patch is small and well contained. I see no reason not to merge #1970960: Twig implementation does not print int(0) and press on with your good idea to test all scalar variable types.

The commit message should additionally attribute alexrayu.

Updated issue summary.

And now removing the @todo :) doh!

Still RTBC per #14.

I second this RTBC also:

* Fixes two bugs
* Unblocks some twig conversions
* Has tests to show the bugs, adds extensive coverage for rendering via twig template.
* Is performance optimized

=> RTBC

RTBC per #30 - ahem ...

Something to consider: it might be worth creating a new template/callback for these additional tests. Maybe it's just me but it feels a bit like we are commandeering theme_test.template_test.html.twig which was previously a simple template override test:

+++ b/core/modules/system/tests/themes/test_theme_twig/theme_test.template_test.html.twigundefined
@@ -1,2 +1,8 @@
 {# Output for Theme API test #}
 Success: Template overridden.
+
+<ul>
+{% for key, php_value in php_values %}
+  <li>{{ key }}: {{ php_value.value }}</li>
+{% endfor %}
+</ul>

@Cottser yep I agree... let's not do that... added a new twig_theme_test module so we don't add any twigness to the theme_test module.

Remove unnecessary use...

Ah, that looks much better. Thanks @alexpott! :)

+++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.moduleundefined
@@ -0,0 +1,69 @@
+
+/**
+ * Implements hook_menu().
+ */
+function twig_theme_test_menu() {
+  $items['twig-theme-test/php-variables'] = array(
+    'page callback' => 'twig_theme_test_php_variables_callback',
+    'access callback' => TRUE,
+    'type' => MENU_CALLBACK,
+  );
+  return $items;
+}

Does this need a routing.yml now if we're adding a new page callback? Does the page callback need to be a controller? Not sure what the policy is around this now :)

Nitpickery:

1. +++ b/core/modules/system/tests/modules/twig_theme_test/templates/twig_theme_test.php_variables.html.twigundefined
   @@ -0,0 +1,6 @@
   +{# Output for Twig Theme PHP variables test #}
   

   End this comment in a period per http://drupal.org/node/1354#drupal.

2. +++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.moduleundefined
   @@ -0,0 +1,69 @@
   + * Menu callback for testing php variables in a twig template.
   

   All caps for PHP.

3. +++ b/core/modules/system/tests/modules/twig_theme_test/twig_theme_test.moduleundefined
   @@ -0,0 +1,69 @@
   + * Menu callback for testing php variables in a twig template.
   ...
   +  // Prefix each variable with "twig_" so that twig doesn't get confused
   

   Twig should be capitalized (not the "twig_" one of course).

Adding quick fix tag once this is RTBC again :).

Moved to new routing system and fixed everything in #35 plus a few other coding standards related things.

Thanks @alexpott, looks good. I went over the whole patch again and this is all I could find:

+++ b/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.phpundefined
@@ -0,0 +1,32 @@
+/**
+ * Controller routines for book routes.
+ */
+class TwigThemeTestController implements ControllerInterface {

Docblock needs updating :)

+++ b/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.phpundefined
@@ -0,0 +1,32 @@
+   * Menu callback for testing php variables in a Twig template.

All caps PHP please.

Ah I think there's some discrepancies with newlines too:

+++ b/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.phpundefined
@@ -0,0 +1,32 @@
+<?php
+/**
+ * @file
+ * Contains \Drupal\twig_theme_test\TwigThemeTestController.
+ */

http://drupal.org/node/1918356 shows a blank line before the @file, http://drupal.org/node/1353118 doesn't. I'm guessing the former is accurate.

+++ b/core/modules/system/tests/modules/twig_theme_test/lib/Drupal/twig_theme_test/TwigThemeTestController.phpundefined
@@ -0,0 +1,32 @@
+use Drupal\Core\ControllerInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+
+
+/**
+ * Controller routines for book routes.
+ */
+class TwigThemeTestController implements ControllerInterface {

Extra blank line after use statements can be removed.

Damn c&p from Drupal\book\Controller\BookController ... at least I didn't commit it :)

Looks great. Thanks everyone!

Updated issue summary.

Add commit message

Committed/pushed to 8.x, thanks!