Add posibility to specify method in router:match #9340

pmartelletti · 2013-10-18T18:19:47Z

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!

I think this was not possible, or I just was not able to find it anywhere, so I just implemented myself.

Tobion · 2013-10-18T18:45:10Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php

@@ -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)')


without underscore

pmartelletti · 2013-10-18T18:48:37Z

Should I do a new PR with the suggested changes?

Tobion · 2013-10-18T18:54:00Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php

-        $matcher = new TraceableUrlMatcher($router->getRouteCollection(), $router->getContext());
+        $context = $router->getContext();
+        $method = $input->getArgument('method');
+        if ( $method ) {


without spaces insde ()

please have a look at the symfony cs

Tobion · 2013-10-18T18:55:50Z

@pmartelletti I like the PR. I also had this idea. You don't need a new PR. Updating this one is fine.

stof · 2013-10-18T18:56:37Z

we would need host as well, as the routes can be aware of the host now

pmartelletti · 2013-10-18T18:58:32Z

Do you want me to add 'host' as an optiona paramter too?

pmartelletti · 2013-10-18T19:11:08Z

What do you think about it now? 😟

fabpot · 2013-10-28T10:35:34Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php

@@ -49,13 +50,25 @@ protected function configure()
        $this
            ->setName('router:match')
            ->setDefinition(array(
-                new InputArgument('path_info', InputArgument::REQUIRED, 'A path info'),


should be reverted

what do you mean by reverted? That it should not be marked as modified in the diff?

you removed the ending comma

fabpot · 2013-10-28T13:00:38Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php

            ))
+            ->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')


those 2 lines should be moved to the setDefinition call like done in other Symfony commands.

In the AssetsInstallCommand options are set like this, that's why I opted to make it this way. 😟 Sorry about that.

fabpot · 2013-10-28T13:07:45Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php

@@ -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')


can you add a comma at the end of this one too?

cordoval · 2013-10-28T17:35:08Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php

@@ -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');


you could inject elegantly the assignments into the if expressions

if ($method = $input->getOption('method')) {

a la @fabpot

cordoval · 2013-10-28T17:36:46Z

also add some command testing

hacfi · 2013-10-28T23:51:08Z

Great addition..three days ago I was disappointed to find out that it wasn’t implemented yet!

pmartelletti · 2013-11-06T06:13:53Z

@cordoval the actual command does not have any testing, or at least it's not under

FrameworkBundle/Tests/Command/

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. 😄

wouterj · 2013-11-06T06:36:44Z

You're correct, there are no tests for this command. Could you please create them?

fabpot · 2013-12-29T18:43:07Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php

@@ -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'),


I would remove the m shortcut here and use null instead like you did for the host.

Forces to match against the specified method -> Sets the HTTP method

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 of https://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 to http).

I think the default should be all methods, not only GET?

The default cannot be all method. The request context has only 1 method.

fabpot · 2013-12-29T18:45:49Z

Can you add the PR header in your description? (see http://symfony.com/doc/current/contributing/code/patches.html#make-a-pull-request)?

fabpot · 2013-12-30T06:35:30Z

src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php

+        }
+        if ($host = $input->getOption('host')) {
+            $context->setHost($host);
+        }


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).

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

by changing the parameter

What parameter are you referring to?

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.

jakzal · 2014-01-08T20:57:59Z

@pmartelletti do you have time to finish this one off? There's little left to do (remember to add the PR table).

pmartelletti · 2014-01-08T23:45:34Z

Yes i will , as soon as i'm back from holidays later next week. Sorry about
the delay!

El miércoles, 8 de enero de 2014, Jakub Zalas escribió:

@pmartelletti https://github.com/pmartelletti do you have time to
finish this one off? There's little left to do (remember to add the PR
tablehttp://fabbot.io/report/symfony/symfony/9340/bc9edc2bda5671e1df2901fb29a60076a48c862c
).

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/9340#issuecomment-31877145
.

Pablo María Martelletti

franmomu · 2014-03-05T16:23:21Z

Should a scheme option be added as well?

EDIT: Sorry, I have just realised that this was said before.

pmartelletti · 2014-03-05T16:40:15Z

I've totally forgot about this PR. I have to make some updates on it and update existing one.

romainneutron · 2014-03-13T13:28:56Z

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 | MIT Replaces #9340 Commits ------- acc66b9 [FrameworkBundle] Add posibility to specify method and host in router:match command

Add posibility to specify method in router:match

d9bfc62

I think this was not possible, or I just was not able to find it anywhere, so I just implemented myself.

Tobion reviewed Oct 18, 2013
View reviewed changes

update parameter name for method

a19d824

Tobion reviewed Oct 18, 2013
View reviewed changes

pmartelletti added 2 commits October 18, 2013 16:08

change the method and host arguments to options

97324a3

update command use help

0f6320e

fabpot reviewed Oct 28, 2013
View reviewed changes

Set option definitions in one line only

a6b927b

fabpot reviewed Oct 28, 2013
View reviewed changes

Change command definition

49cfc40

fabpot reviewed Oct 28, 2013
View reviewed changes

Update RouterMatchCommand.php

7329101

cordoval reviewed Oct 28, 2013
View reviewed changes

Asign variables in conditional

bc9edc2

fabpot reviewed Dec 29, 2013
View reviewed changes

fabpot reviewed Dec 30, 2013
View reviewed changes

romainneutron mentioned this pull request Mar 13, 2014

[FrameworkBundle] Add posibility to specify method and host in router:match command #10439

Merged

fabpot closed this Mar 13, 2014

Add posibility to specify method in router:match #9340

Add posibility to specify method in router:match #9340

Conversation

pmartelletti commented Oct 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pmartelletti commented Oct 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tobion commented Oct 18, 2013

stof commented Oct 18, 2013

pmartelletti commented Oct 18, 2013

pmartelletti commented Oct 18, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cordoval commented Oct 28, 2013

hacfi commented Oct 28, 2013

pmartelletti commented Nov 6, 2013

wouterj commented Nov 6, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Dec 29, 2013

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakzal commented Jan 8, 2014

pmartelletti commented Jan 8, 2014

franmomu commented Mar 5, 2014

pmartelletti commented Mar 5, 2014

romainneutron commented Mar 13, 2014