hogan.js style template inheritance #198

jazzdan · 2014-04-04T19:05:21Z

Modeled after hogan.js, with functional tests for template inheritance copied from the hogan.js test suite.

An additional test I performed was modifying the mustache/spec to have these same template inheritance rules. I then successfully ran the test suites of mustache.php with this patch and the most recent version of hogan.js after pointing them both at my modified mustache spec. It appears that they are both 100% compatible with one another.

This wasn't a solo effort by any means. Much thanks to my coworkers @dtb, @mdg and @benburry at @etsy.

bobthecow · 2014-04-04T19:32:51Z

src/Mustache/Compiler.php

+    {
+        $block = '';
+
+        $real_children = array_filter($children, function($ch) {


This doesn't work on PHP 5.2.x, and Mustache supports 5.2.x until the next major release.

bobthecow · 2014-04-04T19:47:34Z

How feasible would it be to add a pragma for template inheritance, then treat T_PARENT as T_PARTIAL unless it's present? Using pragmas to extend Mustache into non-spec behavior has a long, rich history :)

I understand that this makes templates a bit ugly, but I also really value compatibility with the Mustache spec — and with existing templates of existing users. While template inheritance is awesome, it's not strictly compatible with Mustache. For example, {{$foo}} is a perfectly valid context variable name in vanilla Mustache.

To make it easier for people who just want to use all the awesome all the time, it might make sense to add Mustache_Engine constructor options for enabling pragmas without putting them in all your templates.

bobthecow · 2014-04-04T19:48:51Z

src/Mustache/Compiler.php

+                        $node[Mustache_Tokenizer::NAME],
+                        isset($node[Mustache_Tokenizer::INDENT]) ? $node[Mustache_Tokenizer::INDENT] : '',
+                        $level,
+                        $node['nodes']


This should be $node[Mustache_Tokenizer::NODES]

bobthecow · 2014-04-04T19:59:07Z

test/Mustache/Test/Functional/MustacheSpecTest.php

+    public function loadInheritanceSpec()
+    {
+        return $this->loadSpec('inheritance');
+    }


Is there a way to include these tests even without the modified version of mustache/spec?

Yeah, I accidentally committed this. It can be removed because I added all the tests to the functional test here.

… to be used from array_filter instead

bobthecow · 2014-04-04T20:08:25Z

Looking great so far. Thanks @jazzdan, @dtb, @mdg and @benburry!

…rom the mustache spec

jazzdan · 2014-04-04T21:28:00Z

@bobthecow, implementing template inheritance as a pragma doesn’t work for our use case. Our developers are writing front-end features in hogan.js and sharing templates between the client and the server. If I understand pragmas correct, then implementing template inheritance as a pragma would mean that some templates could no longer be shared between the client (hogan) and the server (mustache.php).

Is it possible that, instead, this change could go out with the next major version? (Being not backwards-compatible)

bobthecow · 2014-04-05T00:28:59Z

@jazzdan Yeah, I hear you about the annoyance of pragmas, but I definitely want to make non-spec features opt-in, at least for now.

First, you should be able to use pragmas and any library that doesn't understand a given pragma will just skip over it. For example, if Mustache.php used {{% INHERITANCE }} as the pragma name, your template would look like this:

{{% INHERITANCE }}
{{< foo }}
{{$ bar }}this is bar{{/ bar }}
{{/ foo }}

In Mustache.php, that would toggle template inheritance and everything would work as expected. But Hogan.js, which doesn't know about the {{% INHERITANCE }} pragma, would treat that tag as a "variable not in the current context stack" and would, effectively, see this:

{{< foo }}
{{$ bar }}this is bar{{/ bar }}
{{/ foo }}

Second, I would consider adding a constructor option to the Mustache_Engine for toggling features at a global level rather than in individual templates:

$m = new Mustache_Engine([
    'enabled_pragmas' => [Mustache_Engine::PRAGMA_INHERITANCE]
]);

Would that make sense?

… of variables. Renamed parent_* to block_*.

bobthecow · 2014-04-09T15:11:01Z

src/Mustache/Parser.php

+                    break;
+
+                case Mustache_Tokenizer::T_BLOCK_VAR:
+                    if ($parent[Mustache_Tokenizer::TYPE] == Mustache_Tokenizer::T_PARENT) {


It seems like this would fail for blocks inside sections inside parent tags:

{{< foo }} {{# bar }} {{$ baz }}{{/ baz }} {{/ bar }} {{/ foo }}

… which probably is a perfectly valid failure case, since the contents of parent tags should really just be the blocks to be overridden. But if that's the case, do we want to throw an exception when parsing if a block arg is ever nested inside something besides a parent tag? Or is it better to let things fail and not tell people why?

(i haven't tested in java or hogan, so i don't know whether this works, is ignored, or throws an exception for either of those)

It does throw an exception in hogan.js:

Hogan = require("hogan.js"); var template = "{{< foo }}{{# bar }}{{$ baz }}{{/ baz }}{{/ bar }}{{/ foo }}"; var foo = "{{$baz}}default content{{/baz}}"; var data = { bar: "set by user" } var template = Hogan.compile(template); var output = template.render(data, {'foo': header}); console.log(output); // Returns /* * /home/dmiller/development/test.js/node_modules/hogan.js/lib/compiler.js:203 * throw new Error('Illegal content in < super tag.'); * ^ * Error: Illegal content in < super tag. * at buildTree (/home/dmiller/development/test.js/node_modules/hogan.js/lib/compiler.js:203:15) * at buildTree (/home/dmiller/development/test.js/node_modules/hogan.js/lib/compiler.js:208:23) * at Object.Hogan.parse (/home/dmiller/development/test.js/node_modules/hogan.js/lib/compiler.js:394:12) * at Object.Hogan.compile (/home/dmiller/development/test.js/node_modules/hogan.js/lib/compiler.js:416:35) * at Object.<anonymous> (/home/dmiller/development/test.js/index.js:11:22) * at Module._compile (module.js:456:26) * at Object.Module._extensions..js (module.js:474:10) * at Module.load (module.js:356:32) * at Function.Module._load (module.js:312:12) * at Function.Module.runMain (module.js:497:10) */

I'll add a throw here for that.

…untered inside of a parent tag. Other small cleanups

…ent tag

bobthecow · 2014-04-14T22:28:24Z

src/Mustache/Template.php

@@ -61,10 +61,13 @@ public function __invoke($context = array())
     * @param mixed $context Array or object rendering context (default: array())
     *
     * @return string Rendered template
+     * TODO construct a parent context stack here and pass it in


So is this TODO a TODONE yet?

It surely is!

bobthecow · 2014-04-15T07:32:11Z

From the GRMustache test cases linked to comments on the spec issue, we should probably make sure we handle cases like this correctly:

  - name: Shenanigans
    desc: Templates can override parent blocks inside sections
    data: {items: [1, 2, 3]}
    template: '{{<parent}}{{$value}}<{{.}}>{{/value}}{{/parent}}'
    partials:
      parent: '{{#items}}{{$value}}ignored{{/value}}{{/items}}'
    expected: '<1><2><3>'

It currently renders as <1><1><1>.

jazzdan · 2014-04-22T03:41:51Z

Hey @bobthecow, I spent some time looking at this test case. I think that in order to support this the template inheritance feature I wrote would have to be rewritten to be lazily evaluated, instead of strictly evaluated. =\ Unfortunately I have run out of time to work on this at work, but I'd welcome any contributions to implement that behavior.

I've confirmed that this test does pass in hogan.

bobthecow · 2014-04-22T04:44:02Z

@jazzdan I've got an idea or two, I'll take a poke at it when I get a minute.

jazzdan · 2014-06-26T00:06:42Z

@bobthecow did you ever get a chance to look at the GRMustache test case? Is there anything we can do to help get this into a release? Thanks for all your help so far, happy to report this has been running in production at @etsy for a few months now with no issues.

bobthecow · 2014-06-27T06:39:30Z

@jazzdan Thanks for the poke. Unfortunately I haven't had the time to take a shot at that last issue.

I'm fine releasing this as-is, with the understanding that it'll most likely change to be lazily evaluated in the future. Since that's the case, I'd prefer to guard the feature with a pragma to make it extra-clear that this isn't part of the spec :)

I've merged your branch and added a pragma in the pragma-blocks feature branch.

I've also added the ability to globally enable pragmas in the Engine, since that seems to be a much more useful way to enable them most of the time... take a look at that and let me know what you think?

If you think this works, I'd like to add some documentation around this specific pragma / feature (including the bit about lazy evaluation), then it should be ready to drop into a release.

jazzdan · 2014-06-30T20:33:12Z

@bobthecow awesome, thanks! I'd be happy to write up a wiki page here on it some time this week.

bobthecow · 2014-06-30T21:47:25Z

That'd be great, thanks :)

jazzdan · 2014-07-03T15:30:36Z

First draft of documentation available here.

bobthecow · 2014-07-09T17:59:58Z

@jazzdan Thanks :)

bobthecow · 2014-08-16T06:19:41Z

Sorry about the delay. This has been merged into dev, and I'm planning to cut a release some time this weekend. Thanks again for everything :)

bobthecow · 2014-08-26T20:11:57Z

Thanks @jazzdan, @dtb, @mdg and @benburry at @etsy for this feature. It has shipped in v2.7.0 :)

hogan.js style template inheritance

db14a6c

bobthecow reviewed Apr 4, 2014
View reviewed changes

referencing nodes by class constant rather than string literal

c6c67ce

bobthecow reviewed Apr 4, 2014
View reviewed changes

php 5.2.x doesn't support closures, so added a private static funtion…

f47a095

… to be used from array_filter instead

Dan Miller added 4 commits April 4, 2014 20:42

removing unnecessary local variable in findFromParent function

31800de

s/new_context/newContext/g

a631de1

removing deprecated code for loading non-existent inheritance tests f…

ebb7fda

…rom the mustache spec

reverting vendor spec change

3c57d5f

bobthecow mentioned this pull request Apr 6, 2014

Implement template inheritance via {{% BLOCKS }} pragma. #130

Closed

Dan Miller added 2 commits April 7, 2014 19:12

DRYing parentArgSection into the section method

63b835b

Separate stacks are necessary for blocks to be resolved independently…

af8a62c

… of variables. Renamed parent_* to block_*.

bobthecow reviewed Apr 9, 2014
View reviewed changes

Dan Miller added 4 commits April 9, 2014 16:15

Throw exception when something other than text or block tags are enco…

d27840f

…untered inside of a parent tag. Other small cleanups

Adding data provider to test legal and illegal syntax inside of a par…

efae429

…ent tag

ensure that text is ignored outside of a block inside of a parent tag

56713e5

Don\'t escape html from blocks, removing unneeded variable assigns

f4e9921

jazzdan mentioned this pull request Apr 14, 2014

Add template inheritance spec mustache/spec#75

Closed

bobthecow reviewed Apr 14, 2014
View reviewed changes

removing old todo

9cb926a

bobthecow added feature labels Jul 28, 2014

bobthecow merged commit 9cb926a into bobthecow:dev Aug 16, 2014

bobthecow removed the pending label Dec 30, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hogan.js style template inheritance #198

hogan.js style template inheritance #198

jazzdan commented Apr 4, 2014

bobthecow Apr 4, 2014

bobthecow commented Apr 4, 2014

bobthecow Apr 4, 2014

bobthecow Apr 4, 2014

jazzdan Apr 4, 2014

bobthecow commented Apr 4, 2014

jazzdan commented Apr 4, 2014

bobthecow commented Apr 5, 2014

bobthecow Apr 9, 2014

bobthecow Apr 9, 2014

jazzdan Apr 9, 2014

bobthecow Apr 14, 2014

jazzdan Apr 15, 2014

bobthecow commented Apr 15, 2014

jazzdan commented Apr 22, 2014

bobthecow commented Apr 22, 2014

jazzdan commented Jun 26, 2014

bobthecow commented Jun 27, 2014

jazzdan commented Jun 30, 2014

bobthecow commented Jun 30, 2014

jazzdan commented Jul 3, 2014

bobthecow commented Jul 9, 2014

bobthecow commented Aug 16, 2014

bobthecow commented Aug 26, 2014

hogan.js style template inheritance #198

hogan.js style template inheritance #198

Conversation

jazzdan commented Apr 4, 2014

Choose a reason for hiding this comment

bobthecow commented Apr 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bobthecow commented Apr 4, 2014

jazzdan commented Apr 4, 2014

bobthecow commented Apr 5, 2014

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 Apr 15, 2014

jazzdan commented Apr 22, 2014

bobthecow commented Apr 22, 2014

jazzdan commented Jun 26, 2014

bobthecow commented Jun 27, 2014

jazzdan commented Jun 30, 2014

bobthecow commented Jun 30, 2014

jazzdan commented Jul 3, 2014

bobthecow commented Jul 9, 2014

bobthecow commented Aug 16, 2014

bobthecow commented Aug 26, 2014