PHP-based routing configuration also needs to be patched.

@nicolas-grekas we did not add thecontroller feature on the PHP loader, so I guessed this was on purpose. I'm not really sure it's a lot valuable to do so.. what do you think?

I'm talking abouthttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/Loader/Configurator/Traits/RouteTrait.php#L121

Right, so you mean we should add aredirect method in theRouteTrait?

As stated in#24640 (comment) and below, I would do it differently: instead of setting a controller, theredirect_to entry could just set a new entry in "defaults", named eg "_redirect_to" instead of "_controller". Then it'd be up to high level layers to act upon it.

@nicolas-grekas definitely agree, see myproposed options here.

The real question now is: who is going to handle the_redirect (or two_redirects) attributes? HttpKernel or FrameworkBundle? I'd argue for HttpKernel so it can benefit other libraries only using the kernel. If it's HttpKernel, we need to move the logic from FrameworkBundle'sRedirectController to the HttpKernel component.

Adding one more question we need to answer:
what about the "permanent" flag, and the "ignoreAttributes" setting?

what about the "permanent" flag, and the "ignoreAttributes" setting?

I'd sayredirect_permanent andredirect_ignore_attributes configurations (and their XML equivalents with-s)

@stof@nicolas-grekas I've updated the PR to use_redirect_to and_redirect_permanent attributes populated by the loaders inRouting. A new listener inFrameworkBundle adds the_controller onkernel.request when it sees the_redirect_* attributes in theRequest.

@sroze what would be the new way of doing this configuration in yaml? Can we do without a listener (yet another one for this looks like overhead to me...)

what would be the new way of doing this configuration in yaml?

The one in the PR description :)

Can we do without a listener (yet another one for this looks like overhead to me...)

Well... we could. Do you want me to merge with the existing one or doing something else? 🤔

AppVeyor's failure is not related. It's a network issue. Can somebody restart it?

ping@Tobion :)

Now that I know the router a bit better, I'd suggest leveragingRedirectableUrlMatcherInterface::redirect() instead. This also means I'd suggest to remove the permanent/non-permanent flag. Let's make this very simple, and make it a core feature on theRouting component only, since is has everything needed to deal with that standalone.

I'd suggest leveraging RedirectableUrlMatcherInterface::redirect() instead.

Can you guide me through what you have in mind a little bit more? Maybe one way would be to roughly explain how you'd change this pull-request?

when a_redirect entry is found in the matched route, a redirectable matcher could internally
return $this->redirect($ret['_redirect'], $ret['_route']);

I've been thinking about this proposal ... and I propose to make a minor change. Instead of usingredirect_to for redirecting both URLs and routes, we could be more explicit and defineredirect_to andredirect_to_route (this is also consistent with other Symfony parts, where "redirect" is for URLs and routes have a special redirect method). In practice:

# redirect_to for absolute or relative URLsblog_show:path:/blog/{slug}redirect_to:/some-relative-urlpermanent:true# redirect_to_route without route argumentsblog_show:path:/blog/{slug}redirect_to_route:'some-other-route'# this also works, to be consistent with the next case:# redirect_to_route: ['some-other-route']# redirect_to_route with route argumentsblog_show:path:/blog/{slug}redirect_to_route:['some-other-route', { arg1: 'value1', arg2: 'value2' }]

@sroze I know you are very busy and you also have a much more important PR related to the new Message component ... but it'd be great if you could finish this feature before the Symfony 4.1 "feature freeze" starts in 12 days. Thanks!

@javiereguiluz yep, agree with you, it would be nice. I'll update the PR this week :)

I've explored the option of moving this logic directly within the router, especiallywithin thePhpMatcherDumper and it worked for theredirect idea. It did not work at all for theredirect_to_route idea because it would require aUrlGeneratorInterface while the dumper matcher is... just a matcher. One way to tackle this would be to have a sort of mini-generator while dumping the matcher but this 1) would duplicate some logic and 2) won't support things like route parameters. This is therefore not a satisfying approach.

The problem with the current implementation is that is it specific to a few details about the routing and is sort of between the Routing component and the FrameworkBundle bundle. Redirection configuration (while allowed by the Routing component) wouldn't work without the FrameworkBundle, which is not what we'd like.

While discussing all of that with@nicolas-grekas, we jumped back to actually... what we are trying to solve: simplifying the YAML configuration. Itmight be that the only issue, for now, is theSymfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction FQCN that we always have to copy/paste. Nico's idea, which might make sense is that instead of redirect-specific configuration, we simply add controller aliases. Therefore, the redirection configuration could look like that:

redirect_to_admin:path:/defaults:_controller:_redirect_pathpath:/adminpermanent:true

What do you all think?

I haven't looked at the internals ... but from the outside this looks "just" a syntactic sugar that should be "easy" to implement. So, the end-user writes this in YAML:

blog_show:path:/blog/{slug}redirect_to:/some-relative-urlpermanent:true

And Symfony, when parsing that YAML file, transforms it to this transparently:

blog_show:path:/blog/{slug}defaults:_controller:Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectActionpath:/some-relative-urlpermanent:true

So, this feature "just" needs to look for aredirect_to option and then, create thedefaults._controller anddefaults.path options automatically ... and let Symfony take care of the rest. No more changes would be needed because the verbose config already works: this feature "just" transforms concise config to verbose config internally. Would that work?

@javiereguiluz the real problem is that:

The problem with the current implementation is that is it specific to a few details about the routing and is sort of between the Routing component and the FrameworkBundle bundle. Redirection configuration (while allowed by the Routing component) wouldn't work without the FrameworkBundle, which is not what we'd like.

@sroze this is how I imagine the implementation of this feature (the code doesn't work, I just invented it to show what I'm asking for):

public function processRoutes($yamlContent){    $routesConfig = Yaml::parse($yamlContent);    foreach ($routesConfig as $routeConfig) {+        if (isset($routeConfig['redirect_to'])) {+           $routeConfig['_defaults']['_controller'] = 'Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectAction';+           $routeConfig['_defaults']['path'] = $routeConfig['redirect_to'];+           unset($routeConfig['redirect_to']);+       }        // ...    }    return $routeCollection;}

Yes, I agree with you. But it would mean the Routing component depends on the FrameworkBundle (at least, the way it is now).

I'm now convinced the aliasing feature is all that is needed at least as a first step. This should be a feature of RouterListener in HttpKernel IMHO (could also be in Routing matchers, but I feel better with RouterListener). So down this path, we would just need aRouterListener::setControllerAliases(array $aliases) and done.

Javier has another goal, which is to move defaults out of the "defaults" nesting level.
This goal is unrelated to redirection to me. Or, if it's for redirections only, then I'm very unsure, as this would just create a new specific case that ppl will need to learn.

All-in-all, the controller aliasing looks like the right next step to me.

To better compare the alternatives, here is a summary:

Current situation

blog_show:path:/blog/{slug}defaults:_controller:Symfony\Bundle\FrameworkBundle\Controller\RedirectController::urlRedirectActionpath:/some-relative-urlpermanent:true

Proposal 1

blog_show:path:/blog/{slug}redirect_to:/some-relative-urlpermanent:true

Proposal 2

blog_show:path:/blog/{slug}defaults:_controller:_redirect_pathpath:/some-relative-urlpermanent:true

I think it's OK to create aredirect_to option because we recently moved thedefaults._controller option to justcontroller and people loved it.

I'm 👎 for adding redirect specific root-options because it creates a very specific case people will have to learn, whereas the proposed alternative (2) just leverages existing and new generic knowledge. The redirect-specific entries would also prevent using new or exotic redirection options, because these are specific to the actual redirector implementation.

The only counter-argument to@nicolas-grekas' PoV I can see is that a redirection is not a "very specific case" of routing.

From a user PoV, I like theredirect_to option from Javier. Looks neat to me. That does not work well as it would not allow for specific HTTP status code, ...

Let's close this one, as this direction is a dead end :)