in the file: core/lib/Drupal/Core/Menu/LocalTaskManager.php,
There is an unused variable found. $routes.
But it has comment like
// Pre-fetch all routes involved in the tree. This reduces the number
// of SQL queries that would otherwise be triggered by the access manager.
$routes = $route_names ? $this->routeProvider->getRoutesByNames($route_names) : array();Confirmed : the variable is unused any where else.
| Comment | File | Size | Author |
|---|---|---|---|
| #4 | 2609114-4.patch | 813 bytes | rakesh.gectcr |
| #2 | 2609114-1.patch | 720 bytes | rakesh.gectcr |
Comments
Comment #2
rakesh.gectcrIn the attached patch which i removed the variable.
Comment #3
chx commentedNormally I would recommend putting a note on RouteProviderInterface::getRoutesByNames saying it is expected to help with such optimization but since it's Symfony, you can't fix it. If you feel very adventurous you can file an upstream PR for documentation saying so but I doubt they would accept it.
So just change the code to
because according to the comment this leads to a speedup and in the SQL implementation it does.
Comment #4
rakesh.gectcr@chx,
You rocks!
I have done the changes according to your comment.
Comment #5
chx commentedComment #6
dawehnerIs it just me that given that #2 passed we should ideally create some test coverage here?
Comment #7
tim.plunkettAgreed.
Comment #8
chx commentedDisagreed. #2 passed because it removed a performance optimization but nothing else. The comment clearly tells you this. What do you want to test? That the SQL implementation of getRoutesByNames preloades and then getRoutesByName doesn't hit the database? That doesn't belong to this issue.
Comment #9
dawehnerSo yeah what @tim.plunkett and @dawehner wants to test:
We want to ensure that we preload the routes, which is non obvious, especially, with nested local tasks ...
Comment #10
dawehnerRight, this issue is also not 'rc eligible' because it touches non test code.
Comment #11
chx commentedWell, to be honest, it removes a not used variable and does not do anything else.
Comment #13
xjmThanks! Normally we could call this RC eligible since it is only removing an unused variable, actually, but it makes sense to check in this case since it's changing a line of code. So tagging as an RC target.
I don't think we need tests here. The inline comment already tells us what we need to know, and the original patch in #2 was problematic because it did not heed the inline comment.
Since this particular unused variable removal involves changing an existing line of code, it makes sense to have it in its own issue. However, for the related issues that just involve deleting a line that has an unused variable -- those are very easy to review as a single scope by checking for the variable in scope and (as we discovered on this issue) reading the removed line and lines around it. So let's combine those other issues in #2610116: Removing unused variables, but committed and pushed this one to 8.0.x. Thanks!
Comment #14
dawehnerWow, did we really committed this without test coverage?