Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Zend\Console does not accept 0 in --argument=0 #3851

Closed
paul-vg opened this Issue Feb 21, 2013 · 19 comments

Comments

Projects
None yet
5 participants

paul-vg commented Feb 21, 2013

e.g. in module.config.php:

return array(
    'console' => array(
        'router' => array(
            'routes' => array(
                'application\update' => array(
                    'options' => array(
                        'route' => 'update inventory --lights=',
                        'defaults' => array(
                            'controller' => 'Application\Controller\Inventory',
                            'action'     => 'update',
                        ),
                    ),
                ),
            ),
        ),
    ),
 );

In InventoryController.php:

public function updateAction()
{
    echo 'There are: ', $this->getRequest()->getParam('lights'), " lights!\n";
}

Expected output:

$ php public/index.php update inventory --lights=0
There are: 0 lights!

Actual output:

$ php public/index.php update inventory --lights=0
PHP Notice:  Undefined offset: 2 in /home/jblow/ZendSkeletonApplication/vendor/zendframework/zendframework/library/Zend/Mvc/Router/Console/Simple.php on line 657
PHP Stack trace:
PHP   1. {main}() /home/jblow/ZendSkeletonApplication/public/index.php:0
PHP   2. Zend\Mvc\Application->run() /home/jblow/ZendSkeletonApplication/public/index.php:12
PHP   3. Zend\EventManager\EventManager->trigger() /home/jblow/ZendSkeletonApplication/vendor/zendframework/zendframework/library/Zend/Mvc/Application.php:275
PHP   4. Zend\EventManager\EventManager->triggerListeners() /home/jblow/ZendSkeletonApplication/vendor/zendframework/zendframework/library/Zend/EventManager/EventManager.php:204
PHP   5. call_user_func() /home/jblow/ZendSkeletonApplication/vendor/zendframework/zendframework/library/Zend/EventManager/EventManager.php:460
PHP   6. Zend\Mvc\RouteListener->onRoute() /home/jblow/ZendSkeletonApplication/vendor/zendframework/zendframework/library/Zend/EventManager/EventManager.php:460
PHP   7. Zend\Mvc\Router\SimpleRouteStack->match() /home/jblow/ZendSkeletonApplication/vendor/zendframework/zendframework/library/Zend/Mvc/RouteListener.php:65
PHP   8. Zend\Mvc\Router\Console\Simple->match() /home/jblow/ZendSkeletonApplication/vendor/zendframework/zendframework/library/Zend/Mvc/Router/SimpleRouteStack.php:292
There are: lights!

@Bittarman Bittarman closed this Feb 21, 2013

@Bittarman Bittarman reopened this Feb 21, 2013

Contributor

bakura10 commented Feb 21, 2013

I've alraedy opened an issue, this should have been fix in 2.1.2 iirc.

paul-vg commented Feb 21, 2013

Sorry for not mentioning my setup. I'm using ZF2.1.2, PHP 5.4.11 under Fedora release 17.

paul-vg commented Mar 1, 2013

It's actually worse than this because if you have 2 parameters, the entire argument following a 0 will be read instead of the 0. E.g.: --lights=0 --fingers=5 will set the key "lights" to value "--fingers=5".

Contributor

blanchonvincent commented Mar 3, 2013

@paul-vg is still an issue with 2.1.3 ?

paul-vg commented Mar 4, 2013

Yes, I just checked.

paul-vg commented Mar 5, 2013

Thanks, but that could introduce a regression regarding required arguments. For instance "--lights=" is now accepted but used to throw an exception. This is fine for me but might lead to unexpected results in other applications.

Contributor

blanchonvincent commented Mar 5, 2013

@paul-vg don't understand the problem. All unit tests passed, give me a use case for the regression please.

paul-vg commented Mar 5, 2013

The use case is in the documentation: http://framework.zend.com/manual/2.1/en/modules/zend.console.routes.html#value-flags

It is also possible to define mandatory value flags

A mandatory value flag without a value (e.g. "--firstName=") would throw an exception, which is the expected behaviour as all of the provided examples have a value.

Contributor

blanchonvincent commented Mar 5, 2013

@paul-vg thank you for the details

Contributor

blanchonvincent commented Mar 5, 2013

@paul-vg i think it's ok with the fix. A null param will not accepted.

paul-vg commented Mar 5, 2013

Null no, empty string yes.

Contributor

blanchonvincent commented Mar 5, 2013

@paul-vg ok i will fix that in the evening, thank you for the feedback

Contributor

blanchonvincent commented Mar 5, 2013

@paul-vg i updated the PR, can you take a look at the fix and test ? thx :)

paul-vg commented Mar 6, 2013

The fix also requires a value for normal (optional) value flags, e.g. this route:
update inventory [--lights=]
would no longer accept "update inventory --lights" (value separated by space, no value given) and "update inventory --lights=" (same but value separated by =).

If --lights= is accepted and read correctly, it becomes possible to do this: "--lights= --fingers=10" (which is arguably useful). It's not really avoidable to read "--lights --fingers" as "--lights=--fingers" (which is what it already did anyway), but breaking the empty string value flag parameter case looks like a regression if the behaviour should be consistent with the Http router (which does return null).

There might be a documentation bug in here because these corner cases aren't documented.

Contributor

blanchonvincent commented Mar 6, 2013

@paul-vg

"If --lights= is accepted and read correctly", no, "--lights=" is not accepted, it's fix.

For "update inventory [--lights=]", there is a problem ? "update inventory --lights=" is accepted ?

paul-vg commented Mar 6, 2013

Naturally, if a value flag parameter is mandatory "--lights=" should be rejected. That works. What I really meant was "--lights=" should be accepted and read when it's NOT mandatory. That would allow "--lights= --fingers=10" to work as expected.

And yes, that's the case where there's still a problem (AFAICT). "update inventory --lights=" is not accepted with the route you mentioned.

Owner

weierophinney commented Mar 6, 2013

@paul-vg Actually, when you specify an argument as [--lights=], the expectation is that the argument, if present, requires a value. As such, update inventory --lights= should not match, as it's invalid; no value was provided to the argument. Similarly, if you make the flag required, i.e., --lights=, the = indicates that a value is required.

paul-vg commented Mar 6, 2013

Well in my case that would be good enough but it'd still break BC.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment