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

httpcaddyfile: Rewrite root and rewrite parsing to allow omitting matcher #5844

Merged
merged 2 commits into from
Jan 15, 2024

Conversation

francislavoie
Copy link
Member

@francislavoie francislavoie commented Sep 30, 2023

I had this idea from reviewing @dunglas's code recently (where he copy-pasted some code from the Caddy codebase itself). The php_fastcgi directive uses RegisterDirective to get full control of parsing, instead of the RegisterHandlerDirective shortcut which does automatic request matcher parsing.

That made me realize we can do the same for root, and that we could actually make root /srv work, instead of the awkward root * /srv with an explicit * matcher) that we've been stuck with since v2.0.

Nice and simple, just a small rewrite of the parsing to count the number of arguments first, and if we have only one argument then we can assume it's the actual root path and that there's no matcher, because it never makes sense to ever only pass a matcher to root and no root path.

Added a test to cover the happy path, and tested the 0 and 3+ argument cases by hand quickly.

Before:

root * /srv

After:

root /srv

🎉

@francislavoie francislavoie added the feature ⚙️ New feature or request label Sep 30, 2023
@mholt mholt added this to the 2.9.0 milestone Oct 3, 2023
@francislavoie francislavoie changed the title httpcaddyfile: Rewrite root parsing to allow omitting matcher httpcaddyfile: Rewrite root and rewrite parsing to allow omitting matcher Oct 14, 2023
@francislavoie
Copy link
Member Author

Realized we can do the same with the rewrite directive, it had the same problem as root.

@francislavoie
Copy link
Member Author

This is good to go, just need an approval 😄

@francislavoie francislavoie enabled auto-merge (squash) January 13, 2024 21:55
Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool, thanks. I initially didn't do this because it's hard to tell the difference between root <matcher> (missing actual root path) and root <path> (implied wildcard matcher).

I think experience has shown that wildcard matchers are by far the most common, which maybe I should have predicted, but anyway; I think this is reasonable to assume that a single arg is the root path.

@francislavoie francislavoie merged commit 5e2f1b5 into master Jan 15, 2024
25 checks passed
@francislavoie francislavoie deleted the root-no-matcher branch January 15, 2024 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature ⚙️ New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants