Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix "employee" option not found #256

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

leup
Copy link

@leup leup commented Jul 6, 2022

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
Copy link
Author

leup commented Sep 9, 2022

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 leup closed this Sep 9, 2022
The configure() method is often overridden by Commands without calling the parent method.
Moving to constructor is safer.
@leup leup reopened this Sep 9, 2022
@leup
Copy link
Author

leup commented Sep 9, 2022

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
Copy link
Contributor

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
Copy link
Author

leup commented Sep 26, 2022

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
Copy link
Contributor

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
Copy link
Contributor

SebSept commented Nov 4, 2022

I didn't test, but lgtm.

SebSept
SebSept previously approved these changes Nov 4, 2022
@SebSept SebSept dismissed their stale review November 4, 2022 10:46

I should considere all consequences

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Unknown option "employee"
3 participants