-
Notifications
You must be signed in to change notification settings - Fork 2.5k
Console route improvements #4449
Console route improvements #4449
Conversation
* @param array $constraints | ||
* @param array $defaults | ||
* @param array $aliases | ||
* @return \Zend\Console\ConsoleRouteMatcher |
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.
@return self
will work here.
I like this approach! :) Scheduling for 2.3.0. |
Updated! |
$this->defaults = $defaults; | ||
$this->constraints = $constraints; | ||
$this->aliases = $aliases; | ||
$this->matcher = new ConsoleRouteMatcher($route, $constraints, $defaults, $aliases); |
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.
Perhabs we should add an interface for the console route matcher class. Add typehint on the constructor and allow passing the route matcher. If null is provided, we can still use the default implementation always.
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 was considering about making it replaceable, but then I didn't think it would be useful (that's also why $matcher
property was initially private).
If you think otherwise - sure, I can update it :-) But I have one concern here: constructor accepts filters and validators, but it looks like they are not used anywhere. Can I remove them before adding new parameter (in order to clear the code), or would you consider it as BC break?
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.
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.
Well, removing parameters is of course a BC break. I guess that nearly nobody instantiates this class manually, so this could be perhabs a possible BC, you should better ask @Thinkscape about it. He can give you much better feedback on that topic.
If you can say for sure, that there is no usecase in replacing the matcher with an alternative implementation, i'm good with your implementation.
For extendability reasons I always prefer to code against an interface and be able to replace every single implementation, but could be, that it's not needed in this case.
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 are missing features that did not make it into "2.0 final".
The best solution would be to implement those:
$constraints
should work the same as in http routes - copy-paste would most likely work fine + tests.$defaults
would need to be implemented in the "matcher" same as in the route object.$aliases
are self-explanatory.$filters
is just an array ofZend\Filter\FilterInterface
instance (i.e. filter chain)$validators
is an array of instances ofZend\Validator\ValidatorInterface
.
Filters go through captured param values, then validators are invoked. If any validator fails, this equals to route mismatch (return false
).
This way we'd have both the BC and all features covered.
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.
$filters is just an array of Zend\Filter\FilterInterface instance (i.e.
filter chain)
$validators is an array of instances of Zend\Validator\ValidatorInterface.
I'd argue these should be of type Zend\Filter\FilterChain
and
Zend\Validator\ValidatorChain
, respectively; that allows us to typehint,
and ensures consistency with the rest of the framework.
On Thu, May 16, 2013 at 12:33 PM, Artur Bodera [email protected]:
In library/Zend/Mvc/Router/Console/Simple.php:
@@ -97,9 +73,7 @@ public function __construct(
$filters = null,
$validators = null
) {
$this->defaults = $defaults;
$this->constraints = $constraints;
$this->aliases = $aliases;
$this->matcher = new ConsoleRouteMatcher($route, $constraints, $defaults, $aliases);
Those are missing features that did not make it into "2.0 final".
The best solution would be to implement those:
- $constraints should work the samehttp://framework.zend.com/manual/2.0/en/modules/zend.mvc.routing.html#zend-mvc-router-http-segmentas in http
routeshttps://github.com/zendframework/zf2/blob/master/library/Zend/Mvc/Router/Http/Segment.php#L213- copy-paste would most likely work fine + tests.- $defaults would need to be implemented in the "matcher" same as in
the route object.- $aliases are self-explanatory.
- $filters is just an array of Zend\Filter\FilterInterface instance
(i.e. filter chain)- $validators is an array of instances of
Zend\Validator\ValidatorInterface.Filters go through captured param values, then validators are invoked. If
any validator fails, this equals to route mismatch (return false).This way we'd have both the BC and all features covered.
—
Reply to this email directly or view it on GitHubhttps://github.com//pull/4449/files#r4260060
.
Matthew Weier O'Phinney
[email protected]
http://mwop.net/
definitely a good approach, thanks! |
OK. Currently route constructor works like before: $route = new Simple('--foo='); ... or it can be injected with custom matcher: use Zend\Console\RouteMatcher\RouteMatcherInterface;
use Zend\Mvc\Router\Console\Simple;
class CustomRouteMatcher implements RouteMatcherInterface
{
public function match($params)
{
// ...
}
}
$matcher = new CustomRouteMatcher('custom:route:definition');
$route = new Simple($matcher); @Thinkscape: I will try to find some time to implement missing features you mentioned, and open separate PR. |
@weierophinney agreed. Chain actually implements respective Interface, so it's better to hint for @mtymek forget arrays, see above. Separate PR means this PR will have BC break. It'd be best if the second one came before this one. Just watch out for merge conflicts. |
@Thinkscape just thinking, there's still plenty of time before v2.3, so I could actually do everything in one go. Expect some questions though, here you have a few to start with :-)
|
|
I added support for aliases and tests. As for filters and validators, if I change them from arrays to |
Take a look at InputFilter for some inspiration. That might be a decent approach... |
I don't get it. Imagine following route:
I need different validation rules for name and email, so I cannot pass single $nameValidator = new Zend\Validator\ValidatorChain();
$nameValidator->add(new Zend\Validator\StringLength());
$nameValidator->add(new Zend\I18n\Validator\Alpha());
$emailValidator = new Zend\Validator\EmailAddress();
$validators = ;
$matcher = new DefaultRouteMatcher(
'add-user <name> <email>',
array(),
array(),
array(),
array(),
array(
'email' => $emailValidator,
'name' => $nameValidator,
)
); Or am I missing something here? |
Yes, "array of". |
Done - please review. |
Looks clear. Great job :-) |
@@ -68,19 +50,14 @@ class Simple implements RouteInterface | |||
protected $assembledParams = array(); | |||
|
|||
/** | |||
* @var \Zend\Validator\ValidatorChain | |||
* @var \Zend\Console\RouteMatcher\DefaultRouteMatcher |
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.
Shouldn't this be RouteMatcherInterface
instead of DefaultRouteMatcher
?
…atcher Console route improvements
- Ensured docblock references were correct
…re/separate_console_route_matcher Console route improvements
- Ensured docblock references were correct
This PR attempts to finish missing features in console routes. It also extracts code responsible for parsing command line arguments into separate class under
Zend\Console
namespace. This allows using console commands without full MVC stack (even in non ZF2-based projects), for instance:ConsoleRouteMatcher is now used internally by
Zend\Mvc\Router\Console\Simple
.