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

Allow file_server browse templates to use the same functions as templates #3637

Closed
francislavoie opened this issue Aug 5, 2020 · 17 comments · Fixed by #4093
Closed

Allow file_server browse templates to use the same functions as templates #3637

francislavoie opened this issue Aug 5, 2020 · 17 comments · Fixed by #4093
Labels
feature ⚙️ New feature or request good first issue 🐤 Good for newcomers help wanted 🆘 Extra attention is needed
Milestone

Comments

@francislavoie
Copy link
Member

See the discussion here: https://caddy.community/t/v2-http-handlers-templates-functions-in-file-server-browse-template/9214

The file_server directive has a browse option which enables a directory index to be served when a directory is requested. By default, this index is rendered using a template which can be found in the code. The default can be overridden with a template provided by the user.

It seems that the file_server browse template doesn't use the same code paths as the templates directive, which means that the functions documented here cannot also be used.

The code should be refactored such that the logic for the templates is shared between these two such that all the template functions Caddy provides works both for the general templates and for the file server directory index.

@francislavoie francislavoie added feature ⚙️ New feature or request good first issue 🐤 Good for newcomers labels Aug 5, 2020
@francislavoie francislavoie added this to the 2.x milestone Aug 5, 2020
@mholt mholt added the help wanted 🆘 Extra attention is needed label Aug 5, 2020
@divbhasin
Copy link

Hi, new contributor here 👋. I can work on this issue.

@francislavoie
Copy link
Member Author

Cool! Go for it @divbhasin 😄

@divbhasin
Copy link

Here is what I am thinking: I need to make the executeTemplateInBuffer function in templates/tplcontext.go accessible to fileserver browse. This is because that function attaches the other functions like httpInclude to a template, and returns a template that has been executed on a buffer.

Am I on the right track @francislavoie?

@francislavoie
Copy link
Member Author

Yeah, that sounds about right. Maybe the templateContext should be shared to allow browse to use it.

@caddyserver caddyserver deleted a comment from vojkuv Aug 30, 2020
@caddyserver caddyserver deleted a comment from vojkuv Aug 30, 2020
@mholt
Copy link
Member

mholt commented Aug 31, 2020

Hmm, this is tricky though because in order to support of all of templates functions you'd need to import the modules/caddyhttp/templates package and use its Templates type, because templateContext depends on it.

This will need more consideration/discussion; I think there'll need to be some sort of new function or method that is exported to wrap all the unexported logic needed for this, without having to create a whole HTTP handler or something.

@divbhasin
Copy link

@mholt yeah I have been struggling with how to incorporate the Templates type into browse.

@0xbzho
Copy link

0xbzho commented Feb 15, 2021

Until this is resolved, what's the best workaround? A fork? A Caddy module? Thanks.

@princemaple
Copy link

@0xbzho I think this one is just waiting for a proper PR. If you have the capability to make a working fork or a functionally equivalent module, maybe just send a PR.

@francislavoie
Copy link
Member Author

There's an open but incomplete PR here: #3982

If you wish to help finish it, feel free to contribute.

@princemaple
Copy link

FYI all, #3982 (the PR) is closed due to inactivity.

@xdu31
Copy link
Contributor

xdu31 commented Mar 28, 2021

Hi @francislavoie and @princemaple , I can help on this, I actually already have patches locally. If this feature is still needed, I can raise a PR in a few days. I have been working with Caddy in my recent project.

@francislavoie
Copy link
Member Author

Yes, PRs welcome 👍

@xdu31
Copy link
Contributor

xdu31 commented Apr 1, 2021

Yes, PRs welcome 👍

Hi @francislavoie, going to provide the detailed test cases done on the PR and the test caddyfile soon.

@xdu31
Copy link
Contributor

xdu31 commented Apr 2, 2021

Yes, PRs welcome 👍

Hi @francislavoie, going to provide the detailed test cases done on the PR and the test caddyfile soon.

@francislavoie @mholt #4093 (comment)

@xdu31
Copy link
Contributor

xdu31 commented Apr 3, 2021

Yes, PRs welcome 👍

Hi @francislavoie, going to provide the detailed test cases done on the PR and the test caddyfile soon.

@francislavoie @mholt #4093 (comment)

@francislavoie @mholt @princemaple #4093 (comment)

@princemaple
Copy link

I'm not a maintainer :) Just a random guy patiently waiting on this feature.

@xdu31
Copy link
Contributor

xdu31 commented Apr 3, 2021

I'm not a maintainer :) Just a random guy patiently waiting on this feature.

@mholt @francislavoie I tested with both html & json modes, also included a test on the templates plugin.

@francislavoie francislavoie modified the milestones: 2.x, v2.4.0 May 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request good first issue 🐤 Good for newcomers help wanted 🆘 Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants