Skip to content

Routes with trailing slash broken after upgrade to 4.1.8 #29363

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.

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
jimmycleuren opened this issue Nov 28, 2018 · 9 comments
Closed

Routes with trailing slash broken after upgrade to 4.1.8 #29363

jimmycleuren opened this issue Nov 28, 2018 · 9 comments

Comments

@jimmycleuren
Copy link

jimmycleuren commented Nov 28, 2018

Symfony version(s) affected: 4.1.8

Description
After the upgrade to 4.1.8, some routes with a trailing slash don't work anymore.

How to reproduce

/**
 * @Route("/user")
 */
class UserController extends Controller
{
    /**
     * @Route("/", name="user_index", methods="GET")
     */
    public function index(): Response
    {
    }
}

The route "/user/" works in 4.1.7, but gives the following error in 4.1.8:

2/2
Symfony\Component\HttpKernel\Exception\NotFoundHttpException:
No route found for "GET /user/"

  at vendor/symfony/http-kernel/EventListener/RouterListener.php:139
  at Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest(object(GetResponseEvent), 'kernel.request', object(TraceableEventDispatcher))
     (vendor/symfony/event-dispatcher/Debug/WrappedListener.php:111)
  at Symfony\Component\EventDispatcher\Debug\WrappedListener->__invoke(object(GetResponseEvent), 'kernel.request', object(EventDispatcher))
     (vendor/symfony/event-dispatcher/EventDispatcher.php:212)
  at Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(array(object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener)), 'kernel.request', object(GetResponseEvent))
     (vendor/symfony/event-dispatcher/EventDispatcher.php:44)
  at Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.request', object(GetResponseEvent))
     (vendor/symfony/event-dispatcher/Debug/TraceableEventDispatcher.php:141)
  at Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher->dispatch('kernel.request', object(GetResponseEvent))
     (vendor/symfony/http-kernel/HttpKernel.php:125)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (vendor/symfony/http-kernel/HttpKernel.php:66)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor/symfony/http-kernel/Kernel.php:188)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (public/index.php:37)

1/2
Symfony\Component\Routing\Exception\ResourceNotFoundException:

  at /tmp/cache/dev/srcDevDebugProjectContainerUrlMatcher.php:50
  at srcDevDebugProjectContainerUrlMatcher->match('/user')
     (vendor/symfony/routing/Matcher/UrlMatcher.php:107)
  at Symfony\Component\Routing\Matcher\UrlMatcher->matchRequest(object(Request))
     (vendor/symfony/routing/Router.php:262)
  at Symfony\Component\Routing\Router->matchRequest(object(Request))
     (vendor/symfony/http-kernel/EventListener/RouterListener.php:115)
  at Symfony\Component\HttpKernel\EventListener\RouterListener->onKernelRequest(object(GetResponseEvent), 'kernel.request', object(TraceableEventDispatcher))
     (vendor/symfony/event-dispatcher/Debug/WrappedListener.php:111)
  at Symfony\Component\EventDispatcher\Debug\WrappedListener->__invoke(object(GetResponseEvent), 'kernel.request', object(EventDispatcher))
     (vendor/symfony/event-dispatcher/EventDispatcher.php:212)
  at Symfony\Component\EventDispatcher\EventDispatcher->doDispatch(array(object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener), object(WrappedListener)), 'kernel.request', object(GetResponseEvent))
     (vendor/symfony/event-dispatcher/EventDispatcher.php:44)
  at Symfony\Component\EventDispatcher\EventDispatcher->dispatch('kernel.request', object(GetResponseEvent))
     (vendor/symfony/event-dispatcher/Debug/TraceableEventDispatcher.php:141)
  at Symfony\Component\EventDispatcher\Debug\TraceableEventDispatcher->dispatch('kernel.request', object(GetResponseEvent))
     (vendor/symfony/http-kernel/HttpKernel.php:125)
  at Symfony\Component\HttpKernel\HttpKernel->handleRaw(object(Request), 1)
     (vendor/symfony/http-kernel/HttpKernel.php:66)
  at Symfony\Component\HttpKernel\HttpKernel->handle(object(Request), 1, true)
     (vendor/symfony/http-kernel/Kernel.php:188)
  at Symfony\Component\HttpKernel\Kernel->handle(object(Request))
     (public/index.php:37)

Additional context

Route is still visible in the debug:router command
Downgrade to 4.1.7 resolved the issue

@nicolas-grekas
Copy link
Member

Hello, I just tried a 4.1.8 app with this exact controller, it works well.
Could you please provide a reproducing app?

@jimmycleuren
Copy link
Author

It seems I can only simulate it in my private repo as well. What I do notice, is that the route that is not working, is located in the $regexList of srcDevDebugProjectContainerUrlMatcher.php::doMatch.

But I don't know what the reason is for a route to move there instead of the initial switch statement and can't simulate it with a test app.

@Anag13
Copy link

Anag13 commented Nov 29, 2018

Hi, we can also confirm this from our end after the 4.1.8 update - none of the usual REST endpoints with trailing slashes work anymore and we experience the same behaviour as @jimmycleuren. Same as in his case, downgrading to 4.1.7 also fixes the issue.

Not sure if it helps in identifying the problem, but setting redirection support to false will also fix the issue.

// Does not work
$this->supportsRedirections = isset($interfaces[RedirectableUrlMatcherInterface::class]);

// Works same as before
$this->supportsRedirections = false;

@nicolas-grekas
Copy link
Member

From @murnieza in #29287 (comment):

I have route /foo/{bar<.*>} and after this change /foo/ does not match my route anymore.

I'm on it.

@nicolas-grekas
Copy link
Member

Hi everyone, please check #29373 and confirm it fixes your issues if you can.

nicolas-grekas added a commit that referenced this issue Nov 29, 2018
This PR was merged into the 4.1 branch.

Discussion
----------

[Routing] fix trailing slash redirection

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29363
| License       | MIT
| Doc PR        | -

Commits
-------

fbaba23 [Routing] fix trailing slash redirection
@julienfalque
Copy link
Contributor

I have a similar issue on a project where routes with and without trailing slash both exist. With symfony/routing v4.1.7 this worked fine but with v4.1.8, one of the routes never matches and a redirection to the other route occurs instead.

I added the following test to RedirectableUrlMatcherTest:

    public function testNoRedirectWhenBothRoutesExist()
    {
        $coll = new RouteCollection();
        $coll->add('foo_without_slash', new Route('/foo'));
        $coll->add('foo_with_slash', new Route('/foo/'));

        $matcher = $this->getUrlMatcher($coll);
        $matcher->expects($this->never())->method('redirect');
        $this->assertSame(array('_route' => 'foo_without_slash'), $matcher->match('/foo'));
        $this->assertSame(array('_route' => 'foo_with_slash'), $matcher->match('/foo/'));

        $coll = new RouteCollection();
        $coll->add('foo', new Route('/foo{trailingSlash}', array(), array('trailingSlash' => '/?')));

        $matcher = $this->getUrlMatcher($coll);
        $matcher->expects($this->never())->method('redirect');
        $this->assertSame(array('trailingSlash' => '', '_route' => 'foo'), $matcher->match('/foo'));
        $this->assertSame(array('trailingSlash' => '/', '_route' => 'foo'), $matcher->match('/foo/'));
    }

which passes on v4.1.7 but fails on v4.1.8. I wasn't able to make it pass without breaking other tests though.

@nicolas-grekas
Copy link
Member

nicolas-grekas commented Nov 29, 2018

@julienfalque AFAIK that behavior is consistent with how 3.4 behaves. If I'm not wrong, this means you relied on a bug that is now fixed.

@emodric
Copy link
Contributor

emodric commented Nov 30, 2018

In my case, I get a redirection loop between the routes with and without the slash.

It is not fixed by #29380, however.

nicolas-grekas added a commit that referenced this issue Nov 30, 2018
This PR was merged into the 4.1 branch.

Discussion
----------

[Routing] fix greediness of trailing slash

| Q             | A
| ------------- | ---
| Branch?       | 4.1
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #29363
| License       | MIT
| Doc PR        | -

Commits
-------

eb6aee6 [Routing] fix greediness of trailing slash
@nicolas-grekas
Copy link
Member

#29380 updated, should fix them all. Please report back if not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

7 participants