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

include doesn't seem to work in a theme #324

Closed
sebtombs opened this issue Jun 25, 2018 · 16 comments
Closed

include doesn't seem to work in a theme #324

sebtombs opened this issue Jun 25, 2018 · 16 comments
Labels
done in pr Already done in a PR enhancement

Comments

@sebtombs
Copy link

I am attempting to migrate a Bolt site to be static due to poor database performance by the hosting provider for the site. The reason I've picked Gutenberg rather than Hugo is because Tera is Twig-like, so it makes it easier (and also I much prefer Tera/Twig syntax).

However, I've hit a snag. The Bolt templates I am starting with use a hierarchy of partial files. Having changed the suffices and sorted out the syntactic differences between Bolt-extended Twig and Tera, I am still getting the following error:

Reason: Failed to render 'boltlike/templates/kb.html' (error happened in a parent template)
Reason: Template 'partials/_topbar.html' not found

Now, partials/_topbar.html is included into partials/_master.html, which is itself extended by kb.html. If I move the include to kb.html, or put the included file in the theme templates directory rather than partials subdirectory, I get the same issue.

So, "extends" seems to work, but "include" doesn't. In the three sample themes (book, even, and hyde) that I've looked at, "include" isn't used.

However, if I put _topbar.html in the actual templates directory, rather than the templates directory in the theme, all works as expected. So the problem is only present when the include destination is in a theme.

This is no big deal for me at the moment, as I can just set things up in the template, but may catch others out who want to use include in a theme.

@Keats
Copy link
Collaborator

Keats commented Jun 25, 2018

It is mentioned in https://www.getgutenberg.io/documentation/themes/creating-a-theme/#caveat
The main reason is that include can be anywhere in the AST and I didn't want to update the code that rewrites import to be recursive because of laziness. I'd happily take a PR for it though

@Keats
Copy link
Collaborator

Keats commented Jul 17, 2018

If anyone is interested in fixing it, it should be in components/utils/templates.rs:rewrite_theme_paths but it will need to walk the AST recursively. Not very hard and I might do it myself after the 0.4 release but that would be a nice first issue for someone interested in contributing

@sebtombs
Copy link
Author

Apologies for not reading the documentation properly, and for my tardy response. I hope to work on this issue some time in the next few weeks - not quite there yet with my site conversion which needs to come first.

@Keats
Copy link
Collaborator

Keats commented Jul 30, 2018

No worries, it can be worked on any time

@southerntofu
Copy link
Contributor

After some testing, i believe this fixes the issue. Could people confirm or point out problems with this approach? (tests pass, i use it on some sites)

@Keats
Copy link
Collaborator

Keats commented Apr 22, 2020

This seems a bit too simple? I'm happy to be proven wrong but it's weird that the code ends up that simple

@southerntofu
Copy link
Contributor

southerntofu commented Apr 22, 2020

I had the same "that can't be it" feeling at some point, but this doesn't seem to break anything on my sites though i'm not doing anything crazy on there.

Makes sense because tera.extend places templates in the same namespace, which is why the whole function was needed in the first place. So if includes are in the same namespace, no reason includes couldn't work across theme/site boundaries

PS: I actually tested an include in a theme and it just worked. I ended up replacing that with a macro for 0.10.1 compatibility though. But maybe i've missed something so please point it out if that's the case

@Keats
Copy link
Collaborator

Keats commented Apr 23, 2020

I see what's happening. Right now the theme system allows you to override just a block from a theme template:

{% extends "a-theme/templates/page.html" %}

{% block content %}
{{ super() }}
SOMETHING ELSE
{% endblock content %}

It is essentially inheriting from a theme template, which is why the code is complex right now. In your commit, it works fine if you're replacing the whole file, but you lose the ability to override just one block. Whether it's valuable or not, I don't know - I don't use themes.
See https://www.getzola.org/documentation/themes/installing-and-using-themes/#customizing-a-theme for the docs.

@sebtombs
Copy link
Author

As is obvious from the delay since I submitted this issue, my plan to do something about it got derailed by other priorities. However, I can say that the proposed fix would appear to break our use case where we do override blocks within theme templates as described in the post above.

@southerntofu
Copy link
Contributor

Hi @sebtombs and thanks for the feedback. Could you take a look at #1004 and see if this new patch respects your usecase?

@sebtombs
Copy link
Author

Hi @southerntofu. Doesn't the test case in templates/section.html need to still have a {% block content %}? It looks to me like the new test case is proving that the site template is extended correctly with the one in the theme, not that the site template can correctly extend that in the theme. I think the latter was the intent. @Keats please feel free to correct me if I'm wrong.

@Keats
Copy link
Collaborator

Keats commented Apr 23, 2020

I don't think the patch will work for the usecase I mentioned.

@southerntofu
Copy link
Contributor

I edited the testcase in a new commit. I believe it now matches what you described.

@sebtombs
Copy link
Author

@southerntofu, I think the first change (5d2...) did, but the second one to include super() (3fc...) isn't quite right. I think what @Keats meant was to have something in the site template content block to show that it has been picked up AND a super() to show the theme template can still be used. It sounds like your new code does work from your earlier tests but we just need a test case that shows both override and inheritance still work so that any future changes are also properly tested.
So, perhaps have the page loop in the site template and the sub-section loop in the theme one?

@southerntofu
Copy link
Contributor

Thanks @sebtombs that was valuable advice. Sorry it took me a while to figure out what you meant.

@Keats
Copy link
Collaborator

Keats commented Jun 18, 2020

It will work in the next version thansk to @southerntofu !

@Keats Keats added done in pr Already done in a PR and removed good first issue help wanted labels Jun 18, 2020
@Keats Keats closed this as completed Sep 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
done in pr Already done in a PR enhancement
Projects
None yet
Development

No branches or pull requests

3 participants