Added R=302 #14
Added R=302 #14
Conversation
No "?" added if no query string fix "public const"
| @@ -48,7 +48,7 @@ public function generate( string $input, int $type, bool $comments ) : string { | |||
| } | |||
|
|
|||
| $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...
| @@ -63,20 +63,22 @@ public function generateRewrite( string $from, string $to, int $type ) : string | |||
| } | |||
| } | |||
|
|
|||
| $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.
| } | ||
|
|
||
| throw new InvalidArgumentException("Unhandled RewriteType: {$type}", $type); | ||
| } | ||
|
|
||
| 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.
fix - no "?" added if no query string
fix - RewriteTypes "public const" compat.
Remove blank line separator