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

Partial lookups no longer take into account paths. #84

Closed
keeto opened this issue Jul 14, 2014 · 10 comments
Closed

Partial lookups no longer take into account paths. #84

keeto opened this issue Jul 14, 2014 · 10 comments

Comments

@keeto
Copy link
Contributor

keeto commented Jul 14, 2014

Upon checking the current head, the following no longer works:

{{> partial/some-partial}}

when compiled with the following:

$x = LightnCandy::compile(
  $templateStr,
  array(
    'flags' => LightnCandy::FLAG_ERROR_EXCEPTION | LightnCandy::FLAG_STANDALONE | LightnCandy::FLAG_HANDLEBARSJS | LightnCandy::FLAG_SPVARS,
    'basedir' => array(
       'some_path/to/templates'
    )
);

The file some-partial is located in some_path/to/templates/partials/some-partial. This used to work some time ago, but now silently fails to compile. Is this new behaviour?

@madsleejensen
Copy link

hi keeto i just created the same issue (#83) i temporary rolled back to a earlier commit.

@zordius
Copy link
Owner

zordius commented Jul 14, 2014

will focus on #83 .

@zordius zordius closed this as completed Jul 14, 2014
@zordius
Copy link
Owner

zordius commented Jul 14, 2014

Now this is fixed you may refer to #83 , thanks.

@keeto
Copy link
Contributor Author

keeto commented Jul 15, 2014

I'm still having issues with this. :(

This used to work:

template.hbs:

{{> somedirectory/something.hbs}}

somedirectory/something.hbs:

{{> ./hello/something_else.hbs}}

somedirectory/hello/something_else.hbs:

Hello World

Should this still be supported behaviour?

@zordius
Copy link
Owner

zordius commented Jul 15, 2014

Well ...... Partials are logical pieces in mustache/handlebars . For example, when it be executed at browser / client side / none file system environments , you can not expect it auto turn into file system naming logic. You can organize partial in different directories . In handlebars.js the partial works just like this way:

Handlebars.registerPartial('foo', '123 ...{{test}}');

When you {{>foo}} it works very well, but you can not use {{>./foo}} , because it means find a partial named as './foo' . You may test the behavior here: http://jsfiddle.net/8735c/ , try to change {{>foo}} to {{>./foo}} .

So, should we extend the partial support to 'relative path resolve on file system' ? Basically it can be done in lightncandy , maybe it works in old version of lightncandy , but it is not standard.....supporting on it may cause problems when you rendering at client side or using other handlebars implements.

@zordius
Copy link
Owner

zordius commented Jul 15, 2014

My plan is stopping support the relative path . If it works, the behavior is not guaranteed.

@keeto
Copy link
Contributor Author

keeto commented Jul 15, 2014

I'd rather have it as an option, or a flag perhaps.

@zordius
Copy link
Owner

zordius commented Jul 16, 2014

Hmmmm........

  1. when you like to use {{>.foo}} to lookup {{foo}} , it will point to same partial file because of PHP / file system nature ( /some/path/foo.tmpl == /some/path/./foo.tmpl ) . lightncandy supports this just because file_get_contents() and file_exists() does.
  2. If you use {{>.foo}} and {{foo}} in the same time , it works well and use the same file. But....
    • lightncandy build 2 partial entries for the same file.
    • if you register 2 different content for these two partial names in handlebars.js , then you get different output results between lightncandy and handlebars.js
  3. When you {{>path/foo}} and it said {{>bar}} , it means somewhere/bar.tmpl , but not somewhere/path/bar.tmpl . What you want is PHP / include() / require() behavior, but not for handlebars.js . If we like to support it.....
    • Question 1: {{>path/foo}} said {{>bar}} , and {{>another/tee}} said {{>bar}} , they mean same bar , or they want two different bar as path/bar and another/bar ? Here we hit 1 definition issue.
    • Question 2: {{../../..foo}} means somewhere on your system , it may be a security hole: someone may use some path to overwrite PHP file by the way?
  4. If you just want to get path/bar in path/foo by {{>bar}} , you may use multiple basedir in lightncandy option:
LightnCandy::compile($template, Array(
    'flags' => LightnCandy::FLAG_STANDALONE,
    'basedir' => Array(
        '/usr/local/share/handlebars/templates',
        '/usr/local/share/handlebars/templates/path1',
        '/usr/local/share/handlebars/templates/path2',
    )
));

Then {{>bar}} will get first hit of /usr/local/share/handlebars/templates/bar.tmpl , /usr/local/share/handlebars/templates/path1/bar.tmpl , /usr/local/share/handlebars/templates/path2/bar.tmpl . But, you really need to deal with partial name very carefully to prevent name conflicted.

If this solution can not resolve your use case, may you show me more about your scenario ? Thanks.

@keeto
Copy link
Contributor Author

keeto commented Jul 16, 2014

Actually, I just realized that this is a pretty specific use-case for us. It's a bit too complicated to explain why we're in this situation, but it boils down to fancy file resolution that we want.

I think it'll be unreasonable for me to ask for a pretty specific solution, since this is a very specific problem that we have. One way we can solve this is to subclass LightnCandy and override the necessary lookup methods. This would work, but it'll be a bit of a complication if you decide to change any of the APIs.

It would be really awesome if we could refactor readPartial into two different methods: resolvePartial and compilePartial. The resolvePartial method would just take care of resolving the partial name to a filename, and compilePartial will use that filename to read the template file and compile it.

resolvePartial should receive 3 arguments: $partialName, &$context and $resolvedImportingPartialPath. With this API, we can override a single method and put it some different logic for the resolution.

@zordius
Copy link
Owner

zordius commented Jul 16, 2014

Ya good, I will work on this enhancement later , thank you for the specific design.

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

No branches or pull requests

3 participants