Skip to content

Comments

fix order creation on PrestaShop V9#12

Open
matthieu-rolland wants to merge 1 commit intoPrestaShop:mainfrom
matthieu-rolland:fix-order-creation-on-v9
Open

fix order creation on PrestaShop V9#12
matthieu-rolland wants to merge 1 commit intoPrestaShop:mainfrom
matthieu-rolland:fix-order-creation-on-v9

Conversation

@matthieu-rolland
Copy link
Contributor

Questions Answers
Description? Since PrestaShop V9 an employee id must be set in context if no cart id is set, it makes sense in real use cases, when used in CLI for fixtures we need to set an employee id
Type? bug fix
BC breaks? no
Deprecations? no
Fixed ticket? no
Sponsor company PrestaShop SA
How to test? php bin/console prestashop:shop-creator --orders=10

Comment on lines +31 to +35
// Because we could be in CLI mode, there might be no employee in context, so we must set it manually
$context = Context::getContext();
if (!isset($context->employee) || !isset($context->employee->id)) {
$context->employee = new Employee(1);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// Because we could be in CLI mode, there might be no employee in context, so we must set it manually
$context = Context::getContext();
if (!isset($context->employee) || !isset($context->employee->id)) {
$context->employee = new Employee(1);
}

This service is not related to the context, and even less about its initialisation It shouldn't be its responsibility to init the legacy context

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have a useful tool for that, not very well known or used, that allows the Symfony command to initialize the context instead:
https://github.com/PrestaShop/PrestaShop/blob/d68bfe619ea9ef8e9d57e96ce817028c68f3b885/tests/Integration/PrestaShopBundle/Command/LoadLegacyClassesinCommandTest.php#L123-L124

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matthieu-rolland friendly reminder 😉

Copy link
Contributor Author

@matthieu-rolland matthieu-rolland Jul 4, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@matks when using the tool suggested by @jolelievre I saw that it was broken for this use case, so I made a fix on the core

I need this PR to be merged first: PrestaShop/PrestaShop#36358, it's waiting for qa dev

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR is now fixed 😄 PrestaShop/PrestaShop#36358

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.

3 participants