search

XaminProject / handlebars.php

Handlebars processor for php
331 stars 132 forks source link

Fix context switching for mustache blocks #164

Closed mAAdhaTTah closed 7 years ago

mAAdhaTTah commented 7 years ago

When you switch into a mustache block, an object shouldn't be iterated over. Associative arrays are closer to objects than lists, so their behavior should be more like objects.

This is a bit of a strawman proposal, as there's clearly some prior art here that I'm not fully aware of, but it is an inconsistency between Handlebars.php and Mustache.php/Handlebars.js. It also does appear that Handlebars.js won't iterate over objects, although I can't seem to find whether objects are support to be iterated over in the spec. It's up for debate whether an associative array is more like an object or an array in other languages, and this does fail another test which I don't know how to resolve.

everplays commented 7 years ago

@mAAdhaTTah it would be useful if you provide a snippet that shows the behavior in mustache or handlebars.

Regarding the failing test, that is testing exactly the opposite of what you are trying to achieve here. So if really mustache does not iterate over objects, that test is wrong and should be removed.

mAAdhaTTah commented 7 years ago

I've created a repo here that demonstrates the issue.

I can remove the failing test, since it is, as you say, testing the opposite of what I want. I just wanted to bring it up because changing the behavior in this way is technically backwards-breaking, on the off chance anyone is expecting the Mustache blocks in Handlebars.php to render in that way. That said, I would expect anyone using Handlebars.php to use the if / each helpers, rather than a plain Mustache, which would avoid the issue found here, and would also explain why this incompatibility hasn't been found earlier..

everplays commented 7 years ago

I agree. Using helpers is more explicit and makes the intentions more clear. If you fix the tests, I'll merge it in.

mAAdhaTTah commented 7 years ago

Ok, updated tests are passing. I refactored the _mustacheStyleSection method to clean up some of the logic; I borrowed isIterable from Mustache.php (renamed to _checkIterable to match the code style). Let me know if you have any changes/suggestions.

everplays commented 7 years ago

Thanks for the PR, @mAAdhaTTah.