Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.6k
Add posibility to specify method in router:match#9340
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
I think this was not possible, or I just was not able to find it anywhere, so I just implemented myself.
| @@ -50,12 +50,13 @@ protected function configure() | |||
| ->setName('router:match') | |||
| ->setDefinition(array( | |||
| new InputArgument('path_info', InputArgument::REQUIRED, 'A path info'), | |||
| new InputArgument('_method', InputArgument::OPTIONAL, 'Forces to match against the specified method (optional)') | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
without underscore
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
👍
Should I do a new PR with the suggested changes? |
| $matcher = new TraceableUrlMatcher($router->getRouteCollection(), $router->getContext()); | ||
| $context = $router->getContext(); | ||
| $method = $input->getArgument('method'); | ||
| if ( $method ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
without spaces insde ()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
please have a look at the symfony cs
@pmartelletti I like the PR. I also had this idea. You don't need a new PR. Updating this one is fine. |
we would need |
Do you want me to add 'host' as an optiona paramter too? |
What do you think about it now? 😟 |
| @@ -49,13 +50,25 @@ protected function configure() | |||
| $this | |||
| ->setName('router:match') | |||
| ->setDefinition(array( | |||
| new InputArgument('path_info', InputArgument::REQUIRED, 'A path info'), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
should be reverted
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
what do you mean by reverted? That it should not be marked as modified in the diff?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you removed the ending comma
| )) | ||
| ->addOption('method', 'm', InputOption::VALUE_OPTIONAL, 'Forces to match against the specified method') | ||
| ->addOption('host', null, InputOption::VALUE_OPTIONAL, 'Forces the host to be the one specified by this parameter') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
those 2 lines should be moved to thesetDefinition call like done in other Symfony commands.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
In the AssetsInstallCommand options are set like this, that's why I opted to make it this way. 😟 Sorry about that.
| @@ -50,12 +51,14 @@ protected function configure() | |||
| ->setName('router:match') | |||
| ->setDefinition(array( | |||
| new InputArgument('path_info', InputArgument::REQUIRED, 'A path info'), | |||
| new InputOption('method', 'm', InputOption::VALUE_OPTIONAL, 'Forces to match against the specified method'), | |||
| new InputOption('host', null, InputOption::VALUE_OPTIONAL, 'Forces the host to be the one specified by this parameter') | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
can you add a comma at the end of this one too?
| @@ -67,7 +70,17 @@ protected function configure() | |||
| protected function execute(InputInterface $input, OutputInterface $output) | |||
| { | |||
| $router = $this->getContainer()->get('router'); | |||
| $matcher = new TraceableUrlMatcher($router->getRouteCollection(), $router->getContext()); | |||
| $context = $router->getContext(); | |||
| $method = $input->getOption('method'); | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
you could inject elegantly the assignments into the if expressions
if ($method = $input->getOption('method')) {a la@fabpot
also add some command testing |
Great addition..three days ago I was disappointed to find out that it wasn’t implemented yet! |
@cordoval the actual command does not have any testing, or at least it's not under where I expected it should be. If tests are somewhere else, would be appreciated if you point me where so I don't duplicate testing. 😄 |
You're correct, there are no tests for this command. Could you please create them? |
| @@ -50,12 +51,14 @@ protected function configure() | |||
| ->setName('router:match') | |||
| ->setDefinition(array( | |||
| new InputArgument('path_info', InputArgument::REQUIRED, 'A path info'), | |||
| new InputOption('method', 'm', InputOption::VALUE_OPTIONAL, 'Forces to match against the specified method'), | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I would remove them shortcut here and usenull instead like you did for the host.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Forces to match against the specified method ->Sets the HTTP method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
VALUE_OPTIONAL should also be changed. It's not optional because just specifying--method doesn't make sense.
So instead it should be required and specify a default (GET) for it (fifth argument AFAIK).
The same also applies for host (localhost). These are the defaults ofhttps://github.com/symfony/symfony/blob/master/src/Symfony/Component/Routing/RequestContext.php#L53 as well.
Also it would be nice to add one more option: scheme (defaults tohttp).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I think the default should be all methods, not only GET?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The default cannot be all method. The request context has only 1 method.
Can you add the PR header in your description? (seehttp://symfony.com/doc/current/contributing/code/patches.html#make-a-pull-request)? |
| } | ||
| if ($host = $input->getOption('host')) { | ||
| $context->setHost($host); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
After applying the changes asked by@Tobion, you can safely remove the conditions and always set the values (as it would set the default value if the options are not passed explicitly).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I disagree here. The default values of the request context can be changed by changing the parameter. It would be weird if the command starts ignoring these overwritten defaults by always setting its own value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
by changing the parameter
What parameter are you referring to?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If you mean a custom subclass with changed parameter defaults, then IMHO we should still set the values in the command to make it independent and work consistently. Otherwise the command would behave differently depending on what request context is used.
@pmartelletti do you have time to finish this one off? There's little left to do (remember toadd the PR table). |
Yes i will , as soon as i'm back from holidays later next week. Sorry about El miércoles, 8 de enero de 2014, Jakub Zalas escribió:
Pablo María Martelletti |
Should a EDIT: Sorry, I have just realised that this was said before. |
I've totally forgot about this PR. I have to make some updates on it and update existing one. |
This can be closed in favor of#10439 |
… host in router:match command (romainneutron)This PR was merged into the 2.5-dev branch.Discussion----------[FrameworkBundle] Add posibility to specify method and host in router:match command| Q | A| ------------- | ---| Bug fix? | yes| New feature? | yes| BC breaks? | no| Deprecations? | no| Tests pass? | yes| License | MITReplaces#9340Commits-------acc66b9 [FrameworkBundle] Add posibility to specify method and host in router:match command
I think this was not possible, or I just was not able to find it anywhere, so I just implemented myself. Please feel free to ignore this PR if this was already possible from the command line, but please DO tell me how is it done. Thanks!