[Router] added appending of new optional document fragment #12979

rodnaph · 2014-12-14T22:19:28Z

added a new optional parameter to the generate method for the document fragment. when specified this is appended to generated urls.

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	none
License	MIT
Doc PR	none

timglabisch · 2014-12-14T22:33:41Z

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

@@ -284,6 +284,11 @@ protected function doGenerate($variables, $defaults, $requirements, $tokens, $pa
            $url .= '?'.strtr($query, array('%2F' => '/'));
        }

+        // add fragment if needed
+        if ($fragment) {
+            $url .= '#'.rawurlencode($fragment);


why are parameters a array and fragement a string?

because it is a string, there is no natural divider that we can use to represent arrays.

henrikbjorn · 2014-12-15T07:47:19Z

👍

Koc · 2014-12-15T08:29:00Z

ref #3910

Koc · 2014-12-15T08:35:18Z

Also interface changes is a BC-break

rodnaph · 2014-12-15T08:54:09Z

I didn't see that other ticket, thanks for referencing it. I would counter the reason it was closed though because...

Fragment generation requires encoding that could gain correctness from framework help (rather than having to encode the fragment yourself)
With the redirectToRoute helper in 2.6 it now remains clumsy if you are redirecting to a route with a fragment (ie. You can't so need to use generateUrl and redirect)

I also didn't think of the BC break... That's unfortunate.

rodnaph · 2014-12-15T09:07:50Z

I didn't realise there were special parameters (underscore prefixed) to generate, that sounds like a better approach - hopefully my reasons above will help get that other ticket reopened at least.

rodnaph · 2014-12-15T10:15:34Z

I reworked the patch to use a 'special' parameter inline with other Symfony special parameters for the fragment. This removes the BC issue with the interface, though would obviously cause a problem if people were already using _fragment as a valid query string parameter.

Tobion · 2014-12-15T12:30:01Z

Still a BC break as you figured yourself.

dawehner · 2014-12-21T15:27:08Z

Just a quick comment. In Drupal we have all kind of additional options, like the fragment, but also language, whether we want to render it as http or https ...
The signature looks like the following:

  public function generateFromRoute($name, $parameters = array(), $options = array());

Tobion · 2015-02-03T22:27:39Z

I agree that a helper for the fragment is useful because of the encoding need. And I cannot think of a better solution than the one provided here. And the tiny BC break is negligible because it's unlikely.

Three things are missing for me to be merged:

the special meaning of _fragment must be documented in the phpdoc of UrlGeneratorInterface
the slash / should be decoded from the fragment just as it has been done for the query in decodes some special chars in a URL query #12223. this is also part of the rfc: "The characters slash ("/") and question mark ("?") are allowed to
represent data within the fragment identifier."
we should add a test when_fragment is used as a path parameter name, e.g. /fragment/{_fragment} both for generating and matching. and a test with a same document relative reference with just a fragment, e.g. #fragment as a result.

fabpot · 2015-09-14T07:46:45Z

@rodnaph Can you address @Tobion concerns?

rodnaph · 2015-09-14T09:14:35Z

@Tobion

I've addressed your first two concerns.

For the third I thought the best solution would be to throw a LogicException when compiling the route, as _fragment has no meaning as a path parameter. I'm not sure what you meant about "a test with a same document relative reference with just a fragment" - would that be a route with no pattern? eg. new Route(''); ?

I also rebased onto current master as there were some failing lint issues.

/cc @fabpot

bendavies · 2015-09-17T08:52:11Z

+1 @Tobion

stloyd · 2015-09-17T08:53:11Z

src/Symfony/Component/Routing/RouteCompiler.php

@@ -59,6 +59,13 @@ public static function compile(Route $route)
        $staticPrefix = $result['staticPrefix'];

        $pathVariables = $result['variables'];
+
+        foreach ($pathVariables as $pathParam) {
+            if ('_fragment' == $pathParam) {


Should be ===.

Though not documented, it looks like the variables output by compilePattern will always be strings, so a strict type equality would not be required.

Unless of course this is a coding standard, or maybe if a test could show it was required (as it seems to me without proof either way then both strict and non-strict comparison have the possibility of causing a bug).

Our coding standards are that we always use strict comparison unless we explicitly need the type juggling (because type juggling can actually cause totally unexpected behavior if you are not careful).
And given we know that strings are always returned, we know we don't need this type juggling.

inso · 2015-09-20T22:33:31Z

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

        if ($extra && $query = http_build_query($extra, '', '&')) {
            // "/" and "?" can be left decoded for better user experience, see
            // http://tools.ietf.org/html/rfc3986#section-3.4
            $url .= '?'.strtr($query, array('%2F' => '/'));
        }

+        if ($fragment) {


this is broken for $fragment = '0'

rodnaph · 2015-09-21T08:48:42Z

@inso Fixed, thanks.

Koc · 2015-09-21T09:09:48Z

src/Symfony/Component/Routing/RouteCompiler.php

@@ -59,6 +60,13 @@ public static function compile(Route $route)
        $staticPrefix = $result['staticPrefix'];

        $pathVariables = $result['variables'];
+
+        foreach ($pathVariables as $pathParam) {


array_search?

fabpot · 2016-03-04T11:14:56Z

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

        $extra = array_diff_key($parameters, $variables, $defaults);
+
+        // extract fragment
+        $fragment = isset($extra['_fragment']) ? $extra['_fragment'] : false;


the default value should be "", not false

fabpot · 2016-03-04T11:15:53Z

Apart from my small comment, 👍

ping @Tobion

@rodnaph Can you rebase on current master?

rodnaph · 2016-03-04T12:15:35Z

@fabpot done

stloyd · 2016-03-04T13:00:01Z

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

        if ($extra && $query = http_build_query($extra, '', '&')) {
            // "/" and "?" can be left decoded for better user experience, see
            // http://tools.ietf.org/html/rfc3986#section-3.4
            $url .= '?'.strtr($query, array('%2F' => '/'));
        }

+        if (strlen($fragment)) {


Should be '' === $fragment.

'' !== $fragment

…t fragment

fabpot · 2016-06-16T16:09:56Z

Thank you @rodnaph.

…ment (rodnaph) This PR was merged into the 3.2-dev branch. Discussion ---------- [Router] added appending of new optional document fragment added a new optional parameter to the generate method for the document fragment. when specified this is appended to generated urls. | Q | A | ------------- | --- | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | none | License | MIT | Doc PR | none Commits ------- 6d79a56 [Routing] adds _fragment special option to url generation for document fragment

…ms (xabbuh) This PR was merged into the 3.2-dev branch. Discussion ---------- [Routing] treat fragment after resolving query string params | Q | A | ------------- | --- | Branch? | master | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | | License | MIT | Doc PR | The implementation from #12979 led to a conflict with #18280 which was merged in the meantime. Commits ------- 7475aa8 treat fragment after resolving query string params

King2500 · 2016-07-06T13:40:28Z

Why not use the more intuitive (and less conflicting) parameter key # instead of _fragment, like:

// generating a URL with a fragment (/settings#password)
$this->get('router')->generate('user_settings', ['#' => 'password']);

Tobion · 2016-07-06T14:44:35Z

@King2500 true, could be an alternative. I guess _fragment was the first idea because it's also _locale that has a somewhat special meaning.

javiereguiluz · 2016-07-06T14:53:38Z

@King2500 I'm always in favor of short option names, but in this case I'd prefer to keep the consistency. Every special routing option in Symfony follows the same pattern: _controller, _locale, _route, _route_params, _redirected, etc.

…(alexislefebvre) This PR was squashed before being merged into the master branch (closes #6783). Discussion ---------- Routing: add explanation for the "_fragment" parameter Add explanation about this PR: symfony/symfony#12979 Presented in http://symfony.com/blog/new-in-symfony-3-2-routing-improvements I copy-pasted the start of the announcement as the explanation since I couldn't have found a better explanation. I hope that won't be a problem. Commits ------- d657abd Routing: add explanation for the "_fragment" parameter

rodnaph force-pushed the router-generate-document-fragment branch from 8e5ec77 to 8b16335 Compare December 14, 2014 22:22

timglabisch reviewed Dec 14, 2014
View reviewed changes

rodnaph force-pushed the router-generate-document-fragment branch from 8b16335 to 6fc2bd5 Compare December 15, 2014 10:11

fabpot added the Routing label Dec 18, 2014

rodnaph force-pushed the router-generate-document-fragment branch from 1847f98 to 049f0c6 Compare September 14, 2015 09:10

stloyd reviewed Sep 17, 2015
View reviewed changes

rodnaph force-pushed the router-generate-document-fragment branch from c005615 to 31e1d21 Compare September 18, 2015 15:08

inso reviewed Sep 20, 2015
View reviewed changes

rodnaph force-pushed the router-generate-document-fragment branch from 31e1d21 to 344291d Compare September 21, 2015 08:46

Koc reviewed Sep 21, 2015
View reviewed changes

rodnaph force-pushed the router-generate-document-fragment branch from 344291d to 5f00a59 Compare October 22, 2015 13:53

javiereguiluz added the Feature label Mar 3, 2016

fabpot reviewed Mar 4, 2016
View reviewed changes

rodnaph force-pushed the router-generate-document-fragment branch from 5f00a59 to 342d7eb Compare March 4, 2016 12:01

stloyd reviewed Mar 4, 2016
View reviewed changes

[Routing] adds _fragment special option to url generation for documen…

6d79a56

…t fragment

rodnaph force-pushed the router-generate-document-fragment branch from 342d7eb to 6d79a56 Compare March 26, 2016 23:11

fabpot merged commit 6d79a56 into symfony:master Jun 16, 2016

xabbuh mentioned this pull request Jun 17, 2016

[Routing] treat fragment after resolving query string params #19088

Merged

alexislefebvre mentioned this pull request Jul 29, 2016

Routing: add explanation for the "_fragment" parameter symfony/symfony-docs#6783

Closed

fabpot mentioned this pull request Oct 27, 2016

Release v3.2.0-BETA1 #20317

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Router] added appending of new optional document fragment #12979

[Router] added appending of new optional document fragment #12979

rodnaph commented Dec 14, 2014

timglabisch Dec 14, 2014

henrikbjorn Dec 15, 2014

henrikbjorn commented Dec 15, 2014

Koc commented Dec 15, 2014

Koc commented Dec 15, 2014

rodnaph commented Dec 15, 2014

rodnaph commented Dec 15, 2014

rodnaph commented Dec 15, 2014

Tobion commented Dec 15, 2014

dawehner commented Dec 21, 2014

Tobion commented Feb 3, 2015

fabpot commented Sep 14, 2015

rodnaph commented Sep 14, 2015

bendavies commented Sep 17, 2015

stloyd Sep 17, 2015

rodnaph Sep 17, 2015

stof Sep 17, 2015

inso Sep 20, 2015

rodnaph commented Sep 21, 2015

Koc Sep 21, 2015

fabpot Mar 4, 2016

fabpot commented Mar 4, 2016

rodnaph commented Mar 4, 2016

stloyd Mar 4, 2016

xabbuh Mar 7, 2016

fabpot commented Jun 16, 2016

King2500 commented Jul 6, 2016

Tobion commented Jul 6, 2016

javiereguiluz commented Jul 6, 2016

[Router] added appending of new optional document fragment #12979

[Router] added appending of new optional document fragment #12979

Conversation

rodnaph commented Dec 14, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

henrikbjorn commented Dec 15, 2014

Koc commented Dec 15, 2014

Koc commented Dec 15, 2014

rodnaph commented Dec 15, 2014

rodnaph commented Dec 15, 2014

rodnaph commented Dec 15, 2014

Tobion commented Dec 15, 2014

dawehner commented Dec 21, 2014

Tobion commented Feb 3, 2015

fabpot commented Sep 14, 2015

rodnaph commented Sep 14, 2015

bendavies commented Sep 17, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rodnaph commented Sep 21, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Mar 4, 2016

rodnaph commented Mar 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jun 16, 2016

King2500 commented Jul 6, 2016

Tobion commented Jul 6, 2016

javiereguiluz commented Jul 6, 2016