Context refactorisation for controller #34390

M0rgan01 · 2023-10-26T07:49:47Z

Questions	Answers
Branch?	develop
Description?	-
Type?	refacto
Category?	BO
BC breaks?	no
Deprecations?	no
How to test?	-
UI Tests	Legacy: https://github.com/M0rgan01/ga.tests.ui.pr/actions/runs/6827242059 / Symfony: https://github.com/M0rgan01/ga.tests.ui.pr/actions/runs/6827243906
Fixed issue or discussion?	Fixes #33191
Related PRs	-
Sponsor company	-

src/Adapter/Context/ControllerContextBuilder.php

src/Adapter/Context/Controller.php

matks · 2023-11-07T16:07:45Z

src/Core/Context/LegacyController.php

+use Module;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+use Traversable;
+


You can use php strict typing 😄

jolelievre · 2023-10-26T09:46:37Z

src/PrestaShopBundle/Resources/config/services.yml

@@ -1,5 +1,16 @@
 parameters:
  multishop.settings.share_orders: !php/const Shop::SHARE_ORDER
+  controllers.locked.to.all.shop.context:


Suggested change

controllers.locked.to.all.shop.context:

prestashop.all_shops_controllers:

jolelievre · 2023-11-08T16:52:53Z

classes/Context.php

@@ -28,6 +28,7 @@
 use PrestaShop\PrestaShop\Adapter\ContainerFinder;
 use PrestaShop\PrestaShop\Adapter\Module\Repository\ModuleRepository;
 use PrestaShop\PrestaShop\Adapter\SymfonyContainer;
+use PrestaShop\PrestaShop\Core\Context\LegacyController as ControllerContext;


I'm not sure the alias is needed, we could keep LegacyController which describes more the object IMHO

phpstan-baseline.neon

jolelievre · 2023-11-08T16:57:25Z

src/Adapter/ContextStateManager.php

+     *
+     * @return $this
+     */
+    public function setController(?LegacyController $legacyController): self


Nothing to add about the method itself, however this MUST be tested At least in unit tests of the ContextStateManager here:

PrestaShop/tests/Unit/Adapter/ContextStateManagerTest.php

Line 40 in d200141

class ContextStateManagerTest extends ContextStateTestCase

You can inspire yourself from what I did in my PR for language context https://github.com/PrestaShop/PrestaShop/pull/34500/files#diff-b20eac977acbd6001bb22e5a07171efb9b80020d3abefa5c66cf34f0ed6e3a30R179

I didn't have time to look into the integration tests yet so I don't remember how hard they are to adapt But this component is too sensitive to remain untested

jolelievre · 2023-11-08T16:59:32Z

src/Core/Context/LegacyController.php

+class LegacyController
+{
+    // Dependency container
+    protected ContainerInterface $container;


With the Symfony convention, that we try to follow, the protected method/fields are supposed to be declared after the public ones, so it'd be nice to move this one a bit lower

jolelievre · 2023-11-08T17:26:59Z

src/PrestaShopBundle/EventListener/Context/Admin/CountryContextListener.php

@@ -42,7 +45,7 @@ public function __construct(

    public function onKernelRequest(RequestEvent $event): void
    {
-        if (!$event->isMainRequest()) {
+        if (!$event->isMainRequest() && $this->isApiRequest($event->getRequest())) {


Suggested change

if (!$event->isMainRequest() && $this->isApiRequest($event->getRequest())) {

if (!$event->isMainRequest() || $this->isApiRequest($event->getRequest())) {

Isn't it an OR condition here? 🤔 Same comment for all the other listeners since it's the same code

jolelievre · 2023-11-08T17:28:01Z

src/PrestaShopBundle/Resources/config/services/core/context.yml

+    bind:
+      $controllersLockedToAllShopContext: '%prestashop.controllers_all_shop_context%'
+      $container: "@service_container"
+      $coreDir: !php/const _PS_CORE_DIR_


Suggested change

$coreDir: !php/const _PS_CORE_DIR_

$projectDir: '%kernel.project_dir%'

jolelievre · 2023-11-08T17:28:53Z

src/PrestaShopBundle/EventListener/ApiPlatformTrait.php

+
+trait ApiPlatformTrait
+{
+    protected function isApiRequest(Request $request): bool


Suggested change

protected function isApiRequest(Request $request): bool

protected function isApiPlatformRequest(Request $request): bool

I don't think it's a good thing to name a method by the name of a framework. If we change tomorrow we will have to change all the calls, in addition to changing the body of the method

Same thing for file name

jolelievre · 2023-11-08T17:33:46Z

tests/Unit/Core/Context/LegacyControllerContextBuilderTest.php

+{
+    use MockConfigurationTrait;
+
+    public function testLegacyBuild(): void


Another argument about my comment above, about making the controller name settable via a setter method It would make it easier to test this via a data provider You can loop through different values of the controller name that will impact the deduced values like className, shopLinkType, multishop_context, overrideFolder and so on Thus you will be able to test and validate much more things from the builder internal behaviour

Note that even if we don't go for the setter strategy for the controller name, the tests can still be dynamized and enhanced since you can also mock the request with different controller name values

jolelievre · 2023-11-08T17:36:59Z

tests/Unit/bootstrap.php

@@ -33,6 +33,7 @@
    define('PHPUNIT_COMPOSER_INSTALL', dirname(__DIR__, 2) . '/vendor/autoload.php');
 }

+define('_COOKIE_KEY_', Tools::passwdGen(56));


Hmm, why is this suddenly needed? 🤔

We call Tools::getAdminToken in the builder, it needs the constant

jolelievre · 2023-11-10T11:40:02Z

src/Adapter/ContextStateManager.php

+     *
+     * @return $this
+     */
+    public function setController(?LegacyControllerContext $legacyController): self


This shouldn't be limited to the legacy controller only, but all the potential classes allowed in Context::controller Including the real legacy controllers (both admin and front)

Suggested change

public function setController(?LegacyControllerContext $legacyController): self

public function setController(?LegacyControllerContext|Controller $legacyController): self

Using the legacy Controller parent class will allow using all the different types (front/admin/modulefront/moduleadmin/...)

jolelievre · 2023-11-10T11:46:34Z

src/Core/Context/LegacyControllerContext.php

+    public array $_conf;
+
+    public array $_error;


I looked a bit more into detail the usages of these two fields, seems like they are mostly used internally as well by admin controllers like they would use $this->_conf[45] to add a message in smarty or in a json response

But as we removed some other internal properties I'm not sure these ones are actually relevant, so we can maybe remove them as well (at least for now until someone complains about it)

The strategy 'we keep as much of the public field as possible because we don't know which ones are used' from the start is no longer true x) But it is better in the sense that it cleans the fields that should have been protected

Yep I changed my mind a little bit But I didn't realize there were so many public properties that are actually purely related to internal behaviour of the admin controller and that should have been protected probably

src/Core/Context/LegacyControllerContext.php

jolelievre · 2023-11-10T11:50:46Z

src/Core/Context/LegacyControllerContextBuilder.php

+        }
+    }
+
+    private function getConf(): array


As commented above, maybe this could be removed 🤔

jolelievre · 2023-11-10T11:51:49Z

src/PrestaShopBundle/EventListener/ApiPlatformTrait.php

+ * Utility Trait, enabling the detection of whether a request is an API Platform request.
+ * This allows us to condition listeners and thus control the creation of contexts.
+ */
+trait ApiPlatformTrait


I though you didn't like the ApiPlatform part because it's too related to the framework
Didn't we think about ExternalApiTrait::isExternalApiRequest ?

Yes I prefer ExternalApi.

jolelievre · 2023-11-10T11:55:05Z

tests/Unit/Adapter/ContextStateManagerTest.php

+        $contextStateManager = new ContextStateManager($this->legacyContext);
+        $this->assertNull($contextStateManager->getContextFieldsStack());
+
+        $contextStateManager->setController($this->createLegacyControllerContextMock('AdminProductsController'));


You should use a third different value as this one is the same as the initial one

Suggested change

$contextStateManager->setController($this->createLegacyControllerContextMock('AdminProductsController'));

$contextStateManager->setController($this->createLegacyControllerContextMock('AdminOrdersController'));

jolelievre · 2023-11-10T11:55:39Z

tests/Unit/Adapter/ContextStateManagerTest.php

+        $this->assertNull($contextStateManager->getContextFieldsStack());
+
+        $contextStateManager->setController($this->createLegacyControllerContextMock('AdminProductsController'));
+        $this->assertEquals('AdminProductsController', $context->controller->controller_name);


Suggested change

$this->assertEquals('AdminProductsController', $context->controller->controller_name);

$this->assertEquals('AdminOrdersController', $context->controller->controller_name);

jolelievre · 2023-11-10T12:00:17Z

tests/Unit/Core/Context/LegacyControllerContextBuilderTest.php

+{
+    use MockConfigurationTrait;
+
+    public function testBuild(): void


Are you familiar with the @dataProvider in PHPUnit tests? This is more adapted when you mean to test different values with the same test logic You can dynamize the parameters of the test method and pass it multiple settings (from the initialization parameters to the expected values)

It's easier to add multiple use cases then

You can find an example here

PrestaShop/tests/Unit/Core/Context/CurrencyContextBuilderTest.php

Line 57 in f4cea14

public function testBuild(int $languageId, string $expectedName, string $expectedSymbol, string $expectedPattern): void

This would allow you testing all the special use cases (especially for className and multishop context) efficiently and it reduces the size of code needed while keeping the readability easier (at least in my opinion, but maybe it's because I'm used to it)

jolelievre

Thanks @M0rgan01

just a few suggestions but none of them are blocking. Nice work!

jolelievre · 2023-11-12T17:19:59Z

src/Core/Context/LegacyControllerContext.php

+ */
+class LegacyControllerContext
+{
+    public ?string $className;


Not necessarily to be done in this PR, but I think most of those fields are only read and never written, or at least they should never be written Since we can now use the readonly property we should think about which fields could be forced as read only

This would enforce even more the contract that the controller is supposed to follow and reduce the potential bugs due to external modules and code modifying some internal fields

But again not blocking for this PR

jolelievre · 2023-11-13T09:21:48Z

tests/Unit/Core/Context/LegacyControllerContextBuilderTest.php

+        yield 'AdminAccessController' => [
+            'AdminAccessController',
+            'Profile',
+            7,


It would be easier to understand if you used the combination of the const here for future developers, but it's a detail not worth blocking the PR

Suggested change

7,

ShopConstraint::ALL_SHOPS | ShopConstraint::SHOP | ShopConstraint::SHOP_GROUP,

Constant or comment, but yeah 7 is not explicit 😄

matks

Nothing to add to @jolelievre review

matks · 2023-11-13T09:33:01Z

tests/Unit/Core/Context/LegacyControllerContextBuilderTest.php

+        yield 'AdminAccessController' => [
+            'AdminAccessController',
+            'Profile',
+            7,


Constant or comment, but yeah 7 is not explicit 😄

prestonBot added develop Branch Refactoring Type: Refactoring labels Oct 26, 2023

M0rgan01 force-pushed the improvement/33191 branch 4 times, most recently from b6248a9 to 495d193 Compare October 27, 2023 14:45

M0rgan01 force-pushed the improvement/33191 branch 6 times, most recently from eefdbdf to debad42 Compare November 6, 2023 13:55

M0rgan01 added this to the 9.0.0 milestone Nov 6, 2023

M0rgan01 marked this pull request as ready for review November 6, 2023 15:32

M0rgan01 requested a review from a team as a code owner November 6, 2023 15:32

M0rgan01 commented Nov 6, 2023

View reviewed changes

M0rgan01 force-pushed the improvement/33191 branch 4 times, most recently from e2b84ff to fa5406c Compare November 7, 2023 14:37

matks reviewed Nov 7, 2023

View reviewed changes

M0rgan01 force-pushed the improvement/33191 branch 9 times, most recently from ab861ef to df55257 Compare November 8, 2023 15:02

M0rgan01 force-pushed the improvement/33191 branch from df55257 to 2f41140 Compare November 8, 2023 16:31

jolelievre reviewed Nov 8, 2023

View reviewed changes

M0rgan01 force-pushed the improvement/33191 branch 8 times, most recently from 15e339b to 5872232 Compare November 10, 2023 09:52

jolelievre reviewed Nov 10, 2023

View reviewed changes

M0rgan01 force-pushed the improvement/33191 branch 4 times, most recently from bc4f2ee to 07f7289 Compare November 10, 2023 15:36

Morgan Pichat added 2 commits November 10, 2023 16:52

Create controller context

b575c26

Create API trait

b41e76d

M0rgan01 force-pushed the improvement/33191 branch from 07f7289 to b41e76d Compare November 10, 2023 15:52

jolelievre approved these changes Nov 13, 2023

View reviewed changes

matks approved these changes Nov 13, 2023

View reviewed changes

ps-jarvis added the Waiting for QA Status: action required, waiting for test feedback label Nov 13, 2023

matthieu-rolland approved these changes Nov 13, 2023

View reviewed changes

M0rgan01 merged commit 2cb5615 into PrestaShop:develop Nov 13, 2023
18 checks passed

M0rgan01 removed the Waiting for QA Status: action required, waiting for test feedback label Nov 13, 2023

	controllers.locked.to.all.shop.context:
	prestashop.all_shops_controllers:

	if (!$event->isMainRequest() && $this->isApiRequest($event->getRequest())) {
	if (!$event->isMainRequest() \|\| $this->isApiRequest($event->getRequest())) {

	$coreDir: !php/const _PS_CORE_DIR_
	$projectDir: '%kernel.project_dir%'

	protected function isApiRequest(Request $request): bool
	protected function isApiPlatformRequest(Request $request): bool

	public function setController(?LegacyControllerContext $legacyController): self
	public function setController(?LegacyControllerContext\|Controller $legacyController): self

	$contextStateManager->setController($this->createLegacyControllerContextMock('AdminProductsController'));
	$contextStateManager->setController($this->createLegacyControllerContextMock('AdminOrdersController'));

	$this->assertEquals('AdminProductsController', $context->controller->controller_name);
	$this->assertEquals('AdminOrdersController', $context->controller->controller_name);

	7,
	ShopConstraint::ALL_SHOPS \| ShopConstraint::SHOP \| ShopConstraint::SHOP_GROUP,

Context refactorisation for controller #34390

Context refactorisation for controller #34390

Conversation

M0rgan01 commented Oct 26, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

M0rgan01 commented Oct 26, 2023 •

edited