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

Regular expression support for simple rewrite rule #2082

Merged
merged 3 commits into from Apr 15, 2018

Conversation

Projects
None yet
5 participants
@abiosoft
Copy link
Collaborator

abiosoft commented Mar 24, 2018

1. What does this change do, exactly?

  • Adds regexp support for simple rewrite rule.
  • Also includes an optional not syntax to negate the regex. Negating regex can be pain for an average user and Go's regex support makes it even more difficult. I think an optional not syntax should not hurt.

Reason:

  • The simple rewrite rule is too basic and it is rarely used.
  • Many rewrite config blocks are only used for regexp.
  • Users that do not need if conditions do not need to open to config block, hence a cleaner Caddyfile.

Outcome:

Sample wordpress config can become a one liner.

rewrite not ^/wp-admin {path} {path}/ /index.php?{query}

2. Please link to the relevant issues.

No issues reported. Noticed the trend from users' Caddyfiles.

3. Which documentation changes (if any) need to be made because of this PR?

Rewrite:

Indication of regexp support and an additional option not field.

rewrite [not] from to... 

4. Checklist

  • I have written tests and verified that they fail without my change
  • I have squashed any insignificant commits
  • This change has comments for package types, values, functions, and non-obvious lines of code
  • I am willing to help maintain this change if there are issues with it later
@@ -131,6 +131,45 @@ func TestRewrite(t *testing.T) {
}
}

// TestWordpress is a test for wordpress usecase.
func TestWordpress(t *testing.T) {

This comment has been minimized.

@elcore

elcore Mar 24, 2018

Collaborator

Does this need to be Wordpress specific?

This comment has been minimized.

@abiosoft

abiosoft Mar 24, 2018

Author Collaborator

not really, don't mind me. I was using to test while writing the code and decided to leave it there and let you all judge if it should stay.

This comment has been minimized.

@elcore

elcore Mar 24, 2018

Collaborator

Okay, I'll leave this to Matt 😄

This comment has been minimized.

@tobya

tobya Apr 12, 2018

Collaborator

I think its fine to leave this as a test.

@mholt

This comment has been minimized.

Copy link
Owner

mholt commented Mar 26, 2018

I may not get this into the 0.10.12 release tomorrow morning, but that's okay -- we can probably get it into the next one. Thanks Abiola! I'll look this over soon. :)

@mholt

mholt approved these changes Apr 15, 2018

Copy link
Owner

mholt left a comment

Thank you Abiola! Nice to see a contribution from you, we've missed it ;)

@mholt mholt merged commit 9fe2ef4 into mholt:master Apr 15, 2018

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@francislavoie

This comment has been minimized.

Copy link
Collaborator

francislavoie commented Apr 15, 2018

I'm only reading this now, just asking so I understand; I have the following currently for most of my sites:

    rewrite {
        to {path} {path}/ /index.php?{query}
    }

With this change, I could now shorten it to the following, correct?

    rewrite / {path} {path}/ /index.php?{query}

Is that right? Was the above not possible before?

@mholt

This comment has been minimized.

Copy link
Owner

mholt commented Apr 15, 2018

And now that I think of it, I should ask: is this backwards-compatible with existing one-liner rewrite uses?

@abiosoft

This comment has been minimized.

Copy link
Collaborator Author

abiosoft commented Apr 15, 2018

@francislavoie yeah, that is correct. rewrite / {path} {path}/ /index.php?{query} is not possible before. It does exact matching and / only matches requests to the site root i.e. it would not even match /index.php.

@mholt it is not 100% backward compatible, from is now with regex support.

My argument is that the one-liner should be a sensible default and people should only open a config block for more control, but what is currently happening is no one is using the one liner and many people open a config block to only use regex.

@abiosoft abiosoft deleted the abiosoft:simple-rewrite-regexp branch Apr 15, 2018

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