groue / GRMustache

Flexible and production-ready Mustache templates for MacOS Cocoa and iOS
http://mustache.github.com/
MIT License
1.44k stars 190 forks source link

Conditional expression always returns true for tokens named the same as a filter #82

Closed zygoat closed 10 years ago

zygoat commented 10 years ago

In one of our templates we happen to have a token named zip. Since it may not necessarily be present in the source dictionary passed to our template, we render it conditionally using the syntax {{#zip}}{{zip}}{{/zip}}. (We have used this construct all over the place for a long time.)

After upgrading to GRMustache 7.2.0, I started seeing the literal text <GRMustacheBlockVariadicFilter: 0x178b0160>, rather than nothing, in cases where the zip token was unset.

I have worked around this by setting a nominal (non-"standard library") baseContext in GRMustacheConfiguration. However, I question whether the behaviour I'm seeing is intentional, or can be construed as a bug.

In my opinion, the logic test for {{#zip}} should evaluate to false if no such key exists, regardless of whether there exists a similarly-named filter. I would expect a logic test for the availability of such a filter to be expressed as {{#zip()}} or such—in the same way that presumably the presence of parentheses distinguishes {{zip}} and {{zip(foo, bar)}} as being a key and a filter, respectively.

Otherwise, it becomes incumbent on the user to audit all of his mustache templates and examine the "standard library" for potential name collisions after every GRMustache release, which is clearly untenable.

thanks,

b

groue commented 10 years ago

Otherwise, it becomes incumbent on the user to audit all of his mustache templates and examine the "standard library" for potential name collisions after every GRMustache release, which is clearly untenable.

True. My apologies for having played with the default library without having seen this problem coming.

I'm happy you were able to find the work-around.

In my opinion, the logic test for {{#zip}} should evaluate to false if no such key exists, regardless of whether there exists a similarly-named filter.

I could, I guess, modify the algorithm of expression evaluation somehow, and have zip give a different value in the {{# zip }} tag, and in the {{# zip(...) }} tag.

However this would not generally fix the issue with the standard library. It is not only made of filters. Take the localize key, for example. It can be used as:

So...

I have been wrong messing with the standard library. I should indeed have bumped the major version, since running templates suddenly stopped working. If there is a bug, this is it.

Hmmm. I don't quite know yet what to do.

groue commented 10 years ago

Looking for solutions that prevent standard library keys to conflict with user keys:

  1. with a reserved prefix for standard library keys: {{# @each(...) }}...{{/}}, {{ @localize(...) }}, etc. @ is arguably the best "reserved" character because the each filter already emits @index, @first, etc. keys.

    con: existing templates that use standard library have to be rewritten

    pro: future-proof

  2. with a root object: {{# GRMustache.each(...) }}...{{/}}, {{ GRMustache.localize(...) }}.

    con: existing templates that use standard library have to be rewritten

    con: GRMustache name limits portability, should any other Mustache engine adopt a GRMustache-like standard library.

    con: @ as in @.each is too close from @each (see above) for being interesting.

    pro: future-proof

    pro: Users can already do this via:

    template.baseContext = [GRMustacheContext contextWithObject: @{@"GRMustache": [GRMustache standardLibrary]}];
  3. with APIs for optional loading of standard library keys.

    con: Now a user key zip prevents accessing the zip filter.

  4. with APIs for optional renaming of standard library keys.

    con: This limits portability, should any other Mustache engine adopt some GRMustache standard library keys.

  5. by removing the standard library, but providing the utilities as sample code or built-in classes.

    pro: future-proof

    con: existing code that renders template that use standard library has to be rewritten

More thinking on the way...

groue commented 10 years ago

There is another compatibility issue (#83).

I advise you to keep using GRMustache v7.1.0. GRMustache v7.2.0 will be flagged as a rotten version. Sorry for the inconvenience.

groue commented 10 years ago

The zip filter has been removed from v7.3.0. Please upgrade!