wa0x6e / Cake-Resque

Resque plugin for CakePHP : for creating background jobs that can be processed offline later
MIT License
159 stars 56 forks source link

Improved AppShell::perform() #62

Closed fhickman closed 10 years ago

fhickman commented 10 years ago

The documentation page for installation suggests that you add this function to your AppShell:

public function perform() {
    $this->initialize();
    $this->{array_shift($this->args)}();
}

I found that this function did not allow me to schedule jobs with commands that used non-positional options. Instead, I use:

public function perform()
{
    $this->initialize();
    return $this->runCommand($this->args[0], $this->args);
}

This way, options are parsed from the argument list in the same way that the Cake ShellDispatcher does.

Thanks!

wa0x6e commented 10 years ago

With runCommand(), you're passing $this->args as function parameters ?

Does that not require that the number of arguments to be known beforehand when writing the function ?

fhickman commented 10 years ago

runCommand is a function on CakePHP's Shell class (not my own code):

public function runCommand($command, $argv);

It takes care of parsing/validating the command line arguments, then invokes the right command/task.

wa0x6e commented 10 years ago

What I mean is that it is invoking function with the job option's as function parameters.

If I'm correct, a job like main, 0, 1, 2, 3 will call the function main($a, $b, $c, $d) {}.

It so, it's a change that is not backward compatible, and should be included only on the next major release.

fhickman commented 10 years ago

Hmm, if I understand what you're asking, I don't think that what you suggest is correct...

Here's a little test case I wrote to demonstrate the differences

// OldShell.php
App::uses('Shell', 'Console');
App::uses('CakeResque', 'Lib');

/**
 * Current Cake-Resque execution hook.
 */
class AppShellOld extends Shell {

    public function perform()
    {
        $this->initialize();
        $this->{array_shift($this->args)}();    // FSH: This way doesn't parse options
    }
}

/**
 * Shell commands to demonstrate current command dispatch.
 */
class OldShell extends AppShellOld {

    public function test_enqueue_args() {
        CakeResque::enqueue('default', 'OldShell', array('main', 'arg0', 'arg1'));
    }

    public function test_enqueue_options() {
        CakeResque::enqueue('default', 'OldShell', array('main', 'arg0', '-v', 'arg1'));
    }

    public function main() {
        $this->out("Main:");
        debug($this->args);
        debug($this->params);
    }
}
// NewShell.php
<?php
App::uses('Shell', 'Console');
App::uses('CakeResque', 'Lib');

/**
 * Proposed change.
 */
class AppShellNew extends Shell {

    public function perform()
    {
        $this->initialize();
        return $this->runCommand($this->args[0], $this->args);
    }
}

/**
 * Shell commands to demonstrate proposed command dispatch change.
 */
class NewShell extends AppShellNew {

    public function test_enqueue_args() {
        CakeResque::enqueue('default', 'NewShell', array('main', 'arg0', 'arg1'));
    }

    public function test_enqueue_options() {
        CakeResque::enqueue('default', 'NewShell', array('main', 'arg0', '-v', 'arg1'));
    }

    public function main() {
        $this->out("Main:");
        debug($this->args);
        debug($this->params);
    }
}

When I run cake old test_enqueue_args and cake old test_enqueue_options, I log this output:

Main:
/app/Console/Command/OldShell.php (line 33)
########## DEBUG ##########
array(
    (int) 0 => 'arg0',
    (int) 1 => 'arg1'
)
###########################
/app/Console/Command/OldShell.php (line 34)
########## DEBUG ##########
array() #<--- No options defined
###########################

Main:
/app/Console/Command/OldShell.php (line 33)
########## DEBUG ##########
array(
    (int) 0 => 'arg0',
    (int) 1 => '-v',  <---- Option passed as argument
    (int) 2 => 'arg1'
)
###########################
/app/Console/Command/OldShell.php (line 34)
########## DEBUG ##########
array()  <----- No options defined
###########################

When I run cake new test_enqueue_args and cake new test_enqueue_options, I log this output:


Welcome to CakePHP v2.4.4 Console
---------------------------------------------------------------
App : app
Path: /Users/frank/proj/server/www/app/
---------------------------------------------------------------
Main:
/app/Console/Command/NewShell.php (line 33)
########## DEBUG ##########
array(
    (int) 0 => 'arg0',  <----- args are correct
    (int) 1 => 'arg1'
)
###########################
/app/Console/Command/NewShell.php (line 34)
########## DEBUG ##########
array(
    'help' => false,  <--- default options are properly defined
    'verbose' => false,
    'quiet' => false
)
###########################

Welcome to CakePHP v2.4.4 Console
---------------------------------------------------------------
App : app
Path: /Users/frank/proj/server/www/app/
---------------------------------------------------------------
Main:
/app/Console/Command/NewShell.php (line 33)
########## DEBUG ##########
array(
    (int) 0 => 'arg0',  <--- args array is as expected
    (int) 1 => 'arg1'
)
###########################
/app/Console/Command/NewShell.php (line 34)
########## DEBUG ##########
array(
    'verbose' => true, <--- 'verbose' option is picked up
    'help' => false,
    'quiet' => false
)
###########################

I believe the "new" version better reflects how Cake developers would expect console commands to be invoked.

P.S. I'm loving this project by the way... thanks!

wa0x6e commented 10 years ago

So from your test case, with runCommand(), the arguments in the called function (main) remain optional ? If so, that change looks interesting ...

wa0x6e commented 10 years ago
public function runCommand($command, $argv);

Shouldn't you also array_shift the $argv, so that the target function receives the same arguments as the old code ?

fhickman commented 10 years ago

The call to array_shift happens in the Cake framework, near the top of Shell::runCommand.

wa0x6e commented 10 years ago

Documentation updated. Thanks.

johannesnagl commented 9 years ago

guys, you rock and have saved my day! :)