[FrameworkBundle] feature: add the ability to search a route #26121
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Travis failure is unrelated |
|
Can you add a test please? |
|
@dunglas I will do, I'm searching a way to add one, I have to look on other command's tests to test the choices. |
20uf
approved these changes
Feb 12, 2018
3a5df79 to
d002dd6
Compare
|
Status: needs review @dunglas test added |
41fb042 to
a9e228d
Compare
|
New test added that test the whole behaviour of the search. |
a9e228d to
6afe696
Compare
dunglas
requested changes
Feb 15, 2018
| @@ -142,4 +150,17 @@ private function extractCallable(Route $route) | |||
| } catch (\InvalidArgumentException $e) { | |||
| } | |||
| } | |||
|
|
|||
| private function findRouteNameContaining(string $name, RouteCollection $routes) | |||
There was a problem hiding this comment.
You can use the : array return type.
| @@ -79,6 +80,13 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
| $routes = $this->router->getRouteCollection(); | |||
|
|
|||
| if ($name) { | |||
| $matchingRoutes = $this->findRouteNameContaining($name, $routes); | |||
|
|
|||
| if (!empty($matchingRoutes) && !$route = $routes->get($name)) { | |||
There was a problem hiding this comment.
Can be simplified in=
$matchingRoutes = $this->findRouteNameContaining($name, $routes)
if ($matchingRoutes && !$route = $routes->get($name) {| { | ||
| $foundRoutesNames = array(); | ||
| foreach ($routes as $routeName => $route) { | ||
| if (false === stripos($routeName, $name)) { |
There was a problem hiding this comment.
Maybe can you invert the test, and put $foundRoutesNames[] = $routeName; in the if block to save 1 line (nitpicking).
6afe696 to
29561eb
Compare
stof
reviewed
Feb 15, 2018
| @@ -79,6 +80,12 @@ protected function execute(InputInterface $input, OutputInterface $output) | |||
| $routes = $this->router->getRouteCollection(); | |||
|
|
|||
| if ($name) { | |||
| $matchingRoutes = $this->findRouteNameContaining($name, $routes); | |||
There was a problem hiding this comment.
I would first try to get the route by exact name before searching route names. It is useless to search the matching routes if we already have a valid name.
|
I suggest to drop the existing |
|
@chalasr Oh... didn't know about the change in the release cycles and the limit of 5 minors. So I'll just overload the command with my own version of this change. |
e30c1cd to
635ce3a
Compare
|
Travis failure is unrelated. |
chalasr
approved these changes
Mar 20, 2018
| @@ -17,7 +17,8 @@ CHANGELOG | |||
| is either the service ID or the FQCN of the controller. | |||
| * Deprecated `Symfony\Bundle\FrameworkBundle\Controller\ControllerNameParser` | |||
| * The `container.service_locator` tag of `ServiceLocator`s is now autoconfigured. | |||
|
|
|||
| * Add the ability to search a route in `debug:router`. | |||
|
|
|||
fabpot
requested changes
Mar 20, 2018
| if (!$route = $routes->get($name)) { | ||
| if (!($route = $routes->get($name)) && $matchingRoutes = $this->findRouteNameContaining($name, $routes)) { | ||
| $default = 1 === count($matchingRoutes) ? $matchingRoutes[0] : null; | ||
| $name = $io->choice('Select one of the following routes to display its information', $matchingRoutes, $default); |
There was a problem hiding this comment.
Select one of the matching routes?
635ce3a to
0187162
Compare
|
@fabpot done. |
fabpot
reviewed
Mar 21, 2018
| if (!$route = $routes->get($name)) { | ||
| if (!($route = $routes->get($name)) && $matchingRoutes = $this->findRouteNameContaining($name, $routes)) { | ||
| $default = 1 === count($matchingRoutes) ? $matchingRoutes[0] : null; | ||
| $name = $io->choice('Select one of the matching routes to display its information', $matchingRoutes, $default); |
There was a problem hiding this comment.
I think the to display its information is not needed.
There was a problem hiding this comment.
I removed this part.
0187162 to
5e6007d
Compare
|
Status: Needs Review |
fabpot
approved these changes
Mar 21, 2018
javiereguiluz
approved these changes
Mar 21, 2018
| $this->assertContains('/session', $display); | ||
| } | ||
|
|
||
| public function testDumpOneRoutes() |
There was a problem hiding this comment.
Minor typo. testDumpOneRoutes -> testDumpOneRoute
5e6007d to
ef0df02
Compare
|
Thank you @Simperfit. |
fabpot
added a commit
that referenced
this pull request
Mar 21, 2018
… route (Simperfit) This PR was merged into the 4.1-dev branch. Discussion ---------- [FrameworkBundle] feature: add the ability to search a route | Q | A | ------------- | --- | Branch? | 4.1 | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no <!-- don't forget to update UPGRADE-*.md files --> | Tests pass? | yes | Fixed tickets | #26033 | License | MIT | Doc PR | symfony/symfony-docs#9236 This add the ability to search a route in the debug:router command.  Commits ------- ef0df02 [FrameworkBundle] feature: add the ability to search a route
javiereguiluz
added a commit
to symfony/symfony-docs
that referenced
this pull request
Mar 23, 2018
…a route (Simperfit) This PR was merged into the master branch. Discussion ---------- [FrameworkBundle] feature: add the ability to search for a route The PR add the documentation for core's PR symfony/symfony#26121 Commits ------- 2d781a8 feature: add the ability to search for a route
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This add the ability to search a route in the debug:router command.
