Use the router to resolve the file links in the profiler #24153
Conversation
| Q | A |
|---|---|
| Branch? | 3.3 |
| Bug fix? | yes |
| New feature? | no |
| BC breaks? | no |
| Deprecations? | no |
| Tests pass? | yes |
| Fixed tickets | #22290 |
| License | MIT |
| Doc PR | - |
|
|
||
| public function __construct($fileLinkFormat = null, RequestStack $requestStack = null, $baseDir = null, $urlFormat = null) | ||
| public function __construct($fileLinkFormat = null, RouterInterface $router = null, $baseDir = null, $queryString = null) |
There was a problem hiding this comment.
the typehint should be UrlGeneratorInterface, as yo only want to generate (Silex does not have a service implementing RouterInterface for instance, as it has 2 services for the matcher and the generator)
|
|
||
| public function __construct($fileLinkFormat = null, RequestStack $requestStack = null, $baseDir = null, $urlFormat = null) | ||
| public function __construct($fileLinkFormat = null, RouterInterface $router = null, $baseDir = null, $queryString = null) |
There was a problem hiding this comment.
Changing the signature is a BC break (it might break SilexWebProfiler btw)
There was a problem hiding this comment.
If I'm right, we don't care about BC breaks in debug tools like this one.
|
|
||
| $this->assertSame('http://www.example.org/app.php/_profiler/open?file=file.php&line=3#line3', $sut->format($file, 3)); | ||
| return new Router(new ClosureLoader(), $routeCollection); |
There was a problem hiding this comment.
you could use return new UrlGenerator($routecollection, new RequestContext()) (with $routeCollection being the RouteCollection itself rather than a closure returning it)
|
|
||
| public function __construct($fileLinkFormat = null, RequestStack $requestStack = null, $baseDir = null, $urlFormat = null) | ||
| public function __construct($fileLinkFormat = null, UrlGeneratorInterface $router = null, $baseDir = null, $queryString = null) |
There was a problem hiding this comment.
The type change is a BC break. I saw the comment saying we don't care because this is debug tool, but I don't think we've done any such BC break before (we did some for the profiler, but they were behavior ones, not "interface" ones AFAIK.)
| ); | ||
| } | ||
|
|
||
| if (null === $this->router) { |
There was a problem hiding this comment.
If router is null, then we probably need to fallback using the request stack for BC compat.
| { | ||
| // TODO: '$requestStack' constructor argument is unused. Remove it in Symfony 4.0. |
There was a problem hiding this comment.
We won't be able to do that. What would be better is to remove the type hint for $requestStack, accept both a RequestStack and a UrlGeneratorInterface, trigger a deprecation if this is a RequestStack. That way, we can remove the deprecated part in 4.0.
|
@javiereguiluz Can you finish this one by taking account comments? Thanks. |
|
|
||
| public function __construct($fileLinkFormat = null, RequestStack $requestStack = null, $baseDir = null, $urlFormat = null) | ||
| public function __construct($fileLinkFormat = null, RequestStack $requestStack = null, $baseDir = null, $queryString = null, UrlGeneratorInterface $router = null) |
There was a problem hiding this comment.
What about doing what I suggested instead. Keeping the second argument, deprecating the fact that you can pass a RequestStack as a second argument, and accept a UrlGenerator as well? That way, we can remove the possibility to pass a RequestStack in 4.0, which is cleaner.
There was a problem hiding this comment.
nvm, I've implemented my suggestion in f659954
|
With the latest changes made by Fabien, this should be ready for the final review. |
|
|
||
| if (null !== $this->urlGenerator) { | ||
| return array( | ||
| $this->urlGenerator->generate('_profiler_open_file').$this->queryString, |
There was a problem hiding this comment.
That ties it to the definition of WebProfilerBundle. I suggest that we make it configurable instead?
There was a problem hiding this comment.
Could we please do that in the future in case enough people ask for it?
|
|
||
| public function __construct($fileLinkFormat = null, RequestStack $requestStack = null, $baseDir = null, $urlFormat = null) | ||
| /** | ||
| * @param $urlGenerator UrlGeneratorInterface |
There was a problem hiding this comment.
@param UrlGeneratorInterface $urlGenerator
|
moving to the 3.4 milestone as the last bugfix release for 3.3 was published today |
|
This looks overkill for the purpose. Instead, I would suggest to just make this hardcoded URL a parameter. And maybe populate this parameter automatically in a late compiler pass. |
|
Hum, in fact here is an approach that works: #22290 |