symfony-cmf / routing-auto

RoutingAuto component
https://cmf.symfony.com
Other
6 stars 11 forks source link

Router->generate always creates links a redirection route while using LeaveRedirectDefunctRouteHandler #31

Closed UFTimmy closed 9 years ago

UFTimmy commented 9 years ago

Howdy,

I have a Page document, and it is a RouteReferrersInterface. So it has a collection of all Routes that link to it. Everything is great, when the page is created, the collection has just the one route that points directly to the Page.

However, when I move that Page, I have the Defunct Route Handler setup to create a redirection route. Now there are two Routes in the Collection. The first route in the Collection is the redirection route. The second Route in the Collection is the direct link to the Page.

Now when I call $this->generateUrl from any Controller, the Router will always generate URLs to the first Route in the collection. In ContentAwareGenerator::getRouteByContent, it loops through the Routes in the collection and returns the first one that matches the locale. I don't use locale based routing, so it's always the first route, which is a redirection route. If no routes match the locale, then it just picks the first route, which would just be a redirection route anyway.

Is there any way to make the LeaveRedirectDefunctRouteHandler modify the first route to be a direct ink to the content, and add routes to the end of the collection that redirect? This way the generation of URLs will always generate URLs directly to the content. This saves an HTTP redirection request, and generates URLs that make more sense to the users.

Thanks!

dantleech commented 9 years ago

Thats a tricky one. I am not sure we would be able to "reorder" the route to be the first one, as routes could be anywhere in the tree.

I think a solution would be to introduce route "priority", where the route with the highest (?) priority wins. The RoutingAutoBundle's PHPCR-ODM adapter could then set a low priority for the redirect routes and a high one for the primary route. Would need some change in the RoutingBundle however /cc @dbu

dbu commented 9 years ago

hm, this is a topic that came up before, in the context of SEO when you may have several routes for the same content, but only one is the canonical. the route references the content, so to know the routes of a content, we look up the referrers - however, this has no way of ordering.

a workaround right now would be to manipulate the order of what you return from the content. or to extend the router and skip redirections. but both are not elegant solutions.

maybe indeed we should have an optional interface for priorities and treat routes without that interface as priority 0. then other routes can go < 0 to come later, or > 0 to be before normal routes... @ElectricMaxxx wdyt?

UFTimmy commented 9 years ago

Thanks for workaround, @dbu. I was able to throw together a quick and dirty route sorting implementation in Page::getRoutes() that works for my purposes. I will keep an eye out for a more permanent, cleaner, solution.

dbu commented 9 years ago

created https://github.com/symfony-cmf/Routing/issues/130

@UFTimmy can you post your solution here for reference? in case somebody else hits the same problem...

UFTimmy commented 9 years ago

Like I said, it's quick and dirty, and I don't even have any non Auto Routes to test with at the moment.

    /**
     * When a redirect route is added, it's the first route in the collection of routes, and that route is the one
     * used to generate URLs. So the URL displayed will always result in a redirection, instead of the direct
     * access. Per suggestion on the open ticket, I am sorting the routes so that the direct route is first, so
     * the route generated will be direct.
     *
     * https://github.com/symfony-cmf/RoutingAuto/issues/31#issuecomment-73904458
     *
     * {@inheritDoc}
     */
    public function getRoutes()
    {
        /**
         * Routes are not auto routes and they don't redirect.
         * Auto routes are auto routes that don't redirect
         * Redirects are any route that redirects.
         */
        $buckets = ['routes' => [], 'autoRoutes' => [], 'redirectRoutes' => []];

        //## Sort routes into their correct bucket.
        foreach ($this->routes as $route) {
            if ($route instanceof AutoRouteInterface) {
                if ($route->getRedirectTarget()) {
                    $buckets['redirectRoutes'][] = $route;
                } else {
                    $buckets['autoRoutes'][] = $route;
                }
            } elseif ($route instanceof RedirectRouteInterface) {
                $buckets['redirectRoutes'][] = $route;
            } else {
                $buckets['routes'][] = $route;
            }
        }

        //## Flatten the buckets into one array, routes, auto, then redirect
        $orderedRoutes = [];
        array_walk_recursive(
            $buckets,
            function ($route) use (&$orderedRoutes) {
                $orderedRoutes[] = $route;
            }
        );

        return $orderedRoutes;
    }

And a little Unit Test for my purposes:

    /**
     * Verify that getRoutes sorts routes properly.
     */
    public function testGetRoutes()
    {
        $page              = new Page();
        $staticRoute       = new Route();
        $autoRoute         = new AutoRoute();
        $autoRedirectRoute = new AutoRoute();
        $autoRedirectRoute->setRedirectTarget($autoRoute);

        $page->setRoutes([$autoRedirectRoute, $autoRoute, $staticRoute]);
        $this->assertEquals(
            [$staticRoute, $autoRoute, $autoRedirectRoute],
            $page->getRoutes(),
            'getRoutes is not sorting the routes properly'
        );
    }