Prepares Command/Query API for migrating "Create order" page #13554
Conversation
| // Some services uses static PrestaShop/Adapter/SymfonyContainer::getInstance() | ||
| // and it depends on global $kernel | ||
| global $kernel; | ||
| $kernel = self::$kernel; |
There was a problem hiding this comment.
@matks 😨 This global state and Context are making tests so much harder to maintain. :/
There was a problem hiding this comment.
Yes THAT is a broken window wide open.
Can you tell me what step required this ?
There was a problem hiding this comment.
As far as I remember it starts at PaymentModule::validateOrder(), then it goes to Tools::displayPrice() which then internally calls ToolsCore::getContextLocale() which then calls SymfonyContainer::getInstance() which uses global $kernel. 😓
There was a problem hiding this comment.
Yes you're right, it's set like this too here: https://github.com/PrestaShop/PrestaShop/blob/develop/tests/Integration/Behaviour/Features/Context/OrderFeatureContext.php#L56
Can we make this setup a function that can be called only once, to avoid duplicate kernel booting ?
There was a problem hiding this comment.
Hmm, but this is what I did, isn't it? 🤔
There was a problem hiding this comment.
Sorry, I was mistaken because we have 2 OrderFeatureContext, one in Domain and one outside. I looked at the wrong one
There was a problem hiding this comment.
Erratum, can we check the global $kernel does not yet exists, and if it does, throw an Exception ?
Something like if (global $kernel !== null) { throw new \Exception('Cannot init SF Kernel twice'); }
A small firewall to avoid crazy bugs later 😄
There was a problem hiding this comment.
Well, I've tried to do it, but ended up modifying other tests as well, so I'd prefer to do it in separate PR when this one gets merged.
sarjon
changed the title
prestonBot
added
the
Waiting for wording
|
Hello, the wording is alright but it is hard for me to picture the whole scheme: are they like random strings before migrating the whole page in another pull request? |
Hi, not sure if I understand your question, but basically it's a backend without any visible changes to UI yet. So yes, UI will be migrated in another PR. :) |
|
@matks could you review? :) |
tests/Integration/Behaviour/Features/Context/EmployeeFeatureContext.php
Outdated
Show resolved
Hide resolved
tests/Integration/Behaviour/Features/Context/OrderFeatureContext.php
Outdated
Show resolved
Hide resolved
tests/Integration/Behaviour/Features/Context/OrderFeatureContext.php
Outdated
Show resolved
Hide resolved
LouiseBonnard
removed
the
Waiting for wording
|
@matks would you mind to take a look at behat steps and scenario? I've updated to use references, I think it looks better now. Not sure why tests are failing, any ideas? :/ |
Co-Authored-By: Mathieu Ferment <mathieu.ferment@prestashop.com>
Co-Authored-By: Mathieu Ferment <mathieu.ferment@prestashop.com>
Co-Authored-By: Mathieu Ferment <mathieu.ferment@prestashop.com>
|
@matks finally, it's green! |
Today is a nice day 😄 |
|
Thank you @sarjon |
This change is