kohana / minion

Everyone loves having a minion they can boss around
113 stars 76 forks source link

When associative array, $options[0] is undefined #101

Closed cyrrill closed 9 years ago

cyrrill commented 9 years ago

When $options contains the 'task' key in its associative array form, an exception will be thrown when trying to access $options[0]:

ErrorException [ 8 ]: Undefined offset: 0 ~ DOCROOT/vendor/kohana/minion/classes/Kohana/Minion/Task.php [ 105 ]

To replicate this independently of Minion's implementation, try running this:

php -r "echo ['task'=>'foo']['task'];" // Will correctly print out "task"

php -r "echo ['task'=>'foo'][0];" // Will throw PHP Notice: Undefined offset: 0 in Command line code on line 1

acoulton commented 9 years ago

@cyrgit thanks for this, any chance you could add a unit test for this behaviour?

cyrrill commented 9 years ago

@acoulton Thanks for reviewing these commits. I've added a unit test for the factory method.

acoulton commented 9 years ago

@cyrgit thanks very much, but the indentation in your test has gone a bit strange. Also, the test might be a little more robust if asserting that it's a Minion_Task_Help in both those cases (rather than a plain Minion_Task). In fact, since Minion_Task_Help is the default, you might consider adding a custom dummy task class in the test case and verifying it's definitely picked up correctly.

eg:

class Minion_TaskTest extends Kohana_Unittest_TestCase {
//...
    public function provider_factory()
    {
        return array(
            array(array('task' => 'tasktest:dummytask'), 'Minion_Tasktest_DummyTask'),
            array(array('tasktest:dummytask'), 'Minion_Tasktest_DummyTask'),
            array(array(), 'Minion_Task_Help')
        );
    }

    public function test_factory($options, $expect_class)
    {
        $this->assertInstanceOf($expect_class, Minion_Task::factory($options));
    }
//...
}

class Minion_Tasktest_DummyTask extends Minion_Task {}
cyrrill commented 9 years ago

@acoulton Whoops, my IDE was configured to use spaces for indentation as per PSR-2 (2.4). I'll change that to tabs.

Also, good call on adding robustness with another subclass. I'll update this asap

cyrrill commented 9 years ago

@acoulton I've put this on pause pending the results of the conversation in #102, which suggests moving away from an array based factory to a string named Task instead.