Add posibility to specify method in router:match#9340
Add posibility to specify method in router:match#9340pmartelletti wants to merge 8 commits intosymfony:masterfrom
Conversation
I think this was not possible, or I just was not able to find it anywhere, so I just implemented myself.
|
Should I do a new PR with the suggested changes? |
There was a problem hiding this comment.
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? 😟 |
There was a problem hiding this comment.
what do you mean by reverted? That it should not be marked as modified in the diff?
There was a problem hiding this comment.
those 2 lines should be moved to the setDefinition call like done in other Symfony commands.
There was a problem hiding this comment.
In the AssetsInstallCommand options are set like this, that's why I opted to make it this way. 😟 Sorry about that.
There was a problem hiding this comment.
can you add a comma at the end of this one too?
There was a problem hiding this comment.
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? |
There was a problem hiding this comment.
I would remove the m shortcut here and use null instead like you did for the host.
There was a problem hiding this comment.
Forces to match against the specified method -> Sets the HTTP method
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
I think the default should be all methods, not only GET?
There was a problem hiding this comment.
The default cannot be all method. The request context has only 1 method.
|
Can you add the PR header in your description? (see http://symfony.com/doc/current/contributing/code/patches.html#make-a-pull-request)? |
There was a problem hiding this comment.
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.
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.
by changing the parameter
What parameter are you referring to?
There was a problem hiding this comment.
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.
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!