Introduce a strict_variables option to the Engine #325

LeSuisse · 2017-08-28T08:49:08Z

When the strict_variables option is enabled, any encountered
unknown variables throw an UnknownVariableException.

This option is useful in a development/CI environnement to catch
mistakes in templates early instead of silently returning an empty string.

This kind of behavior in case of a variable "miss" is accepted by the
mustache manual [1].

[1] https://mustache.github.io/mustache.5.html#Variables

smarchint · 2017-09-22T12:29:28Z

$m = new \Mustache_Engine(['strict_variables' => true]);
$template = '{{#foo}}{{bar}}{{/foo}}';//'{{^foo}}{{bar}}{{/foo}}'
$context = ['nofoo' => ['asd' => 'asd']];
$output = $m->render($template,$context);

As per the PR, this throws Mustache_Exception_UnknownVariableException
For it to work, We need to provide values for the variables in sections and inverted sections.
It doesn't make sense to provide values for all the unnecessary vars.

better objective

'{{bar}}'
- If "bar" is not provided in the context for rendering the above template,
  then thow Mustache_Exception_UnknownVariableException.
'{{#foo}}{{bar}}{{/foo}}' //(section and inverted sections)
- If "foo" is not provided in the context for rendering the above template,
  then it should not throw an exception.
  [And bar should be condered to be in the outer context stack in case of inverted section.]

LeSuisse · 2017-09-22T21:15:00Z

Hi!

Thank your remark, I agree with you so I have made a fix to the PR.
However, I'm not really convinced that's the right way to solve it, I would be happy if I could get some feedback on it.

LeSuisse · 2017-09-22T21:38:21Z

Build fails on PHP 5.2 and 5.3, both are not available on Trusty on Travis CI which is now the default (see travis-ci/travis-ci#2963 and travis-ci/travis-ci#8219). The test matrix should explicitly specify we want a Precise environment for the PHP 5.2 and 5.3 tests.
I'm not sure that's the role of this PR to update the .travis.yml file, @bobthecow could you please advise if you prefer to have a dedicated PR to deal with the issue or if it should be solved in this one?

bobthecow · 2017-09-27T06:56:48Z

src/Mustache/Compiler.php

     * @param int    $entityFlags     (default: ENT_COMPAT)
     *
     * @return string Generated PHP source code
     */
-    public function compile($source, array $tree, $name, $customEscape = false, $charset = 'UTF-8', $strictCallables = false, $entityFlags = ENT_COMPAT)
+    public function compile($source, array $tree, $name, $customEscape = false, $charset = 'UTF-8', $strictCallables = false, $strictVariables = false, $entityFlags = ENT_COMPAT)


in order to maintain backwards compatibility (which this change will have to do) all params must be optional, and must be added to the end of the list. we can come up with a better order for the next release that breaks backwards compatibility.

bobthecow · 2017-09-27T07:03:18Z

src/Mustache/Engine.php

@@ -130,6 +131,9 @@ class Mustache_Engine
     *         // This currently defaults to false, but will default to true in v3.0.
     *         'strict_callables' => true,
     *
+     *         // Treat unknown variables as a failure and throw an exception instead of silently ignore them.


s/ignore/ignoring/

bobthecow · 2017-09-27T07:17:24Z

test/Mustache/Test/ContextTest.php

+        );
+        $context->push($a);
+        $context->find('a.c');
+    }


i'd like to see positive and negative tests. we can probably do it with a single method for "throws exception" and a single method for "doesn't throw exception" and a dataprovider for each.

for the examples below, assume context like this:

[ 'a' => ['b' => 1], 'c' => 1, 'd' => 0, ]

we need coverage for:

finding things that exist doesn't throw, with coverage for top level variables, variables in nested sections, dot notation, and top level variables from inside sections.
e.g.: {{ a }}, {{# a }}{{ b }}{{/ a }}, {{ a.b }} and {{# c }}{{ a }}{{/ c }}, etc.

sections and inverted sections never throw.
e.g.: {{# x }}{{/ x }}, {{^ x }}{{/ x }}, {{# a }}{{# x }}{{/ x }}{{/ a }}, {{# a.x }}{{/ a.x }}, {{# d.x }}{{/ d.x }}, etc.

missing variables inside falsey sections never throw.
e.g.: {{^ a }}{{ x }}{{/ a }} and {{# d }}{{ x }}{{/ d }}, etc.

bobthecow · 2017-09-27T07:18:45Z

test/Mustache/Test/FiveThree/Functional/StrictVariablesTest.php

+ */
+
+/**
+ * @group lambdas


this isn't a lambdas test.

bobthecow · 2017-09-27T07:19:21Z

test/Mustache/Test/FiveThree/Functional/StrictVariablesTest.php

+ * @group lambdas
+ * @group functional
+ */
+class Mustache_Test_FiveThree_Functional_StrictVariablesTest extends PHPUnit_Framework_TestCase


this doesn't need to be a FiveThree test because it doesn't use Closures. move it to Mustache/Test/Functional?

bobthecow · 2017-09-27T07:20:36Z

Thanks for working on this!

The CI issues have been fixed in dev, so if you pull and/or rebase your tests will run.

smarchint · 2017-10-28T04:42:17Z

@LeSuisse hey, any updates on the changes?

LeSuisse · 2017-10-29T18:08:50Z

Hi,

Yeah, sorry I have left this PR unattended, I have been busy/working on other stuff and got a few days off too. I have a deadline mid November so it's unlikely I work on it before that but I intend to fix the issues/remarks spotted by @bobthecow (thank you for the review by the way).

LeSuisse · 2017-12-08T16:21:28Z

I finally got time to update my PR!

@bobthecow Concerning tests I made some changes according to the test cases you given. I'm not sure if that what you were expecting and/or if it is sufficient.

When the strict_variables option is enabled, any encountered unknown variables throw an UnknownVariableException. This option is useful in a development/CI environnement to catch mistakes in templates early instead of silently returning an empty string. This kind of behavior in case of a variable "miss" is accepted by the mustache manual [1]. [1] https://mustache.github.io/mustache.5.html#Variables

bobthecow · 2022-12-03T15:48:42Z

src/Mustache/Compiler.php

+        try {
+            $value = $context->%s(%s);%s
+        } catch (Mustache_Exception_UnknownVariableException $ex) {
+            $value = "";


why aren't sections (and inverted sections) strict?

The PR is a bit old so I have forgotten a bit about it. I will try to deep dive in again next week but this is likely due to some previous comments:
#325 (comment)
#325 (comment)

gotcha. i'll poke around a bit and see if it makes sense to me.

smarchint mentioned this pull request Sep 19, 2017

conditional block variable isset check. #327

Open

bobthecow requested changes Sep 27, 2017

View reviewed changes

LeSuisse force-pushed the feature/strict-variables branch from 5606384 to 8d966dc Compare December 8, 2017 15:59

LeSuisse requested a review from bobthecow December 14, 2019 22:40

bobthecow deleted the branch bobthecow:main December 13, 2021 02:44

bobthecow closed this Dec 13, 2021

bobthecow reopened this Dec 13, 2021

bobthecow changed the base branch from dev to main December 13, 2021 05:05

LeSuisse force-pushed the feature/strict-variables branch from 8d966dc to adbf7cd Compare December 2, 2022 10:32

bobthecow reviewed Dec 3, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a strict_variables option to the Engine #325

Introduce a strict_variables option to the Engine #325

LeSuisse commented Aug 28, 2017

smarchint commented Sep 22, 2017

LeSuisse commented Sep 22, 2017

LeSuisse commented Sep 22, 2017

bobthecow Sep 27, 2017

bobthecow Sep 27, 2017

bobthecow Sep 27, 2017

bobthecow Sep 27, 2017

bobthecow Sep 27, 2017

bobthecow commented Sep 27, 2017

smarchint commented Oct 28, 2017

LeSuisse commented Oct 29, 2017

LeSuisse commented Dec 8, 2017

bobthecow Dec 3, 2022

LeSuisse Dec 3, 2022

bobthecow Dec 3, 2022

Introduce a strict_variables option to the Engine #325

Are you sure you want to change the base?

Introduce a strict_variables option to the Engine #325

Conversation

LeSuisse commented Aug 28, 2017

smarchint commented Sep 22, 2017

LeSuisse commented Sep 22, 2017

LeSuisse commented Sep 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobthecow commented Sep 27, 2017

smarchint commented Oct 28, 2017

LeSuisse commented Oct 29, 2017

LeSuisse commented Dec 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment