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

Add a template_path option to Mojolicious::Renderer::render #1782

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

mkende
Copy link
Contributor

@mkende mkende commented May 10, 2021

Add a template_path option to Mojolicious::Renderer::render that allow to pass the path to a specific template file.

Summary

Currently only templates found inside directories registered with Mojolicious::Renderer::paths can be used. This PR allows to use arbitrary template files.

Motivation

This is useful to reference template files that may have clashing names within different directories (so using relative paths from registered directory would be ambiguous).

In my case, I want to allow my user to provide path to plugins directories (themes for a website) that may each contain a "template/view.html.ep" files for example (so they would all look the same if added the the renderer paths).

References

PR #1780 is actually a work-around for the same feature request (loading an arbitrary file in memory and then rendering it as an inline template with a layout). While I think that the other PR has merit on its own, this here is actually a more straightforward solution to my problem which has the advantage of not changing an existing API so it should not break existing users.

…w to pass the path to a specific template file.
@mkende
Copy link
Contributor Author

mkende commented May 17, 2021

Hi Mojolicious maintainer

Even if you don’t have the time to review this now, could you tell me if this PR seems reasonable and whether it could accepted or if I must find a different approach?

Thanks,

@kraih
Copy link
Member

kraih commented May 17, 2021

Since this introduces a new reserved stash value the PR is at the very least incomplete.

@Grinnz
Copy link
Contributor

Grinnz commented May 17, 2021

How does this interact with the template cache? Is it protected from accessing files outside of registered template directories?

@mkende
Copy link
Contributor Author

mkende commented May 17, 2021

The new reserved stash value is now documented in Mojolicious/Controller.pm, let me know if it should be added somewhere else.

As far as I can tell, the interaction with the cache is reasonable: templates specified with template_path will be cached using their (relative or absolute) path as the cache key. In theory this could conflict with a template specified with template (or with an implicit template) but this seems improbable (it would require for example that the current working directory is added to the registered template directories when using this new feature).

If needed, that can probably be fixed by adding some prefix to the name returned by Mojolicious::Renderer::template_name() for these templates specified by this new mechanism.

@mche
Copy link

mche commented May 20, 2021

How does this interact with the template cache? Is it protected from accessing files outside of registered template directories?

Template cache names would be an full_path_to_template_file instead of file name currently. There are many things from old times that need requirements to rewrite template cache names.

@mkende
Copy link
Contributor Author

mkende commented May 20, 2021

Template cache names would be an full_path_to_template_file instead of file name currently. There are many things from old times that need requirements to rewrite template cache names.

Do you have examples of something that requires being fixed? If there are issues with the PR I can try to improve it but I’m not sure that I understand the problem that you describe.

@mche
Copy link

mche commented May 24, 2021

Template cache names would be an full_path_to_template_file instead of file name currently. There are many things from old times that need requirements to rewrite template cache names.

Do you have examples of something that requires being fixed? If there are issues with the PR I can try to improve it but I’m not sure that I understand the problem that you describe.

#1630

@mkende
Copy link
Contributor Author

mkende commented May 24, 2021

#1630

Can you elaborate on what you mean? As far as I understand, templates are cached by names today and this PR does not change that. For templates referenced with template_path instead of template the name will be the full path to the template which has a very low risk of colliding with another template.

It is possible that this PR could actually help the author of that #1630 question. Even if it does not, I still don’t understand what features could be broken by this PR .

@mche
Copy link

mche commented May 31, 2021

PR does not change that

And would change that.

@mkende
Copy link
Contributor Author

mkende commented May 31, 2021

Are you saying that this existing issue of cache key collision should be fixed in this PR? Otherwise I’m not sur what you mean.

As far as fixing that bug, I don’t think that I would know how to fix it correctly and it is also mostly independent of this PR. I suppose that a discussion should also happen on whether fixing it is worth it. Given that the condition to trigger that bug – dynamically altering the template directory – seems like a very specific usage pattern, the answer to that discussion is not obvious to me and I’m not the right one to lead this discussion.

So, to go back to my PR, I guess the questions that I amm asking you (the Mojolicious maintainers) are the following ones:

  • is my feature request (being able to render arbitrary template) worth being implemented?
  • if so, is this PR a reasonable implementation of this feature request?
  • does it introduce new bugs or issues, or make it more difficult to fix existing issues?
  • am I missing other questions that could prevent this PR from being merged?

Does that sound reasonable? What is needed to move forward with this PR (or to close it if it is not judged useful)?

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

Successfully merging this pull request may close these issues.

None yet

4 participants