Fixed the support of Legacy classes using Console commands #15081

mickaelandrieu · 2019-08-12T14:58:03Z

Questions	Answers
Branch?	develop (but should be fixed asap)
Description?	Creating Symfony Commands in PrestaShop is insanely error-prone! Let's try to add tests, and complete the Console enough to validate 90% of use cases.
Type?	bugfix
Category?	CO
BC breaks?	nope!
Deprecations?	yes
Fixed ticket?	Fixes #14173
How to test?	Tests are done, the build should be green.

<?php

namespace Fop\LatestProductsModule\Command;

use Product;
use PrestaShop\PrestaShop\Core\Console\Command;
use Symfony\Component\Console\Input\InputArgument;
use Symfony\Component\Console\Input\InputInterface;
use Symfony\Component\Console\Output\OutputInterface;
use Symfony\Component\Console\Style\SymfonyStyle;

/**
 * This command display common information about a shop.
 */
final class LatestProductsCommand extends Command
{
    /**
     * {@inheritdoc}
     */
    protected function configure()
    {
        $this
            ->setName('fop:latest-products')
            ->setDescription('Displays Shop status')
            ->setHelp('This command allows you to display status of a shop')
        ;
    }

    /**
     * {@inheritdoc}
     */
    protected function execute(InputInterface $input, OutputInterface $output)
    {
        $products = Product::getNewProducts(1);

        $io = new SymfonyStyle($input, $output);
        $io->title('Legacy Latest Products listing');

        $io->table(
            ['ID', 'Name', 'Quantity', 'Price', 'Activated?'],
            $this->formatProductInformation($products)
        );
    }

    /**
     * @param array $products the list of the products
     *
     * @return array
     */
    private function formatProductInformation(array $products)
    {
        $productsInformation = [];
        /** @var Product $product */
        foreach ($products as $product) {
            $productsInformation[] = [
                $product['id_product'],
                $product['name'],
                $product['quantity'],
                $product['price'],
                $product['active'] ? '✔' : '✘',
            ];
        }

        return $productsInformation;
    }
}

This change is

src/Core/Console/Command.php

src/PrestaShopBundle/EventListener/MultishopCommandListener.php

src/Core/Console/Application.php

src/PrestaShopBundle/Resources/config/services/bundle/event_listener.yml

src/Core/Console/Command.php

mickaelandrieu · 2019-08-13T07:50:21Z

Hello @PrestaShop/prestashop-core-developers,

any chance to get this merged in the next patch version? 🙏

When do you plan to release 1.7.6.1?

matks · 2019-08-13T08:31:26Z

any chance to get this merged in the next patch version? 🙏

I guess this is @eternoendless decision but if we follow our rules, this is neither a regression nor a critical bug 🤔 so it's not something we need to patch (=> would be shipped in a patch version), it's rather a nice improvement for next minor version

When do you plan to release 1.7.6.1?

We have only 1 issue in the 1.7.6 kanban (https://github.com/PrestaShop/PrestaShop/projects/4) so once the Kanban is empty (= every issue is in "done" column), we can process for the QA validation and then the release.
I guess it'll be easier in the end of August when QA team has a little more folks (right now a lot of them are in vacations) 😄

matks · 2019-08-13T08:34:54Z

src/Adapter/Console/ContextLoader.php

+        $this->legacyContext = $legacyContext;
+        $this->shopContext = $shopContext;
+
+        require_once $rootDir . '/../config/config.inc.php';


Could be used for a LFI, please add a validation here

you mean, validate that $rootDir will be a folder? in all cases, this file path will be dynamic.

Maybe better to change this line https://github.com/PrestaShop/PrestaShop/blob/develop/autoload.php#L31 in config.inc.php to load the legacy context because of this https://github.com/PrestaShop/PrestaShop/blob/develop/bin/console#L16

Hello @PierreRambaud, I see what you mean but IMHO the comment on the related file ("Allow call of Legacy classes from classes in /src and /tests") doesn't work as expected.

This is why it could be interesting to change the line and make it works

I'm not sure of the impacts as this file is also used in all contexts, I'd razer not rely on it and focus on what we need when we are in Console context.

How about a new file named autoload.php in console folder instead ?

This file is only loaded in admin-dev/index.php and ./bin/console.
No need to create a new one to do the job its must do

that the thing: I don't think we should use this file at all in Console context: I will remove this call and see what happens :)

matks · 2019-08-13T08:39:30Z

src/Adapter/Console/ContextLoader.php

+    /**
+     * {@inheritdoc}
+     */
+    public function loadConsoleContext(InputInterface $input)


Is InputInterface the right input arg for this command ? It looks like using HttpRequest as an input to me 🤔 maybe we should replace it with

Suggested change

public function loadConsoleContext(InputInterface $input)

public function loadConsoleContext($employeeId, $shopId, $shopGroupdId)

If I need to add a new option I will have to break compatibility, and if I use scalar values I will have to validate data in this function when it's not its role (these options should be validated in the "interact" function of the commands.

matks · 2019-08-13T08:42:59Z

src/Core/Console/Application.php

+        $inputDefinition = $this->getDefinition();
+        $inputDefinition->addOption(new InputOption('employee', '-em', InputOption::VALUE_REQUIRED, 'Specify employee context (id).', null));
+
+        // @see MultiShopCommandListener BC compatible fix to display the options in the Console


It's nice you're trying to solve all issues at once 😄 but I think it's a big challenge
Maybe we should aim to fix the different contexts in the console with different PRs ? I suggest handling the multishop in a separate PR, I'm sure it's a little bomb 💣 that will require a lot of efforts to tackle

it doesn't support multishop feature per see, it only displays the available options for each command :)

it's up to every command to handle or not these options ('id_shop' and 'id_shop_group') and it's probably a little 💣 I do agree

src/Core/Console/Application.php

matks · 2019-08-13T08:43:47Z

src/Core/Console/Application.php

+        $this->setVersion(AppKernel::VERSION);
+
+        $inputDefinition = $this->getDefinition();
+        $inputDefinition->addOption(new InputOption('employee', '-em', InputOption::VALUE_REQUIRED, 'Specify employee context (id).', null));


This is only needed for BO, what if the CLI is used for a FO-related processing ?

I'm thinking that maybe we should aim for BO-oriented CLI console and then a FO-oriented CLI console (separate concerns => separate envs) 🤔

except if I'm wrong you always have an employee set up, even in the Front Office :)

What kind of commands I can create that requires services available in FO?
Also, don't be fooled by the REQUIRED stuff: a CLI option is always optional 🤣

We need to cover more use cases, and I've already created 3 different commands in my module.

matks · 2019-08-13T08:45:02Z

src/Core/Console/Command.php

+ *
+ * @author Mickaël Andrieu <mickael.andrieu@prestashop.com>
+ */
+abstract class Command extends ContainerAwareCommand


Could we improve the naming ? Command is a bit shy to express the fact that this is a "Symfony Command class for PrestaShop allowed to rely on legacy classes"

I formerly named it "PrestaShopCommand", but as this class is part of PrestaShop only; I don't see the value to prefix the name of my class with PrestaShop.

I'm open to suggestions, what do you suggest?

matks · 2019-08-13T08:49:08Z

@mickaelandrieu did a small review, it's great to see there's not so much to be done to fix the issue but I think it'll need extensive QA exploration for complex usecases like multishop

Nice job !

I did something a lot of time ago, dont know if it's related: I tried to make sure every SF Service we create is console-compliant
See #12127
but I surrendered when I saw the amount of work needed (#12127 (comment))

mickaelandrieu · 2019-08-13T14:28:24Z

I did something a lot of time ago, dont know if it's related: I tried to make sure every SF Service we create is console-compliant

What we can do is add an extra validation pass to forbid every class that is not "namespaced" as an argument of a Symfony service: we should only rely on adapters or core classes.

Also, clean all core/prestashop classes to drop every "direct" call to object models: it's not that easy to do but in the long run, this will help you to remove object models (if you intent to ?).

Second extra rule, don't rely directly on the global state like "context" or "request".

This patch sounds complete, but I'm pretty sure such hacky use cases are not covered: I want to write more and more commands to try to break something 🗡 💥

mickaelandrieu · 2019-08-13T19:48:20Z

ping @matks: looking at your PR I've contributed my own command to challenge the Console Context.

I've found some misses that I've fixed for now but I'll do it a better way soon.

As this won't be merged in 1.7.6.1 I'll ~~probably close this pull request soon~~ reduce the scope of this pull request to keep this fix so I can work in a community module instead. This way, so we get a chance to have more feedbacks then the core team will be free to backport the module into the Core and/or include the community module.

Thanks for your review, literally food for my brain 👍

PierreRambaud · 2019-08-19T15:26:08Z

classes/controller/ConsoleController.php

+    /**
+     * {@inheritdoc}
+     */
+    public function checkAccess()


Since we are targeting 7.1+ you can type all your functions and parameters

Sadly, doing this will break the API as the former controller I extend doesn't type functions or arguments

mickaelandrieu · 2019-08-28T13:27:01Z

Tests are here, I can add a lot of tests commands from my own module (that allow people to enjoy this feature starting 1.7.4 or 5).

WDYT?

mickaelandrieu · 2019-08-28T13:28:42Z

classes/controller/ConsoleController.php

+     */
+    protected function buildContainer()
+    {
+        return SymfonyContainer::getInstance();


this function is not used atm, but as the parent function is supposed to return a Container...

mickaelandrieu · 2019-09-17T19:27:03Z

Tests have been added 👍 , it's not in WIP anymore!

PierreRambaud · 2019-09-17T19:29:35Z

bin/console

@@ -12,8 +12,8 @@ use Symfony\Component\Debug\Debug;

 set_time_limit(0);

-require __DIR__.'/../vendor/autoload.php';
-require_once __DIR__.'/../autoload.php';
+require_once __DIR__.'/../vendor/autoload.php';


Suggested change

require_once __DIR__.'/../vendor/autoload.php';

Already loaded in config.inc.php

I know but without it, I have the feeling it won't works.

Let me remove it and let's see if this file is required or not :)

PierreRambaud · 2019-09-17T19:30:11Z

src/PrestaShopBundle/Resources/config/services/adapter/cms_page_category.yml

@@ -6,7 +6,7 @@ services:

    prestashop.adapter.cms_page_category.command_handler.abstract_cms_page_category_handler:
        class: PrestaShop\PrestaShop\Adapter\CMS\PageCategory\CommandHandler\AbstractCmsPageCategoryHandler
-        abstract: true
+        abstract:  true


Suggested change

abstract: true

abstract: true

this file is not supposed to be there , i'll remove it

mickaelandrieu · 2019-09-18T13:40:05Z

Hi @PierreRambaud,

comments addressed.

Thanks for review

And tests pass 🎉

mickaelandrieu · 2020-05-12T14:57:40Z

:/ I had to restore this branch as #14173 is not fixed, but I wont have time to update/maintain it.

mickaelandrieu requested a review from a team as a code owner August 12, 2019 14:58

prestonBot added develop Branch Improvement Type: Improvement labels Aug 12, 2019