Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix #305: do render the value "0" inside conditional #306

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Krinkle
Copy link
Contributor

@Krinkle Krinkle commented Apr 2, 2019

Fixes issue #305.

@@ -151,7 +151,7 @@ public static function lo($cx, $v) {
public static function v($cx, $in, $base, $path, $args = null) {
$count = count($cx['scopes']);
$plen = count($path);
while ($base) {
while ($base !== null) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When the conditional block processes the path foo from the parent context with array( 'foo' => '0' ) the logic for deciding whether the value is "real" is on line 182, where it only has to pass isset().

The isset() function basically performs to checks: The variable/property/key exists, and has a non-null value.

This successfully unwraps the value '0' from array( 'foo' => '0' ).

The string '0' is then passed into the conditional closure, and Runtime::v() is called again when evaluating the placeholder {{foo}}. This time it is no longer a sub property, and it now implicitly checked by this while statement, which rejects all falsey values in PHP. Including many that isset() accepts. Such as: boolean false, number 0 and string '0'.

@Krinkle Krinkle force-pushed the fix-zero branch 2 times, most recently from 4bceeee to 9e8ab9d Compare April 2, 2019 18:36
@Krinkle
Copy link
Contributor Author

Krinkle commented Apr 2, 2019

Build failure is unrelated, see #307.

@zordius
Copy link
Owner

zordius commented Apr 6, 2019

Hello, This pull request breaks a handlebars.js spec test, which was definded in handlebars-lang/handlebars.js#731 . Refer to these jsfiddle:

handlebars.js 4.1.1: https://jsfiddle.net/0rec6d2f/
mustache.js 3.0.1: http://jsfiddle.net/4nLockuy/

Here are some mustache <=> handlebars difference, you can refer to these document:

About block:
https://zordius.github.io/HandlebarsCookbook/0008-block.html#BlockforFalse (search for 'Only mustache.js will think 0 as false')

About #if:
https://zordius.github.io/HandlebarsCookbook/0016-if.html (search for '0 means false')

If you like to follow mustache, the FLAG_MUSTACHE is good, you may also check the document https://github.com/zordius/lightncandy , search 'Mustache Compatibility' for more detail.

If you like to follow handlebars.js, you should use FLAG_HANDLEBARS or check the document https://github.com/zordius/lightncandy , search 'Handlebars Compatibility' for more detail.

@Krinkle
Copy link
Contributor Author

Krinkle commented Apr 6, 2019

@zordius This is about the string '0', not the number 0.

The if logic is already working correctly in lightncandy. When passing data [ 'foo' => '0' ] to {{#foo}}Yes{{/foo}} the word Yes renders correctly in lightncandy.

The problem is in the internal lightncandy logic for getting a value from a path, in ::v().

The example template:

{{#foo}}<b>{{foo}}</b>{{/foo}}

The example data:

'data' => [
  'foo' => '0',
],

When ::v() is called by the processing of {{#foo}}, the following happens:

  • $data is [ 'foo' => '0' ]
  • $path is [ 'foo' ] (this could also be ['foo','bar',baz'] for {{#foo.bar.baz}}).
  • Inside ::v() there is a loop over $path, it is protected by a PHP if statement for $base. It sees the array as "true".
  • Outcome: It is able to return the value '0'.
  • Then, the logic for template parsing decides whether '0' is true for HTML template (not PHP). That logic, decides '0' is true.

Then, for <b>{{foo}}</b> we also use ::v(). There we have the bug.

  • $data is [ '0' ]
  • The protection PHP if statement for $base stops immediately, we never look. This means the Mustache and Handlebar logic does not have an opportunity to decide.

The value is never found, and never printed. We get <b></b>.

I might need help for a better fix. Do you have a recommendation?

@zordius
Copy link
Owner

zordius commented Apr 6, 2019

Well, please refer to #305 , we compared handlebars.js and mustache.js there, again.

Now the handlebars.js behavior is strange but lightncandy is aligned with it. Here are some points:

  1. A better fix should works on block/section context handling.
  2. An issue/bug should be also created at handlebars.js repo to address this issue.
  3. Based on 2, if handlebars.js will be fixed for this, then we can fix lightncandy in better way.
  4. Based on 2, if handlebars.js will not change for this, then the fix will be harder because we can only fix this for mustache flag but not handlebars flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants