Skip to content

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 8 commits into from
Closed

Add posibility to specify method in router:match #9340

Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d9bfc62
Add posibility to specify method in router:match
pmartelletti Oct 18, 2013
a19d824
update parameter name for method
pmartelletti Oct 18, 2013
97324a3
change the method and host arguments to options
pmartelletti Oct 18, 2013
0f6320e
update command use help
pmartelletti Oct 18, 2013
a6b927b
Set option definitions in one line only
pmartelletti Oct 28, 2013
49cfc40
Change command definition
pmartelletti Oct 28, 2013
7329101
Update RouterMatchCommand.php
pmartelletti Oct 28, 2013
bc9edc2
Asign variables in conditional
pmartelletti Nov 6, 2013
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/Symfony/Bundle/FrameworkBundle/Command/RouterMatchCommand.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@

use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Input\InputOption;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Input\ArrayInput;
use Symfony\Component\Routing\RouterInterface;
Expand Down Expand Up @@ -50,12 +51,14 @@ protected function configure()
->setName('router:match')
->setDefinition(array(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be reverted

Copy link
Contributor Author

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?

Copy link
Member

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

new InputArgument('path_info', InputArgument::REQUIRED, 'A path info'),
new InputOption('method', 'm', InputOption::VALUE_OPTIONAL, 'Forces to match against the specified method'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

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

Copy link
Contributor

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

Copy link
Member

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?

Copy link
Member

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.

new InputOption('host', null, InputOption::VALUE_OPTIONAL, 'Forces the host to be the one specified by this parameter'),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Forces the host to be the one specified by this parameter => Sets the HTTP host

))
->setDescription('Helps debug routes by simulating a path info match')
->setHelp(<<<EOF
The <info>%command.name%</info> simulates a path info match:

<info>php %command.full_name% /foo</info>
<info>php %command.full_name% /foo -m METHOD --host HOST</info>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

-m -> --method

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would keep the old example and add a new one with real values instead of HOST (example.com for instance) and METHOD (POST for instance)

EOF
)
;
Expand All @@ -67,7 +70,15 @@ 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();
if ($method = $input->getOption('method')) {
$context->setMethod($method);
}
if ($host = $input->getOption('host')) {
$context->setHost($host);
}
Copy link
Member

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

Copy link
Member

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

Copy link
Contributor

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?

Copy link
Contributor

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.


$matcher = new TraceableUrlMatcher($router->getRouteCollection(), $context);

$traces = $matcher->getTraces($input->getArgument('path_info'));

Expand Down