friends-of-presta / fop_console

Prestashop Module providing a set of shell/terminal commands for developers (PrestaShop 1.7.5+)
Academic Free License v3.0
84 stars 35 forks source link

Fix "employee" option not found #256

Open leup opened 2 years ago

leup commented 2 years ago

Fixes #255

I left the shortcut as defined, but I was not able to make it work.

-em=1 --em=1 -em1 -em 1 --em 1

Whatever I tried, I had "option not found" or "The file "/var/www/html/app/config/config_***.yml" does not exist because it falls under the -e option

leup commented 1 year ago

My fix is not good. It fixed my use case but it does not work with existing commands because they would have to call "parent::configure()" in every command class.

leup commented 1 year ago

Moved the addOption to constructor instead of configure(). The configure() method is implemented by Commands to set name and description, without calling the parent method, like

    protected function configure(): void
    {
        $this->setName('fop:module:generate')
            ->setDescription('Scaffold new PrestaShop module')
        ;
    }

Therefore, doing the addOption within the configure method from the parent Command class means that the addOption is never done. It worked for me because I did my own command and called the parent method like parent::configure().

Before

php bin/console fop:employee:list --employee=1

ERROR [console] Error thrown while running command "fop:employee:list --employee=1". Message: "The "--employee" option does not exist."

tom-combet commented 1 year ago

I'm really sorry for the late help. I'll try to be more active on the project from now on. You're right, we have to call parent configure() method in every command we create. By the way, I prefer to call parent method first personally, before the child statements.

Also, the id_shop and id_shop_group options seems to be planned but not fully implemented too... We could add them too!

Are you still ready to continue the pull request?

leup commented 1 year ago

Hello ! Thanks for your reply :)

You're right, we have to call parent configure() method in every command we create. By the way, I prefer to call parent method first personally, before the child statements.

I manage to avoid modifying all the commands by adding the option into the constructor. Not great but working. I am ready to continue on the pull request, no problem. I just have to be sure what to do :)

tom-combet commented 1 year ago

Hello ! Thanks for your reply :)

You're right, we have to call parent configure() method in every command we create. By the way, I prefer to call parent method first personally, before the child statements.

I manage to avoid modifying all the commands by adding the option into the constructor. Not great but working. I am ready to continue on the pull request, no problem. I just have to be sure what to do :)

I prefer to configure the command in the dedicated method if possible. I'm not against modifying all the commands, but rather the opposite: we should have called the parent method since the beginning of the project imo. So feel free to go ahead and add the parent call in all the commands ;)

SebSept commented 1 year ago

I didn't test, but lgtm.