Improve error reporting in router panel of web profiler

[javiereguiluz]

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

Problem

If you define a route condition like this:

app:
    resource: '@AppBundle/Controller/'
    type:     annotation
    condition: "request.server.get('HTTP_HOST') matches '/.*\.dev/'"

When browsing the Routing panel in the web profiler, you see an exception:

Why?

Because the route condition uses the request object, but the special TraceableUrlMatcher class doesn't get access to the real request object but to the special object obtained via:

$request = $profile->getCollector('request');

These are the contents of this pseudo-request:

request.server.get(...) condition fails because request.server is null. The full exception message shows this:

Solution

I propose to catch all exceptions in TraceableUrlMatcher and display an error message with some details of the exception:

[xabbuh]

Tests are falling now.

Status: Needs work

[javiereguiluz]

I've fixed tests and I've added a test for the new error message.

[OskarStark]

LGTM

[yceruto]

@javiereguiluz I've investigated this problem too and I found a way to display the traces instead of display an exception message:

+use Symfony\Component\HttpFoundation\Request;

class TraceableUrlMatcher extends UrlMatcher
{
     //...
+   public function getTracesRequest(Request $request)
+   {
+       $this->request = $request;
+
+       $ret = $this->getTraces($request->getPathInfo());
+
+       $this->request = null;
+
+       return $ret;
+   }
}

And

use Symfony\Component\HttpFoundation\Request;

class RouterController
{
    //...
    public function panelAction($token)
    {
        //...

+       /** @var RequestDataCollector $request */
        $request = $profile->getCollector('request');

+       $traceRequest = Request::create(
+           $request->getPathInfo(),
+           $request->getRequestServer()->get('REQUEST_METHOD'),
+           $request->getRequestAttributes()->all(),
+           $request->getRequestCookies()->all(),
+           [],
+           $request->getRequestServer()->all()
+       );

        return new Response($this->twig->render('@WebProfiler/Router/panel.html.twig', array(
            'request' => $request,
            'router' => $profile->getCollector('router'),
-           'traces' => $matcher->getTraces($request->getPathInfo()),
+           'traces' => $matcher->getTracesRequest($traceRequest),
        )), 200, array('Content-Type' => 'text/html'));
    }
}

WDYT?

[javiereguiluz]

I've made the changes suggested by @yceruto. Everything works as expected and his solution is much better than mine. Now you can see better error messages when a route with an expression doesn't match:

Thanks @yceruto!

[yceruto]

fabbot suggest to use single quote here.

[yceruto]

AppVeyor build faild (no related)

[fabpot]

getTraces*For*Request I think this is the pattern we are using elsewhere.

[fabpot]

LGTM

[fabpot]

Thank you @javiereguiluz.

[stof]

@javiereguiluz not passing a Request object when evaluating the condition should be considered as a bug IMO. Assuming that the collector has a compatible-enough API is not fine.

[stof]

sorry, just saw that it was indeed done properly in the final diff. I commented too fast

[peshi]

@javiereguiluz

Attempted to load class "Request" from namespace "Symfony\Bundle\WebProfilerBundle\Controller".

In vendor/symfony/symfony/src/Symfony/Bundle/WebProfilerBundle/Controller/RouterController.php
Line 86. $traceRequest = Request::create(

Missing "use" statement?

[xabbuh]

@peshi fixed in #18340