Conversation
No "?" added if no query string fix "public const"
donatj
left a comment
donatj
left a comment
There was a problem hiding this comment.
You've got some stuff indented with spaces, some stuff indented with tabs. Please reformat with tabs.
Please consider my feedback and get the tests into a passing state and I will re-review.
|
|
||
| $output .= $this->generator->generateRewrite($explodedLine[0], $explodedLine[1], $type); | ||
| $output .= "\n\n"; | ||
| $output .= "\n"; |
There was a problem hiding this comment.
This just messes up the output formatting…
There was a problem hiding this comment.
Just to get a shorter ouput...
| } | ||
|
|
||
| $output .= 'RewriteRule ^' . preg_quote(ltrim($fromPath, '/'), ' ') . '$ ' . $this->escapeSubstitution($prefix . ltrim($toPath, '/')) . '?' . $toQuery; | ||
| $output .= 'RewriteRule ^' . preg_quote(ltrim($fromPath, '/'), ' ') . '$ ' . $this->escapeSubstitution($prefix . ltrim($toPath, '/')) . $toQuery; |
There was a problem hiding this comment.
fix - no "?" added if no query string
The trailing question mark is actually very important and there for a reason. It tells Apache not to forward given GET parameters.
There was a problem hiding this comment.
But it adds a blank query string which messes up for a true "friendly URL rewriting".
If you do not add QSA in the rewrite rule options, no query string will be given.
My experiences with your tool show me that this is not necessary to end with "?"
There was a problem hiding this comment.
That is as designed. It is designed to do the exact rewrite the user enters. Nothing more.
If you do not include a query string, a query string should not be included. If the user wants a query string, they can include a query string. Doing what the user told us to do is the more friendly option.
As you have it, the rewrite:
foo.html?bar=10 foo.html
Outputs:
RewriteCond %{QUERY_STRING} (^|&)bar\=10($|&)
RewriteRule ^foo\.html$ /foo.html [L,R=301]
Which results in an endless loop because the ?bar=10 is appended to the redirected URL.
There was a problem hiding this comment.
You're right. I only used it without query string in "from" URL.
We can cancel this pull request ?
I'll work on my fork only.
| private function escapeSubstitution( string $input ) : string { | ||
| return preg_replace('/[-\s%$\\\\]/', '\\\\$0', $input); | ||
| //return preg_replace('/[-\s%$\\\\]/', '\\\\$0', $input); | ||
| return $input; |
There was a problem hiding this comment.
I don't understand what you're up to here?
Escaping substitutions is important.
There was a problem hiding this comment.
My experiences with your tool show me that this is not necessary to escape the rewrited URL.
There was a problem hiding this comment.
foo.html bar%20baz.html is an example of where it is necessary.
As you have it, it outputs RewriteRule ^foo\.html$ /bar baz.html&%{QUERY_STRING} which results in an internal server error. The space needs to be escaped.
| <option value="<?= RewriteTypes::SERVER_REWRITE ?>" <?php echo $paramType === RewriteTypes::SERVER_REWRITE ? ' selected' : '' ?>> | ||
| Rewrite | ||
| </option> | ||
| <option value="<?= RewriteTypes::TEMPORARY_REDIRECT ?>" <?php echo $paramType === RewriteTypes::TEMPORARY_REDIRECT ? ' selected' : '' ?>>302</option> |
There was a problem hiding this comment.
The first option will be selected by default. This added logic is unessessary.
2 participants
fix - no "?" added if no query string
fix - RewriteTypes "public const" compat.
Remove blank line separator