fop_console icon indicating copy to clipboard operation
fop_console copied to clipboard

Fix "employee" option not found

Open leup opened this issue 2 years ago • 6 comments

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 avatar Jul 06 '22 11:07 leup

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 avatar Sep 09 '22 12:09 leup

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."

leup avatar Sep 09 '22 15:09 leup

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?

tom-combet avatar Sep 21 '22 10:09 tom-combet

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 :)

leup avatar Sep 26 '22 08:09 leup

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 ;)

tom-combet avatar Sep 26 '22 13:09 tom-combet

I didn't test, but lgtm.

SebSept avatar Nov 04 '22 10:11 SebSept