XaminProject / handlebars.php

Handlebars processor for php
331 stars 134 forks source link

Extra arguments escaping #63

Closed JustBlackBird closed 10 years ago

JustBlackBird commented 10 years ago

There is a problem with unneeded extra escaping of helpers arguments.

If one want to pass string with a character that must be escaped to a helper he should use a code like the following:

{{someFakeHelper "It is a \"correct\" string."}}

But someFakeHelper cannot use its argument. Due to wrong escaping it will receive a string "It is a \\"correct\\" string.". \Handlebars\Template::parseArguments method applied for the string returns the following arguments:

Thus there is no way to specify string arguments with characters escaping in a template.

I've attach a pull request with a test for the problem.

JustBlackBird commented 10 years ago

Also i can add a slash stripping to \Handlebars\Template::_handlebarsStyleSection method but i am not sure about this approach.

everplays commented 10 years ago

I think I have introduced the bug with d719bd12cafe0713b5dec8a52496efb427a13737 and/or 4e0ed3c7ae6eefb418093a2f5c7089334c6389bd commits. Could you please have a look?

On Fri, Jun 20, 2014 at 7:06 PM, Dmitriy S. Simushev < notifications@github.com> wrote:

Also i can add a slash stripping to \Handlebars\Template::_handlebarsStyleSection method but i am not sure about this approach.

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

regards, behrooz

JustBlackBird commented 10 years ago

I've tried to trace the bug and it seems that extra slashes are added here: https://github.com/XaminProject/handlebars.php/blob/master/src/Handlebars/Tokenizer.php#L131. Thus d719bd12cafe0713b5dec8a52496efb427a13737 is the commit that introduces the bug.

I can say nothing about 4e0ed3c7ae6eefb418093a2f5c7089334c6389bd. May be some other test case can confirm that it introduces similar bug with escaping.

everplays commented 10 years ago

Thanks for the test case @JustBlackBird, I got it fixed. However, I changed the test a bit from

$this->assertEquals('a \"b\" c', $engine->render('{{argsTest "a" "\"b\"" \'c\'}}', array()));

to

$this->assertEquals("a \"b\" c", $engine->render('{{{argsTest "a" "\"b\"" \'c\'}}}', array()));

(single quote to double quote and {{ tag to {{{ to avoid html escaping).

JustBlackBird commented 10 years ago

Cool, thank you for quick fix @everplays.