Skip to content

[Routing] Generate URLs in compliance with PHP_QUERY_RFC3986 #19639

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

Closed
wants to merge 3 commits into from
Closed

[Routing] Generate URLs in compliance with PHP_QUERY_RFC3986 #19639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c0e6bf6
Update UrlGenerator::generate() so it encodes in compliance with PHP_…
Aug 16, 2016
4c9eb00
Fix breaking test under 5.3.x
Aug 16, 2016
389e446
Remove redundant code, fix UrlGenerator::testUrlEncoding()
jameshalsall Aug 16, 2016
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
8 changes: 7 additions & 1 deletion src/Symfony/Component/Routing/Generator/UrlGenerator.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,13 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
return $a == $b ? 0 : 1;
});

if ($extra && $query = http_build_query($extra, '', '&')) {
if (defined('PHP_QUERY_RFC3986')) {
$query = http_build_query($extra, '', '&', PHP_QUERY_RFC3986);
} else {
$query = http_build_query($extra, '', '&');
Copy link
Member

Choose a reason for hiding this comment

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

Can't we use str_replace here, from + to %20?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah 👍🏻. I pushed this in a rush earlier, I'll fix it shortly.

Sent from my iPhone

On 16 Aug 2016, at 20:49, Nicolas Grekas notifications@github.com wrote:

In src/Symfony/Component/Routing/Generator/UrlGenerator.php:

@@ -268,7 +268,17 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
return $a == $b ? 0 : 1;
});

  •    if ($extra && $query = http_build_query($extra, '', '&')) {

  •    // extract fragment

  •    $fragment = isset($extra['_fragment']) ? $extra['_fragment'] : '';

  •    unset($extra['_fragment']);

  •    if (defined('PHP_QUERY_RFC3986')) {

  •        $query = http_build_query($extra, '', '&', PHP_QUERY_RFC3986);

  •    } else {

  •        $query = http_build_query($extra, '', '&');
    Can't we use str_replace here, from + to %20?

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, I'm hesitant to deviate from using PHP_QUERY_RFC3986, as this is now in line with how Twig handles route generation: https://github.com/twigphp/Twig/blob/1.x/lib/Twig/Extension/Core.php#L615

}

if ($extra && $query) {
// "/" and "?" can be left decoded for better user experience, see
// http://tools.ietf.org/html/rfc3986#section-3.4
$url .= '?'.strtr($query, array('%2F' => '/'));
Expand Down
22 changes: 14 additions & 8 deletions src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -325,18 +325,24 @@ public function testGenerateWithSpecialRouteName()

public function testUrlEncoding()
{
if (defined('PHP_QUERY_RFC3986')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was it merged with this that is not needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it is needed, that was only added in PHP 5.4

Copy link
Contributor

Choose a reason for hiding this comment

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

it was merged in master which requires 5.5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it was merged to 2.7

Copy link
Contributor Author

@jameshalsall jameshalsall Aug 18, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

oh, well it was targeted at 2.7 - I can see Fabien merged to master instead - I will send another PR shortly to remove the defined() checks

Copy link
Member

Choose a reason for hiding this comment

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

Because I made the change after the merge.

Copy link
Contributor

Choose a reason for hiding this comment

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

No you didn't do it for this file. See https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Tests/Generator/UrlGeneratorTest.php#L328

Copy link
Member

Choose a reason for hiding this comment

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

oops, sorry about that. fixed now. Good catch as always :)

$expectedPath = '/app.php/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id'
.'/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id'
.'?query=%40%3A%5B%5D/%28%29%2A%27%22%20%2B%2C%3B-._~%26%24%3C%3E%7C%7B%7D%25%5C%5E%60%21%3Ffoo%3Dbar%23id';
} else {
$expectedPath = '/app.php/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id'
.'/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id'
.'?query=%40%3A%5B%5D/%28%29%2A%27%22+%2B%2C%3B-._%7E%26%24%3C%3E%7C%7B%7D%25%5C%5E%60%21%3Ffoo%3Dbar%23id';
}

// This tests the encoding of reserved characters that are used for delimiting of URI components (defined in RFC 3986)
// and other special ASCII chars. These chars are tested as static text path, variable path and query param.
$chars = '@:[]/()*\'" +,;-._~&$<>|{}%\\^`!?foo=bar#id';
$routes = $this->getRoutes('test', new Route("/$chars/{varpath}", array(), array('varpath' => '.+')));
$this->assertSame('/app.php/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id'
.'/@:%5B%5D/%28%29*%27%22%20+,;-._~%26%24%3C%3E|%7B%7D%25%5C%5E%60!%3Ffoo=bar%23id'
.'?query=%40%3A%5B%5D/%28%29%2A%27%22+%2B%2C%3B-._%7E%26%24%3C%3E%7C%7B%7D%25%5C%5E%60%21%3Ffoo%3Dbar%23id',
$this->getGenerator($routes)->generate('test', array(
'varpath' => $chars,
'query' => $chars,
))
);
$this->assertSame($expectedPath, $this->getGenerator($routes)->generate('test', array(
'varpath' => $chars,
'query' => $chars,
)));
}

public function testEncodingOfRelativePathSegments()
Expand Down