-
Notifications
You must be signed in to change notification settings - Fork 21
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
Handle query strings like httpd mod_rewrite #53
Comments
Seem reasonable. As long as this is supported in Apache. So from what I understand it won't match on the query string from now on? And only append the query when the QSA flag is set? Is there any flag to match on the query string on Apache, can't find any? |
Correct. (Unless you start to support the RewriteCond directive; see below)
It will only append with that flag. If the substitution string doesn't contain a query string, then the original query string is maintained.
Matching on query string will be tricky as you'll need to start supporting multi-line config and the RewriteCond directive. From the docs:
This is a pretty significant change so I guess it comes down to your vision for this module :P |
@jgchristian I think I need a few days to think about it. But can you clarify below:
So that I know in more detail the benefits of this change? |
Sure. My use case is that we're slowing porting functionality from a legacy httpd+mod_rewrite+Java app to Node. In the old setup, mod_rewrite is acting as a router and URL rewriter and we'd prefer to move this logic to within the Node app. Some of the existing rules are very specific around the way query strings are handled. Most importantly for me, the httpd mod_rewrite implementation offers more flexibility for query string handling that I wouldn't want to lose. That's not to say it would have to be implemented in the same way (i.e. with RewriteCond). For example, you could probably have a new flag to signify that you want to include the query string when matching the URL. But this is a derivation from the original implementation - so you'd probably have to advertise this module more as 'inspired by' mod_rewrite's syntax :) |
@jgchristian I'm ready to take this in at least QSA part. Are you interested in sending the PR of the work you have done. Sorry for the delay. |
Hi there
From my understanding of the module, the httpd mod_rewrite implementation only considers the path when matching the URLs to be rewritten. From http://httpd.apache.org/docs/current/mod/mod_rewrite.html :
Presently connect-modrewrite matches on both the path and the query string when matching a request URL.
Naturally I'm aware that this is only a partial implementation of mod_rewrite's RewriteRule directive, but this seems like quite a significant derivation from the original implementation. It makes my use case of sharing or porting existing configuration very tricky!
In addition to only matching on the path, it's important for me to support mod_rewrite's behaviour for handling query strings when rewriting URLs. In brief:
My fork of the project contains this commit which pretty much covers the above (minus updates to the documentation)
This is a breaking change to the API, so I'd assume it would require a major version increment.
As it is a breaking change, please advise on whether you'd like me to raise a pull request for this or you would prefer me to maintain the fork.
Thanks
The text was updated successfully, but these errors were encountered: