XaminProject / handlebars.php

Handlebars processor for php
331 stars 134 forks source link

Subexpressions don't work with context resolving #80

Closed JustBlackBird closed 9 years ago

JustBlackBird commented 9 years ago

In the current implementation subexpressions do not work with context at all.

I've created several tests to illustrate the problem.

fzerorubigd commented 9 years ago

Its not like that, I think. For example in {{concat a (concat c b)}} with array('a' => 'A', 'b' => 'B', 'c' => 'C') context :

result of concat c b is CB , so the final concat is concat a CB and CB is not in context and so the result is null (for CB), the real result we get is only 'c' and its correct.

I change your tests, If you think this is wrong, please explain it with more example.

Thank you!

JustBlackBird commented 9 years ago

@fzerorubigd you are wrong with your changes. In all my cases concat helper returns strings, not variables names. And that is the main problem -- there is no way to tell subexpressions that you return just strings.

Here is the example:

{{concat (concat a "\"\'[]") b}}

"\"\'[]" is a correct string, but not a correct variable name. If I try to render the template above I get an InvalidArgumentException with "variable name is invalid" message.

Moreover, according to your changes concat helper tries to get variable from the context and if it is not found it uses just it's name. It is the same as render {{var}} to "var" string if the var variable is not found in the context.

fzerorubigd commented 9 years ago

We need a better argument parser and a simple way to detect variables from the "string" inside helpers. In current API, the helper must detect this, but the better way is to pass an Arguments class to helpers (with ability to transform into string for preventing BC break) And this need much more work.

everplays commented 9 years ago

Before going for parsing arguments and stuff like that, I think we can use a warpper class like safe string to make difference between variable names and static strings. On Aug 29, 2014 1:26 PM, "Fzerorubigd" notifications@github.com wrote:

We need a better argument parser and a simple way to detect variables from the "string" inside helpers. In current API, the helper must detect this, but the better way is to pass an Arguments class to helpers (with ability to transform into string for preventing BC break) And this need much more work.

— Reply to this email directly or view it on GitHub https://github.com/XaminProject/handlebars.php/pull/80#issuecomment-53852431 .

JustBlackBird commented 9 years ago

@fzerorubigd, I agree. I think the current problem is tightly related with #74. It seems that all arguments stuff must be done in Handlebars core, not in a helper.

As for subexpressions, they are useless without proper work with context resolving.

JustBlackBird commented 9 years ago

@everplays, we cannot return string wrapper from a helper. Here any wrapper will be converted to string, so the parent helper will know nothing about such wrappers.

everplays commented 9 years ago

That needs to be fixed as well. However, Regardless of #74, we need a way to know what the result of a subexpression would mean. Subexpression might return the text "a". When it is referring to the variable a and when it means the static string a? We can fix this (which fixes this issue as well) by having a wrapper for static string. On Aug 29, 2014 1:38 PM, "Dmitriy S. Simushev" notifications@github.com wrote:

@everplays https://github.com/everplays, we cannot return string wrapper from a helper. Here https://github.com/XaminProject/handlebars.php/blob/master/src/Handlebars/Template.php#L359 any wrapper will be converted to string, so the parent helper will know nothing about such wrappers.

— Reply to this email directly or view it on GitHub https://github.com/XaminProject/handlebars.php/pull/80#issuecomment-53853357 .

fzerorubigd commented 9 years ago

I don't know exactly, How handlebars.js handle this? for example in concat a b how we can detect if this is plain a or variable name a? or simple we could use concat "a" "b" and concat a b the first for string and other for variable?

JustBlackBird commented 9 years ago

Here is Handlebars.js specs for subexpressions: https://github.com/wycats/handlebars.js/blob/master/spec/subexpressions.js

As I can see arguments in subexpressions are the same as in normal helpers:

everplays commented 9 years ago

https://github.com/wycats/handlebars.js/blob/f4b8c5260c203d6ab7631fa394696f5c7a442586/spec/subexpressions.js#L125 is what we need which, from my understanding, is what I have described.

@fzerorubigd, @JustBlackBird What do you think?

JustBlackBird commented 9 years ago

@everplays, as I can see helpers always return strings, not variables names.

@fzerorubigd I found one more problem with tests. Here the test helper does not resolve variable name, but it should to! The correct helper's body is:

return $context->get($arg) . 'Test.';

As the result the tests are false positive, because they describe incorrect behavior.

@everplays, @fzerorubigd May be I should create a separate issue for discussion?

fzerorubigd commented 9 years ago

@JustBlackBird yes. Please create a seperate issue for that one too. Thank you!