[Twig] added subpath function #3965

Closed
wants to merge 2 commits into
from

Projects

None yet
@Tobion
Symfony member
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #4849, partly #6857
License MIT

The new function subpath merges the variables of the current route request with the ones you optionally passed to the function. On top of that you can also add the query params (disabled by default).

It will generate a path-relative URL once #3958 gets merged but this PR is not necessarily depending on it. Since the target URL, that inherits some params from the current request, is probably a sub-page in the URL hierarchy (which is why it's called subpath), it makes sense to default it to a relative path, which makes the links shorter.

Use cases:

  1. Generating the same route but changing a few parameters while keeping others of the current request (e.g. typical locale switcher).

  2. Generating a different route while removing/overriding/adding some parameters on top of the current params. This is very useful when you have hierarchically structured pages, which is very common.
    Example: We are at /user-slug/article-slug/ (article_show) and want to generate the link to comments of the article (article_comments). At the moment we would have to always repeat the current variables like
    {{ path('article_comments', {'user': 'user-slug', 'article': 'article-slug'}) }} and would for example get /user-slug/article-slug/comments.
    With subpath the advantage is you don't need to specify the variables of the current request again.
    So simply {{ subpath('article_comments') }} is enough for the relative URL comments in this case. But you can optionally overwrite some request variables with {{ subpath('article_comments', {'article': 'different-article-slug'} ) }} which would generate ../different-article-slug/comments (i.e. same user, but different article).

  3. Merging the query params can be used for filtering links, e.g. a facetted navigation page that filters items and you want to generate a link to add/overwrite filters based on the current filters. This is also one reason why removing params is needed which can be achieved by passing null as value for the unwanted param.

  4. Generating a route with the same host params. See #6857.

  5. Generating the same URL. I really thought much about this use-case. And I came to the conclusion it's not necessary for it to have a specific twig function. Of course you can also generate the same URL with path, url and subpath functions. But with path and url, you need to pass all variables again. And with subpath it will just generate an empty string '' because that's the relative path for the current URL. So you don't need to call subpath at all. And if you need an absolute path or url, then you can just access the request information directly (Request::getRequestUri, i.e. in twig {{ app.request.requestUri }}. It's also faster because it doesn't go through the routing url generation at all. So all in all, it's already easy to get the same url. It would also not have much to do with subpath but instead it would be more appropriate to expose a new function like {{ self() }}. But as I said, it's not really needed and not part of this PR.

@stof
Symfony member

This is indeed the wrong way as it makes Twig unusable from the CLI

@Tobion
Symfony member

So injecting the container?

@alexandresalome

Magic was killed in 2010, and I don't think it's a good thing to generate URLs like this. Probably you should integrate this to your project, maybe a bundle or an article. But I don't think it should be part of the framework.

@Tobion
Symfony member

It's not magic. It just exposes the information (_route_params) that are already stored for these use cases in a well defined method.

@sstok sstok commented on an outdated diff May 4, 2012
src/Symfony/Bridge/Twig/Extension/RoutingExtension.php
public function getPath($name, $parameters = array())
{
return $this->generator->generate($name, $parameters, false);
}
- public function getUrl($name, $parameters = array())
+ /**
+ * Returns the path to a route (defaults to current route) with all current route parameters merged
+ * with the passed params. Optionally one can also include the params of the query string.
@sstok
sstok May 4, 2012

You should separate the short and long description.

@Tobion
Symfony member

FYI, Zend Framework also has the possibility to merge the current request params as this PR proposes.
See https://github.com/zendframework/zf2/blob/master/library/Zend/View/Helper/Url.php#L83

@kix

+1 here, we do need to have a way to generate links to current path while being able to alter a single parameter and keeping others the same as they were. I18n locale switching is just one of the cases here.
And this should really be achieved by using the Symfony core, not by third-party bundles.

@stof
Symfony member

@kix Symfony 2.1 introduced an attribute making it quite easy to do this locale switching:

{% set route_params = app.request.attributes.get('_route_params') %}

{# merge the query string params if you want to keep them when switching the locale #}
{% set route_params = route_params|merge(app.request.query.all) %}

{# merge the additional params you want to change #}
{% set route_params = route_params|merge({'_locale': 'fr'}) %}

{{ path(app.request.attributes.get('_route'), route_params) }}
@kix
@stof stof and 2 others commented on an outdated diff Oct 13, 2012
src/Symfony/Bridge/Twig/Extension/RoutingExtension.php
private $generator;
- public function __construct(UrlGeneratorInterface $generator)
+ /**
+ * Constructor.
+ *
+ * @param ContainerInterface $container A ContainerInterface instance
+ * @param UrlGeneratorInterface $generator A UrlGeneratorInterface instance
+ */
+ public function __construct(ContainerInterface $container, UrlGeneratorInterface $generator)
@stof
stof Oct 13, 2012

Depending on the container makes it impossible to use this extension in a project using Twig and Routing without using the DI component (Silex projects for instance)

@Tobion
Tobion Oct 14, 2012

So should i make it __construct(UrlGeneratorInterface $generator, ContainerInterface $container = null)
and then assert that the container is set in getSubPath?

@Tobion
Tobion Mar 14, 2013

@stof would that be better? Silex would only not be able to use the subPath function then, or?

@fabpot
fabpot Aug 22, 2013

I would remove the container dependency altogether and add a setRequest(Request $request = null) method instead. This will allow the container to automatically inject the correct Request object and others would need to call the setRequest() method by themselves.

You can have a look at a similar implementation in LocaleListener for instance. But basically, here is the setRequest() method:

public function setRequest(Request $request = null)
{
    $this->request = $request;
}

And call the method in the container configuration:

<call method="setRequest"><argument type="service" id="request" on-invalid="null" strict="false" /></call>

As the request can be null (outside request handling), you should take care of that edge case in your code.

@Tobion
Tobion Aug 27, 2013

Any reason why on-invalid="null" instead of on-invalid="ignored"? Ignored seems better because why would someone want to set the request to null.

@fabpot
fabpot Aug 27, 2013

When we are not in the request scope (before handling a request or after handling a request), the request is null, so the setter must be able to accept/receive the null value.

@Tobion
Tobion Aug 27, 2013

Yeah but when using on-invalid="ignored" the setter will not be called when request is null. Should work just as fine, doesnt it?

@fabpot
fabpot Aug 27, 2013

The setter must be called so that your object knows that it is acting outside the handling of a request. If not, you will still have access to an older Request object.

@Tobion
Tobion Sep 17, 2013

Hm, actually we don't really want the subrequest info. Only the master request matters. When generating a relative path in a subrequest (e.g. a partial with ESI), it would be wrong from a browser perspective. Because the browser would resolve the relative link based on the master/main/user-entered URL. So the relative link in a subrequest probably goes to the wrong target.

@Tobion
Tobion Sep 17, 2013

But this is just a documentation thing and it should be clear that relative paths in a subrequest don't make much sense. This is nothing special for the new subpath function; the same applies for the url and path methods.

@Tobion
Tobion Sep 17, 2013

this is then also fixed by the request stack

@Tobion
Symfony member

There is an issue that makes it impossible to really re-generate the current URL:
The _route_params attribute in the request also contains the merged default values. So when you use these params to generate the URL, it might not really be the current URL. Example: new Route('/{page}.{_format}', array('_format' => 'html') which matches the URL /page.html. So _route_params = array('page' => 'page', '_format' => 'html').
When regeneting the URL it will produce /page without the format because the default it left out (see #5180), so it's not the current URL. With #5410 it would be the other way round, so still a problem.

So the only way to fix it would be that _route_params only contains the matched params without default and a new attribute like _route_defaults contains the defaults. But the current Listener and UrlMatcher does not allow to distinguish these values.

@Tobion
Symfony member

As there is no way to really regenerate the current URL with the UrlGenerator (see above), we cannot use it in this case. But I just figured we don't need to. We can simply use Request::getUri or Request::getRequestUri to achieve that. It's also more straight-forward because why would we need to regenerate the Url that we already have available...

@Tobion
Symfony member

I added an url generation mode that returns the shortest path. This is not required for this PR. But I think it's a good idea because only the UrlGenerator already has this information available so why not expose this possibility.

@stof stof and 1 other commented on an outdated diff Mar 14, 2013
...Component/Routing/Generator/UrlGeneratorInterface.php
@@ -56,6 +56,12 @@
const NETWORK_PATH = 'network';
/**
+ * Generates a relative or absolute path depending on which is shorter,
+ * e.g. "/dir/file" vs "../dir/file".
+ */
+ const SHORTEST_PATH = 'short';
@stof
stof Mar 14, 2013

This should be a separate PR as it is a separate feature (and I don't really see a need for it)

@Tobion
Tobion Aug 22, 2013

Yeah I will remove it. Was just experimental.

@fabpot fabpot and 1 other commented on an outdated diff Sep 17, 2013
src/Symfony/Bridge/Twig/Extension/RoutingExtension.php
public function getPath($name, $parameters = array(), $relative = false)
{
return $this->generator->generate($name, $parameters, $relative ? UrlGeneratorInterface::RELATIVE_PATH : UrlGeneratorInterface::ABSOLUTE_PATH);
}
- public function getUrl($name, $parameters = array(), $schemeRelative = false)
+ /**
+ * Sets the current request that is needed for the subpath function.
@fabpot
fabpot Sep 17, 2013

You should use the request stack instead.

@Tobion
Tobion Sep 17, 2013

I don't understand what u mean

@fabpot
fabpot Sep 17, 2013

Instead of relying on a setRequest method, you can take the request stack as an argument and call ->getMasterRequest() whenever you need the request. That way, you always have the right master request. The current way won't work if you handle two master requests in the same PHP process (without rebooting the Kernel of course.)

@Tobion
Tobion Sep 17, 2013

ah i see. it's about #8904

@Tobion
Symfony member

@fabpot this is ready. I updated the main description.

@Tobion Tobion commented on the diff Sep 17, 2013
src/Symfony/Bundle/TwigBundle/Resources/config/twig.xml
@@ -81,6 +81,7 @@
<service id="twig.extension.routing" class="%twig.extension.routing.class%" public="false">
<argument type="service" id="router" />
+ <argument type="service" id="request_stack" />
@Tobion
Tobion Sep 17, 2013

twig bundle doesnt have the framework bundle as dependency yet. but since the twig bundle relies on service definitions in framework bundle, shouldn't it be the case?!

@fabpot
fabpot Sep 17, 2013

no, the Twig bridge is used by Silex without the definition as we are using Pimple there, so this is not a hard dependency.

@Tobion
Tobion Sep 17, 2013

this is the twig bundle, not the bridge.

@Tobion Tobion commented on an outdated diff Sep 17, 2013
src/Symfony/Bridge/Twig/Extension/RoutingExtension.php
{
- return $this->generator->generate($name, $parameters, $schemeRelative ? UrlGeneratorInterface::NETWORK_PATH : UrlGeneratorInterface::ABSOLUTE_URL);
+ if (null === $this->requestStack || null === $masterRequest = $this->requestStack->getMasterRequest()) {
@Tobion
Tobion Sep 17, 2013

hm I think using the master request is even more strange. so it uses the params of the master request but the generator will calculate the relative path based on the subrequest path because the request context in routing is always updated with the current request. this would be quite inconsistent. so the relative link would still be wrong for the user agent (browser). i guess we should either use the current request attributes here as well and people need to aware what they are doing when using this method in a subrequest.
or the RoutingListener should only update the routing request context for the master request. I don't see why the routing needs the request context for subrequests.

@Tobion
Tobion Sep 17, 2013

@fabpot what do you think?

@Tobion
Tobion Sep 17, 2013

hm well, the request context cannot only be updated for the master request because the matching process of course needs the current request info. but for generating urls it's actually quite useless to do that inside subrequests based on the subrequest info. this is not only relevant for relative paths, but also when examining whether the host or scheme changed for absolute paths.

what this means is that matching/generating would need different request contexts:

  • matching urls process -> needs current request info
  • generating urls process -> should only be based on the master request info
@Tobion
Tobion Sep 18, 2013

I changed it to use the current request. The url generation problem in subrequest is also the same for path() and url(). So has nothing to do with this pr.

@fabpot its ready for me

Tobion added some commits Apr 17, 2012
@Tobion Tobion [Twig] added subpath function
use the current route by default

add possibility to remove existing params by passing null for it

added tests for the twig routing extension

use synchronized request
b3e68d0
@Tobion Tobion use the request stack instead of synchronized request e8fda4d
@Tobion
Symfony member

@fabpot ping ready

@fabpot
Symfony member

@Tobion Documentation is missing

@fabpot
Symfony member

The more I think about it, the more I find the feature confusing. The probability that a route takes the same arguments as the current one is rather thin. I know that you can remove arguments by setting them to null, but then, I think it's better to just pass the arguments you need. And if you use subpath in a partial that is included in more than one page (so more than one URL), the behavior is not predictable anymore.

I can definitely see the use case for variation of the current route, but then, we won't need the route name as an argument.

So, I'm still -1 on adding this in core as is.

@fabpot
Symfony member

Let me explain my feeling a bit more.

Let's say that my current page is /articles/Foo/12.

This URL has been generated with {{ path('article', { id: 12, category: 'Foo' }) }}

Now, on the page, I want to generate a link to the next article, 13, using {{ subpath('article', { id: 14 }) }} looks wrong as this is not a sub path of the current page.

I do understand the usefulness of the feature but the implementation does not convey the right meaning.

What about doing something like: {{ path('article', { id: 14 }|merge_current_path_attributes) }} instead (replace merge_current_path_attributes with whatever makes more sense and is shorter)?

Let's take another example, a list: {{ path('articles', { sort: 'desc', q: 'foo', page: 4 }) }}

I have several link to change the different values of the route attributes. I can do {{ subpath('articles', { page: 5 }) }} to go to page 5 or {{ path('articles', { page: 5 }|merge_current_path_attributes }}. Again, I prefer the second one as the page is not a sub path of the current page.

Of course, we could also be able to remove some attributes: {{ path('articles', { page: 5 }|merge_current_path_attributes({ q: null }) }}.

@Tobion
Symfony member

So you are just not satisfied with the name subpath. The idea behind the name is that is expresses that the current request attributes are reused and that the resulting reference is relative. That the resulting URL must not be a "subpage" depends on the route defintions of the developer which we cannot influence. The name should just intend what it is designed for. The possibilily to also generate "siblings" or "parents" is just the flexibility. So I'm open to other name suggestions. But the argument that does not necessarily fit the resulting URL would also be true for path itself. Because the path function can also generate full URLs (when host/scheme is different). So you could say it's just as confusing. So again depends on the route definitions that path() does not generate a path, but a URL.

{{ path('articles', { page: 5 }|merge_current_path_attributes }}

Could work as well. But stilll some problems
1. What's the best name?
2. There is still no way to reuse the current route name (except app.request.attributes.get(_route) which is not really friendly).
3. How to optionally include the query params?

@stof
Symfony member

but the only feature added by this PR is the easy reuse of attributes. Generating a relative url is already supported since 2.2 through path(..., ..., true), which will call the url generator with UrlGeneratorInterface::RELATIVE_PATH

@Tobion
Symfony member

@stof I think nobody said something different.

@stof
Symfony member

quoting you: The idea behind the name is that is expresses that the current request attributes are reused and that the resulting reference is relative.. Making the url relative is not a feature added by this method.

thus, the filter-based implementation proposed by @fabpot above would be compatible with the 4 url generation styles, not only with the relative path.

@Tobion
Symfony member

Huh? I didnt say its a feature added here. I just said why I chose the name subpath because that is what fabpot does not agree with. But I'm open to any other name if you find one.

@fabpot
Symfony member

I think you are both right ;) Indeed I don't like the name, but @stof is right as the feature you want to add can be more easily done with a filter and would work with all generator strategies.

@Tobion
Symfony member

I still don't see how to use the current route with a filter without bc break. So if the 3 problems are solved above, I'm fine using a filter. Although I find a seperate function more clean.

@Tobion
Symfony member

If we don't provide a shortcut, we could say, we don't actually need to do anything. People can already use something like {{ path(app.request.attributes.get('_route'), app.request.attributes.get('_route_params')|merge({ page: 5 })) }}. Looks quite strange, but it's using ur filter suggestion.

@dewos

A shortcut like {{ path(app.request.route, app.request.params|merge({ page: 5 })) }}? Will not be BC?

@stof
Symfony member

@Dewos There is no reason to have a getRoute() getter on the Request object

@ErichHartmann

From a neophye's perspective, I see a lot of value in easily generating new paths in the response from the request's route's attributes. I agree, however, with @stof and @fabpot that it is not a "sub" path. It is not a hierarchy, but instead the request's attributes used to generate a whole new path in the response (atomic?).

I do think the @Tobion idea for a much more simplified filter is important, because as a new implementer, I would never conceive of something as intricate as:

{{ path(app.request.attributes.get('_route'), app.request.attributes.get('_route_params')|merge({ page: 5 })) }}

but...

{{ path('articles', { page: 5 }|merge_request_path_attributes }}

or...

{{ path('articles', { page: 5, q: null, _locale: de-DE }|merge_request_path_attributes }}

... readily makes sense to me and conveys the intent of generating a new path in the response using the request's path's attributes.

(notice the small edit I made to the @fabpot example above -- it's not the "current" path, but belongs to the request's path/route in my mind. The is no temporal or hierarchical component to the function.)

I hope this can yet make it into 2.4.

@bierdok

Why not make the use of the underscore generic in the router ?

For example,

host: {_subdomain}.acme.org
pattern: {_category_id}/{article_id}.html

As we do today, I think about the variable "_locale" that does not need to be redeclared each url generation.

I found a solution to do this properly.

Just add a route listener

namespace Acme\CoreBundle\EventListener;

use Symfony\Bundle\FrameworkBundle\Routing\Router;
use Symfony\Component\EventDispatcher\EventSubscriberInterface;
use Symfony\Component\HttpKernel\Event\GetResponseEvent;
use Symfony\Component\HttpKernel\KernelEvents;

class RouteListener implements EventSubscriberInterface
{
    private $router;

    public function __construct(Router $router)
    {
        $this->router = $router;
    }

    public function onKernelRequest(GetResponseEvent $event)
    {
        $request = $event->getRequest();

        $parameters = $request->attributes->get('_route_params');

        if($parameters) {
            foreach($parameters as $name => $value) {
                if(substr($name, 0, 1) == '_') {
                    $this->router->getContext()->setParameter($name, $value);
                }
            }
        }
    }

    public static function getSubscribedEvents()
    {
        return array(
            KernelEvents::REQUEST => array(array('onKernelRequest', 16)),
        );
    }
}

and instantiate it as a service

services:
    core.route_listener:
        class: Acme\CoreBundle\EventListener\RouteListener
        arguments:
            - "@router"
        tags:
            - { name: kernel.event_subscriber }

I hope this can help you. ;-)

@Lumbendil

The problem with {{ path(app.request.attributes.get('_route'), app.request.attributes.get('_route_params')|merge({ page: 5 })) }} is that it's not user friendly... then how about {{ path(app.currentRoute, app.currentRouteParams|merge({ page: 5 })) }} ? This could be done by adding the GlobalVariables::getCurrentRoute() and GlobalVariables::getCurrentRouteParams() methods.

@Koc

Very useful function. Is it possible ship it with 2.6?

@Tobion
Symfony member

We cannot agree how to provide this feature. So I don't think it will make it to 2.6

I'm summing up the proposals and arguments against it:

  1. Using a new function like {{ subpath('articles', { 'page': 5 } ) }} as this PR suggests.

    • better name?
    • not clear enough what it does?
  2. Using a new filter like {{ path('articles', { 'page': 5 }|merge_current_path_attributes }}

    • this would inverse the logic of merge as normally the second params overwrite the first. but here it's the other way round
    • better name?
    • how to reuse the current route name
    • how to optionally include the query params
  3. Not provide a shortcut at all and let users do somthing like {{ path(app.request.attributes.get('_route'), app.request.attributes.get('_route_params')|merge({ page: 5 })) }}

    • not user-friendly
    • no way to remove params
    • things like query params merging has to be implemented again and again
    • manually merging query params can lead to wrong implementations that break the site, e.g. giving query params more priority than path placeholders allows hackers to break all links
  4. Provide twig globals to ease accessing stuff like {{ path(app.currentRoute, app.currentRouteParams|merge({ page: 5 })) }}

    • more user-friendlyl but same problems as in 3)
@jakzal
Symfony member

How about app.request.attributes.merge('_route_params', {page: 5})?

@jakzal
Symfony member

On a second thought it's not a good idea, since _route_params might not be an array in case of a request attribute.

@Tobion Tobion changed the title from [2.4][Twig] added subpath function to [Twig] added subpath function Dec 9, 2014
@wouterj
Symfony member

ping @xabbuh @jakzal @stof @fabpot @Tobion can we please decide on the solutions mentioned in #3965 (comment) ?

@xabbuh
Symfony member

In my opinion, option four is a step in the right direction. The app.currentRoute and app.currentRouteParams globals could also be useful outside of creating URLs/paths.

Some things we then still need to solve are:

  • removing params
  • merging query params

For the first issue, I could imagine that this is something that could be added to Twig itself (like it is done the other way around with the merge filter).

For the second issue, I don't have a concrete idea right now. Maybe adding another Twig global or function could be the solution.

@fabpot
Symfony member

It really depends on which use cases we are talking about. From the issue description, I think that only the first one is relevant.

@fabpot
Symfony member

I'm closing this very old PR as there is no activity, and no consensus on what to do.

@fabpot fabpot closed this Jun 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment