Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Console route improvements #4449

Conversation

mtymek
Copy link
Contributor

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

* @param array $constraints
* @param array $defaults
* @param array $aliases
* @return \Zend\Console\ConsoleRouteMatcher
Copy link
Member

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.

@weierophinney
Copy link
Member

I like this approach! :) Scheduling for 2.3.0.

@mtymek
Copy link
Contributor Author

mtymek commented May 8, 2013

Updated!

$this->defaults = $defaults;
$this->constraints = $constraints;
$this->aliases = $aliases;
$this->matcher = new ConsoleRouteMatcher($route, $constraints, $defaults, $aliases);
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor

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.

Copy link
Member

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

Copy link
Member

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 notifications@github.51.alwrote:

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
matthew@weierophinney.net
http://mwop.net/

@prolic
Copy link
Contributor

prolic commented May 8, 2013

definitely a good approach, thanks!

@mtymek
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor Author

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
Copy link
Member

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

@mtymek
Copy link
Contributor Author

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
Copy link
Member

Yes, "array of".

@mtymek
Copy link
Contributor Author

mtymek commented May 20, 2013

Done - please review.

@Thinkscape
Copy link
Member

Looks clear. Great job :-)

@@ -68,19 +50,14 @@ class Simple implements RouteInterface
protected $assembledParams = array();

/**
* @var \Zend\Validator\ValidatorChain
* @var \Zend\Console\RouteMatcher\DefaultRouteMatcher
Copy link
Member

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?

@ghost ghost assigned weierophinney May 22, 2013
weierophinney added a commit that referenced this pull request May 22, 2013
weierophinney added a commit that referenced this pull request May 22, 2013
- Ensured docblock references were correct
weierophinney added a commit that referenced this pull request May 22, 2013
@weierophinney weierophinney merged commit 4421715 into zendframework:develop May 22, 2013
weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015
…re/separate_console_route_matcher

Console route improvements
weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015
- Ensured docblock references were correct
weierophinney added a commit to zendframework/zend-console that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants