Console route improvements #4449

Merged
merged 13 commits into from May 22, 2013

Projects

None yet

4 participants

@mtymek
Contributor
mtymek commented May 8, 2013

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:

use Zend\Console\ConsoleRouteMatcher;

array_shift($argv);
$matcher = new ConsoleRouteMatcher("action [--verbose|-v] <param>");
$matches = $matcher->match($argv);
print_r($matches);

ConsoleRouteMatcher is now used internally by Zend\Mvc\Router\Console\Simple.

@weierophinney weierophinney commented on an outdated diff May 8, 2013
library/Zend/Console/ConsoleRouteMatcher.php
+
+ /**
+ * Parameters' name aliases.
+ *
+ * @var array
+ */
+ protected $aliases;
+
+ /**
+ * Class constructor
+ *
+ * @param string $route
+ * @param array $constraints
+ * @param array $defaults
+ * @param array $aliases
+ * @return \Zend\Console\ConsoleRouteMatcher
@weierophinney
weierophinney May 8, 2013 Member

@return self will work here.

@weierophinney weierophinney commented on an outdated diff May 8, 2013
library/Zend/Console/ConsoleRouteMatcher.php
+ * @return \Zend\Console\ConsoleRouteMatcher
+ */
+ public function __construct(
+ $route,
+ array $constraints = array(),
+ array $defaults = array(),
+ array $aliases = array()
+ ) {
+ $this->defaults = $defaults;
+ $this->constraints = $constraints;
+ $this->aliases = $aliases;
+ $this->parts = $this->parseDefinition($route);
+ }
+
+
+ /** Parse a route definition.
@weierophinney
weierophinney May 8, 2013 Member

Move description down one line in the docblock.

@weierophinney weierophinney commented on an outdated diff May 8, 2013
library/Zend/Console/ConsoleRouteMatcher.php
+ 'hasValue' => false,
+ );
+ } else {
+ throw new Exception\InvalidArgumentException(
+ 'Cannot understand Console route at "' . substr($def, $pos) . '"'
+ );
+ }
+
+ $pos += strlen($m[0]);
+ $parts[] = $item;
+ }
+
+ return $parts;
+ }
+
+ public function match($params)
@weierophinney
weierophinney May 8, 2013 Member

Add a docblock here, please.

@weierophinney weierophinney commented on an outdated diff May 8, 2013
library/Zend/Console/ConsoleRouteMatcher.php
+ 'Cannot understand Console route at "' . substr($def, $pos) . '"'
+ );
+ }
+
+ $pos += strlen($m[0]);
+ $parts[] = $item;
+ }
+
+ return $parts;
+ }
+
+ public function match($params)
+ {
+ $matches = array();
+
+ /**
@weierophinney
weierophinney May 8, 2013 Member

This is an inline comment - remove the second *.

@weierophinney
weierophinney May 8, 2013 Member

(Do this throughout the code base where comments exist inside methods.)

@weierophinney weierophinney commented on an outdated diff May 8, 2013
library/Zend/Mvc/Router/Console/Simple.php
*/
- protected $validators;
-
- /**
- * @var \Zend\Filter\FilterChain
- */
- protected $filters;
+ private $matcher;
@weierophinney
weierophinney May 8, 2013 Member

Protected visibility, please.

@weierophinney
Member

I like this approach! :) Scheduling for 2.3.0.

@mtymek
Contributor
mtymek commented May 8, 2013

Updated!

@prolic prolic and 3 others commented on an outdated diff May 8, 2013
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);
@prolic
prolic May 8, 2013 Contributor

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.

@mtymek
mtymek May 9, 2013 Contributor

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?

@prolic
prolic May 13, 2013 Contributor

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.

@Thinkscape
Thinkscape May 16, 2013 Member

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

@weierophinney
weierophinney May 16, 2013 Member

$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 notifications@github.comwrote:

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/zendframework/zf2/pull/4449/files#r4260060
.

Matthew Weier O'Phinney
matthew@weierophinney.net
http://mwop.net/

@prolic
Contributor
prolic commented May 8, 2013

definitely a good approach, thanks!

@mtymek
Contributor
mtymek commented May 17, 2013

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.

@Thinkscape
Member

@weierophinney agreed. Chain actually implements respective Interface, so it's better to hint for FilterInterface and ValidatorInterface. This means one could provide a single or a chain of filters/validators.

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

@mtymek
Contributor
mtymek commented May 17, 2013

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

  • you mentioned constraints and defaults as if they were not implemented, but after looking at the code (+ doing running some test scripts), they seem to be working. I guess it's enough to add some unit tests, right?
  • it is not clear for me how aliases should work. How are they different from alternatives ("[-v|--verbose]", etc)?
@Thinkscape
Member
  1. yes
  2. -a|-b|-c is an alternative, not alias. Alias means foo == bar - allows you to map any number of aliases (keys) to any number of (named) params. This is a useful feature i.e. for keeping BC without changing controller logic.
@mtymek
Contributor
mtymek commented May 18, 2013

I added support for aliases and tests.

As for filters and validators, if I change them from arrays to FilterInterface and ValidatorInterface, how can I validate multiple parameters? I thought they should be an array with each element representing validators/filters applied on single param.

@Thinkscape
Member

Take a look at InputFilter for some inspiration. That might be a decent approach...

@mtymek
Contributor
mtymek commented May 18, 2013

I don't get it. Imagine following route:

add-user <name> <email>

I need different validation rules for name and email, so I cannot pass single ValidatorInterface to route matcher. I have to use an array:

$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?

@Thinkscape
Member

Yes, "array of".

@mtymek
Contributor
mtymek commented May 20, 2013

Done - please review.

@Thinkscape
Member

Looks clear. Great job :-)

@weierophinney weierophinney commented on the diff May 22, 2013
library/Zend/Mvc/Router/Console/Simple.php
@@ -68,19 +50,14 @@ class Simple implements RouteInterface
protected $assembledParams = array();
/**
- * @var \Zend\Validator\ValidatorChain
+ * @var \Zend\Console\RouteMatcher\DefaultRouteMatcher
@weierophinney
weierophinney May 22, 2013 Member

Shouldn't this be RouteMatcherInterface instead of DefaultRouteMatcher?

@weierophinney weierophinney added a commit that referenced this pull request May 22, 2013
@weierophinney weierophinney [#4449] docblock fixes
- Ensured docblock references were correct
0a2538c
@weierophinney weierophinney added a commit that referenced this pull request May 22, 2013
@weierophinney weierophinney Merge branch 'feature/4449' into develop
Close #4449
76b7b9c
@weierophinney weierophinney merged commit 4421715 into zendframework:develop May 22, 2013

1 check passed

default The Travis CI build passed
Details
@weierophinney weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge pull request zendframework/zendframework#4449 from mtymek/featu…
…re/separate_console_route_matcher

Console route improvements
a024b12
@weierophinney weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015
@weierophinney weierophinney [zendframework/zendframework#4449] docblock fixes
- Ensured docblock references were correct
91f70dc
@weierophinney weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015
@weierophinney weierophinney Merge branch 'feature/4449' into develop fde42e0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment