codestudiohq / laravel-totem

Manage Your Laravel Schedule From A Web Dashboard
MIT License
1.78k stars 224 forks source link

[BUG] Removing the console instruction. #63

Closed JeanCarloLeal closed 6 years ago

JeanCarloLeal commented 6 years ago

@qschmick and @roshangautam In the Overriding multiples arguments correction, the console statement was removed, creating a bug for my application ... please go back with the console check.

We have to be careful about removing validations.

https://github.com/codestudiohq/laravel-totem/blob/a437c9ef1b10483f78af1527c7ff567c4c3f205e/src/Task.php#L78

CORRECT IS:

public function compileParameters($console = false)
    {
        if ($this->parameters) {
            $regex = '/(?=\S)[^\'"\s]*(?:\'[^\']*\'[^\'"\s]*|"[^"]*"[^\'"\s]*)*/';
            preg_match_all($regex, $this->parameters, $matches, PREG_SET_ORDER, 0);

            $argument_index = 0;
            $parameters = collect($matches)->mapWithKeys(function ($parameter) use ($console, &$argument_index) {
                $param = explode('=', $parameter[0]);

                return count($param) > 1 ?
                    ($console ? ((starts_with($param[0], '--') ? [$param[0] => $param[1]] : [$argument_index++ => $param[1]])) : [$param[0] => $param[1]])
                    : (starts_with($param[0], '--') && !$console ? [$param[0] => true] : $param);

            })->toArray();

            return $parameters;
        }

        return [];
    }
JeanCarloLeal commented 6 years ago

I do not want to be boring, and I understand the chores you have ... but can you take a look at this? @qschmick @roshangautam

qschmick commented 6 years ago

@JeanCarloLeal I'm looking into this

qschmick commented 6 years ago

@JeanCarloLeal Just tagged 2.2.1, this release includes new tests around a very mixed set of arguments with focus on console output as non-console was already tested well. Thanks for the follow up and apologies for the delay.