-
Notifications
You must be signed in to change notification settings - Fork 37
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
base: dev
Are you sure you want to change the base?
Conversation
My fix is not good. |
The configure() method is often overridden by Commands without calling the parent method. Moving to constructor is safer.
Moved the addOption to constructor instead of configure(). protected function configure(): void
{
$this->setName('fop:module:generate')
->setDescription('Scaffold new PrestaShop module')
;
} Therefore, doing the Before
|
I'm really sorry for the late help. I'll try to be more active on the project from now on. Also, the Are you still ready to continue the pull request? |
Hello ! Thanks for your reply :)
I manage to avoid modifying all the commands by adding the option into the constructor. Not great but working. |
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. |
I didn't test, but lgtm. |
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