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.

Sign up for GitHub

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

Jump to bottom

Added R=302 #14

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Added R=302 #14

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,10 +45,9 @@
<form method="post">
<textarea id="tsv-input" cols="100" rows="20" name="tabbed_rewrites" style="width: 100%; height: 265px;" title="TSV Input"><?php echo htmlentities($paramRewrites) ?></textarea><br />
<select name="type" title="Rewrite Type">
<option value="<?= RewriteTypes::PERMANENT_REDIRECT ?>">301</option>
<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>
Copy link
Owner

Choose a reason for hiding this comment

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

The first option will be selected by default. This added logic is unessessary.

<option value="<?= RewriteTypes::PERMANENT_REDIRECT ?>" <?php echo $paramType === RewriteTypes::PERMANENT_REDIRECT ? ' selected' : '' ?>>301</option>
<option value="<?= RewriteTypes::SERVER_REWRITE ?>" <?php echo $paramType === RewriteTypes::SERVER_REWRITE ? ' selected' : '' ?>>Rewrite</option>
</select>
<label>
<input type="checkbox" name="desc_comments" value="1"<?php echo $paramComments ? ' checked' : '' ?>>Comments
Expand Down
10 changes: 6 additions & 4 deletions src/ApacheModRewriteGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ public function generateRewrite( string $from, string $to, int $type ) : string
$toHost = $parsedTo['host'] ?? '';

$fromQuery = $parsedFrom['query'] ?? '';
$toQuery = $parsedTo['query'] ?? '';
$toQuery = (isset($parsedTo['query']) && $parsedTo['query']!='') ? '?'.$parsedTo['query'] : '';

$fromPath = urldecode($parsedFrom['path'] ?? '');
$toPath = urldecode($parsedTo['path'] ?? '');
Expand Down Expand Up @@ -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;
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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 "?"

Copy link
Owner

Choose a reason for hiding this comment

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

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.

image

Copy link
Author

Choose a reason for hiding this comment

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

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.


switch( $type ) {
case RewriteTypes::SERVER_REWRITE:
return "{$output}&%{QUERY_STRING}";
case RewriteTypes::PERMANENT_REDIRECT:
return "{$output} [L,R=301]";
case RewriteTypes::TEMPORARY_REDIRECT:
return "{$output} [L,R=".RewriteTypes::name($type)."]";
}

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;
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand what you're up to here?

Escaping substitutions is important.

Copy link
Author

Choose a reason for hiding this comment

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

My experiences with your tool show me that this is not necessary to escape the rewrited URL.

Copy link
Owner

Choose a reason for hiding this comment

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

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.

}

}
2 changes: 1 addition & 1 deletion src/Engine.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Copy link
Owner

Choose a reason for hiding this comment

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

This just messes up the output formatting…

Copy link
Author

Choose a reason for hiding this comment

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

Just to get a shorter ouput...

} catch( GenerationException $e ) {
$output .= $this->generator->comment('ERROR: ' . $e->getMessage() . ': ' . $line);
$output .= "\n";
Expand Down
7 changes: 5 additions & 2 deletions src/RewriteTypes.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,18 @@

abstract class RewriteTypes {

public const SERVER_REWRITE = 1;
public const PERMANENT_REDIRECT = 2;
const SERVER_REWRITE = 1;
const PERMANENT_REDIRECT = 2;
const TEMPORARY_REDIRECT = 3;

public static function name( int $type ) : string {
switch( $type ) {
case self::SERVER_REWRITE:
return 'Rewrite';
case self::PERMANENT_REDIRECT:
return '301';
case self::TEMPORARY_REDIRECT:
return '302';
}

throw new \InvalidArgumentException('invalid type', $type);
Expand Down