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

HandlebarsTemplateEngine: include/inherit is done wrong #514

Closed
mrnejc opened this issue Jan 31, 2017 · 8 comments
Closed

HandlebarsTemplateEngine: include/inherit is done wrong #514

mrnejc opened this issue Jan 31, 2017 · 8 comments
Labels

Comments

@mrnejc
Copy link
Contributor

@mrnejc mrnejc commented Jan 31, 2017

Version

  • vert.x core: 3.3.3
  • vert.x web: 3.3.3

Context

Template files (*.hbs files) are put into templates directory.
When including template partial into master template or doing template inheritance the reference to another template must also include templates path. This is wrong.

Per documentation at jknack.github.io/handlebars.java including another template in same directory shouldn't require providing paths. See example with header.hbs, footer.hbs and home.hbs.

Do you have a reproducer?

Example project at https://github.com/mrnejc/vertx-web-handlebars-test. HandlebarsTest.java has main() method, starting server on port 8080. Visiting http://localhost:8080/partial displays rendered template.

Example has 3 templates in directory src/main/resources/templates/.

The file partial.hbs references main.hbs as {{> templates/main }} but it should be {{> main }} only. main.hbs also references include.hbs with {{> templates/include }} but should be {{> include }}.

Extra

I see that Loader is changed in current master, but can't test because project doesn't build for me.

@pmlopes
Copy link
Member

@pmlopes pmlopes commented Feb 1, 2017

@mrnejc our CI is green and I can build it locally. What are you experiencing?

@mrnejc
Copy link
Contributor Author

@mrnejc mrnejc commented Feb 1, 2017

edit: deleted - I was building it wrong :)

@mrnejc
Copy link
Contributor Author

@mrnejc mrnejc commented Feb 1, 2017

did a compile with -DskipTests=true

loader works same way with 3.4.0-SNAPSHOT

mrnejc added a commit to mrnejc/vertx-web that referenced this issue Feb 1, 2017
@mrnejc
Copy link
Contributor Author

@mrnejc mrnejc commented Feb 1, 2017

The quick and dirty solution I came to:

on 1st call of Loader.sourceAt its called from TemplateHandler, but if template uses inheritance every consecutive call to method is from Handlebars library that only knows relative paths.

So the solution is to save template path on 1st call and prepend it to template filename if needed.

The solution works with default path templates or setting absolute path to templates.

But this is dirty hack and I don't quite like it.
I guess the only problem with this It will fail if initial path is empty String or path is provided with \\ file separator instead of / ...

@pmlopes
Copy link
Member

@pmlopes pmlopes commented Feb 2, 2017

If there are more than 1 root templates then you have no idea to which template the base path applies...

@mrnejc
Copy link
Contributor Author

@mrnejc mrnejc commented Feb 2, 2017

No, I save just the to template path, not including root template name.
If another root template load is requested the passed in path (from TemplateHandler) is the same as the one already stored and thus not changed.

i.e. the fix only applies to templates that are included in master templates (calls from Handlebars library) since they are provided with relative path.

this is why I think the solution is a bit ugly:
Since Loader instance that stores path is created per instance of HandlebarsTemplateEngineImpl, path will only be wrong if user created single instance of HandlebarsTemplateEngine and passes it into multiple TemplateHandler.create(TemplateEngine engine, String templateDirectory, String contentType) calls with different paths in templateDirectory.

@yunyu
Copy link
Contributor

@yunyu yunyu commented Mar 7, 2017

The upstream FileTemplateLoader requires an explicit base path: https://github.com/jknack/handlebars.java/blob/1f6c48e606dc1303d1e92a0a0eaa94120eba64fd/handlebars/src/main/java/com/github/jknack/handlebars/io/FileTemplateLoader.java#L42

Without breaking the current API, it seems like enforcing one in Vert.x is infeasible and that the proposed guessing-on-first-initialize is the way to go. Unless we can change templateEngine to pass in templateDirectory, or alternatively, subtract the string in the Handlebars engine.

@yunyu
Copy link
Contributor

@yunyu yunyu commented Mar 7, 2017

I spent some more time debugging this, and it looks like it won't be too difficult to fix this in HandlebarsTemplateEngineImpl#render using the subtraction method as I mentioned above.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants
You can’t perform that action at this time.