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

Implementation of CQRS in SqlManager page #9460

Merged
merged 45 commits into from Sep 6, 2018

Conversation

@sarjon
Member

sarjon commented Aug 17, 2018

Questions Answers
Branch? develop
Description? This PR follows POC introduced by @eternoendless in #9370
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? n/a
How to test? All actions in Sql Manager page should work the same.

This change is Reviewable

@eternoendless

Nice job! Consider changing "RequestSql" to "SqlRequest" everywhere.

Reviewed 17 of 17 files at r1.
Reviewable status: all files reviewed, 11 unresolved discussions (waiting on @eternoendless and @sarjon)


src/Adapter/SqlManager/CommandHandler/AddRequestSqlHandler.php, line 30 at r1 (raw file):


use Exception;
use PrestaShop\PrestaShop\Adapter\Entity\RequestSql;

This class hasn't been defined, why not use Request(Core)?


src/Adapter/SqlManager/QueryHandler/GetRequestSqlForEditingHandler.php, line 29 at r1 (raw file):

namespace PrestaShop\PrestaShop\Adapter\SqlManager\QueryHandler;

use PrestaShop\PrestaShop\Adapter\Entity\RequestSql;

Same comment about the non-existing entity


src/Core/Domain/SqlManagement/EditableRequestSql.php, line 97 at r1 (raw file):

     * @return EditableRequestSql
     */
    private function setRequestSqlId($requestSqlId)

You could sign the parameter here


src/Core/Domain/SqlManagement/Command/AddRequestSqlCommand.php, line 82 at r1 (raw file):

    private function setName($name)
    {
        $this->name = $name;

If an empty name is not valid, this would be a good place to verify that.

In addition, you could also verify that the type is string.


src/Core/Domain/SqlManagement/Command/AddRequestSqlCommand.php, line 98 at r1 (raw file):

Previously, PierreRambaud (GoT) wrote…

Is there a way to use symfony asset instead? While prevent many duplicates. And we will able to check for Integer, string, or anything.

+1 for type check.


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/RequestSqlController.php, line 29 at r1 (raw file):

namespace PrestaShopBundle\Controller\Admin\Configure\AdvancedParameters;

use League\Tactician\CommandBus;

I think this can be deleted


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/RequestSqlController.php, line 196 at r1 (raw file):

                );

                $this->getCommandBus()->handle($addRequestSqlCommand);

Don't you think this should be handled in the form data provider? Of course, it would need to have the command bus injected.


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/RequestSqlController.php, line 202 at r1 (raw file):

                return $this->redirectToRoute('admin_request_sql');
            } catch (RequestSqlException $e) {
                //@todo: handle properly

Please add a throw $e as long as it's unhandled so that we don't forget about it.


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/RequestSqlController.php, line 236 at r1 (raw file):


            //@todo: implement $editableRequestSql editing handler
        } catch (RequestSqlException $e){

You'd have to add a case for RequestSqlNotFoundException


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/RequestSqlController.php, line 473 at r1 (raw file):

     * @return CommandBus
     */
    protected function getCommandBus()

I think it would be best to add this helper to FrameworkBundleAdminController instead.


src/PrestaShopBundle/Resources/config/services/adapter/sql_manager.yml, line 24 at r1 (raw file):

        class: 'PrestaShop\PrestaShop\Adapter\SqlManager\CommandHandler\AddRequestSqlHandler'
        tags:
            - { name: 'tactician.handler', command: 'PrestaShop\PrestaShop\Core\Domain\SqlManagement\Command\AddRequestSqlCommand' }

Have you tried using typehint-based mapping? I'm not sure if it will work without enabling autowiring everywhere though :(

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 20, 2018

Member

src/PrestaShopBundle/Resources/config/services/adapter/sql_manager.yml, line 24 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Have you tried using typehint-based mapping? I'm not sure if it will work without enabling autowiring everywhere though :(

Unfortunately typehint-based mapping is available from tactician bundle version 1.1, but since 1.0 they require PHP 7+ to run, so we have to use 0.4 which does not support it. :/

Member

sarjon commented Aug 20, 2018


src/PrestaShopBundle/Resources/config/services/adapter/sql_manager.yml, line 24 at r1 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Have you tried using typehint-based mapping? I'm not sure if it will work without enabling autowiring everywhere though :(

Unfortunately typehint-based mapping is available from tactician bundle version 1.1, but since 1.0 they require PHP 7+ to run, so we have to use 0.4 which does not support it. :/

*/
private function setSql($sql)
{
if (!is_string($sql) || empty($sql)) {

This comment has been minimized.

@sarjon

sarjon Aug 20, 2018

Member

@eternoendless there is a service SqlQueryValidator that checks if SQL query is not malformed, how would you suggest dealing with that validation when creating command?

@sarjon

sarjon Aug 20, 2018

Member

@eternoendless there is a service SqlQueryValidator that checks if SQL query is not malformed, how would you suggest dealing with that validation when creating command?

@eternoendless

I'm not completely sure about the validation which is half handled by RequestSqlFromDataValidator and the other half by RequestSqlFromDataProvider... maybe there's a way to unify it all?

Reviewed 13 of 19 files at r2, 3 of 7 files at r3, 9 of 9 files at r5.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @sarjon, @PierreRambaud, and @eternoendless)


src/Adapter/SqlManager/CommandHandler/AddSqlRequestHandler.php, line 72 at r5 (raw file):

     * @throws SqlRequestException
     */
    private function buildRequestSql()

Maybe pass the command as a parameter in order to keep the class stateless?


src/Adapter/SqlManager/CommandHandler/EditSqlRequestHandler.php, line 61 at r5 (raw file):

     * @throws SqlRequestNotFoundException
     */
    private function editSqlRequest()

I don't think this method is necessary. This could be done in the handle() method and the class would be stateless.


src/Core/Domain/SqlManagement/Command/EditSqlRequestCommand.php, line 33 at r5 (raw file):


/**
 * Class EditSqlRequestCommand defines command for SqlRequest editing

Same as the other command "This command modifies an existing SqlRequest object, replacing its data by the provided one"


src/Core/Domain/SqlManagement/Command/EditSqlRequestCommand.php, line 60 at r5 (raw file):

        SqlRequestId $sqlRequestId,
        $name,
        $sql

What if I only want to change the name or the SQL?

Maybe the command should have public setters for both properties instead of having them in the constructor.

This would allow partial updates. If some properties are null, then it isn't updated.


src/Core/Domain/SqlManagement/Exception/SqlRequestConstraintException.php, line 33 at r5 (raw file):

    const INVALID_NAME_ERROR = 10;
    const INVALID_SQL_QUERY_ERROR = 20;
    const MALFORMED_SQL_QUERY_ERROR = 30;

Since they are exception codes, I don't think the _ERROR suffix is needed.


src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/RequestSqlFormDataProvider.php, line 44 at r5 (raw file):

 * Class RequestSqlFormDataProvider is responsible for getting/saving RequestSql form data
 */
class RequestSqlFormDataProvider

This class does not implement FormDataProviderInterface


src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/RequestSqlFormDataProvider.php, line 120 at r5 (raw file):

        } catch (SqlRequestException $e) {
            $errors[] = $this->getHumanReadableGenericErrorMessage();
        }

I'm not a fan of this suite of catch.

It's best to avoid switch and switch-like statements like this one. The rule of thumb is: if I the answer to the question "How do I add one more case" is "You need to edit the file and add one more case", then You Are Doing It Wrong.

You can rely on dictionaries instead:

public function saveData(array $requestSqlData)
{

    // ...

    try {
        // ...
    } catch (Exception $e) {
       $errors = $this->handleException($e);
    }

    // ...

}

private function handleException(Exception $e)
{
    $exceptionType = get_class($e);

    if ($exceptionType === SqlRequestConstraintException::class) {
        return $this->getConstraintError($e);
    }

    return $this->getErrorByExceptionType($e);
}

private function getConstraintError(SqlRequestConstraintException $e)
{
    $code = $e->getCode();

    $invalidFieldDictionary = [
        SqlRequestConstraintException::INVALID_NAME_ERROR => 'name',
        SqlRequestConstraintException::INVALID_SQL_QUERY_ERROR => 'sql',
        SqlRequestConstraintException::MALFORMED_SQL_QUERY_ERROR => 'sql';
    ];

    if (isset($invalidFieldDictionary[$code])) {
        return [
            'key' => 'The %s field is invalid.',
            'parameters' => [
                $invalidFieldDictionary[$code]
            ],
            'domain' => 'Admin.Notifications.Error',
        ];
    }

    return [
        'key' => 'Invalid data supplied.',
        'parameters' => [],
        'domain' => 'Admin.Notifications.Error',
    ];
}

private function getErrorByExceptionType(Exception $e)
{
    $dict = [
        SqlRequestNotFoundException::class => [
            'key' => 'The object cannot be loaded (or found)',
            'parameters' => [],
            'domain' => 'Admin.Notifications.Error',
        ],
        CannotAddSqlRequestException::class => [
            'key' => 'An error occurred while creating an object.',
            'parameters' => [],
            'domain' => 'Admin.Notifications.Error',
        ],
       // ...
   ];

   $exceptionType = get_class($e);
    
    if (isset($dict[$exceptionType])) {
        return $dict[$exceptionType];
    }

    return [
        'key' => 'Unexpected error occurred.',
        'parameters' => [],
        'domain' => 'Admin.Notifications.Error',
    ];
}

src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/RequestSqlFormHandler.php, line 97 at r5 (raw file):

     * @param array $data
     *
     * @return array

@return array[] array of errors


src/PrestaShopBundle/Resources/config/services/adapter/sql_manager.yml, line 24 at r1 (raw file):

Previously, sarjon (Šarūnas Jonušas) wrote…

Unfortunately typehint-based mapping is available from tactician bundle version 1.1, but since 1.0 they require PHP 7+ to run, so we have to use 0.4 which does not support it. :/

Too bad, let's hope we can upgrade to php 7 soon


src/Core/Domain/SqlManagement/Command/AddSqlRequestCommand.php, line 32 at r5 (raw file):


/**
 * Class AddSqlRequestCommand holds commands data for SqlRequest creation

Commands are our "public" API. As such, I would describe like this: "This command creates a new SqlRequest object"


src/Core/Domain/SqlManagement/Command/AddSqlRequestCommand.php, line 89 at r5 (raw file):

        if (!is_string($name) || empty($name)) {
            throw new SqlRequestConstraintException(
                sprintf('Invalid RequestSql name "%s"', var_export($name, true)),

No need for double quotes around %s when using var_export


src/Core/Domain/SqlManagement/Command/AddSqlRequestCommand.php, line 110 at r5 (raw file):

Previously, sarjon (Šarūnas Jonušas) wrote…

@eternoendless there is a service SqlQueryValidator that checks if SQL query is not malformed, how would you suggest dealing with that validation when creating command?

If from a domain perspective it doesn't make sense to store invalid SQL, then it should be checked here. Just make sure that the check is not performed twice in the controller side.


src/Core/Domain/SqlManagement/Command/AddRequestSqlCommand.php, line 98 at r1 (raw file):

Previously, PierreRambaud (GoT) wrote…

Yes, I mean Symfony Validation, using https://symfony.com/doc/current/validation.html could be great, wdyt?

Maybe it's overkill for this?

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 21, 2018

Member

src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/RequestSqlFormDataProvider.php, line 44 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

This class does not implement FormDataProviderInterface

as far as i understand FormDataProviderInterface is meant to be used in "options" forms. The problem with it is getData() method, which does not allow you to specific id of entity which data you want to get, thus imo this interface could better be under Core/Form/Option/FormDataProviderInterface namespace?

i guess @mickaelandrieu can provide better explanation for this?

Member

sarjon commented Aug 21, 2018


src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/RequestSqlFormDataProvider.php, line 44 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

This class does not implement FormDataProviderInterface

as far as i understand FormDataProviderInterface is meant to be used in "options" forms. The problem with it is getData() method, which does not allow you to specific id of entity which data you want to get, thus imo this interface could better be under Core/Form/Option/FormDataProviderInterface namespace?

i guess @mickaelandrieu can provide better explanation for this?

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 21, 2018

Member

hmm, i think all validation (name and sql + validate if sql query is not malformed) is performed in RequestSqlFormDataValidator, and RequestSqlFormDataProvider only dispatches commands, however, I may be missing something, could you point out the exact code lines?

Member

sarjon commented Aug 21, 2018

hmm, i think all validation (name and sql + validate if sql query is not malformed) is performed in RequestSqlFormDataValidator, and RequestSqlFormDataProvider only dispatches commands, however, I may be missing something, could you point out the exact code lines?

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 21, 2018

Member

src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/RequestSqlFormDataProvider.php, line 120 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I'm not a fan of this suite of catch.

It's best to avoid switch and switch-like statements like this one. The rule of thumb is: if I the answer to the question "How do I add one more case" is "You need to edit the file and add one more case", then You Are Doing It Wrong.

You can rely on dictionaries instead:

public function saveData(array $requestSqlData)
{

    // ...

    try {
        // ...
    } catch (Exception $e) {
       $errors = $this->handleException($e);
    }

    // ...

}

private function handleException(Exception $e)
{
    $exceptionType = get_class($e);

    if ($exceptionType === SqlRequestConstraintException::class) {
        return $this->getConstraintError($e);
    }

    return $this->getErrorByExceptionType($e);
}

private function getConstraintError(SqlRequestConstraintException $e)
{
    $code = $e->getCode();

    $invalidFieldDictionary = [
        SqlRequestConstraintException::INVALID_NAME_ERROR => 'name',
        SqlRequestConstraintException::INVALID_SQL_QUERY_ERROR => 'sql',
        SqlRequestConstraintException::MALFORMED_SQL_QUERY_ERROR => 'sql';
    ];

    if (isset($invalidFieldDictionary[$code])) {
        return [
            'key' => 'The %s field is invalid.',
            'parameters' => [
                $invalidFieldDictionary[$code]
            ],
            'domain' => 'Admin.Notifications.Error',
        ];
    }

    return [
        'key' => 'Invalid data supplied.',
        'parameters' => [],
        'domain' => 'Admin.Notifications.Error',
    ];
}

private function getErrorByExceptionType(Exception $e)
{
    $dict = [
        SqlRequestNotFoundException::class => [
            'key' => 'The object cannot be loaded (or found)',
            'parameters' => [],
            'domain' => 'Admin.Notifications.Error',
        ],
        CannotAddSqlRequestException::class => [
            'key' => 'An error occurred while creating an object.',
            'parameters' => [],
            'domain' => 'Admin.Notifications.Error',
        ],
       // ...
   ];

   $exceptionType = get_class($e);
    
    if (isset($dict[$exceptionType])) {
        return $dict[$exceptionType];
    }

    return [
        'key' => 'Unexpected error occurred.',
        'parameters' => [],
        'domain' => 'Admin.Notifications.Error',
    ];
}

im wondering if this could be extracted into separate service, like SqlRequestExceptionHandler or similar as it could be used multiple times?

Member

sarjon commented Aug 21, 2018


src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/RequestSqlFormDataProvider.php, line 120 at r5 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I'm not a fan of this suite of catch.

It's best to avoid switch and switch-like statements like this one. The rule of thumb is: if I the answer to the question "How do I add one more case" is "You need to edit the file and add one more case", then You Are Doing It Wrong.

You can rely on dictionaries instead:

public function saveData(array $requestSqlData)
{

    // ...

    try {
        // ...
    } catch (Exception $e) {
       $errors = $this->handleException($e);
    }

    // ...

}

private function handleException(Exception $e)
{
    $exceptionType = get_class($e);

    if ($exceptionType === SqlRequestConstraintException::class) {
        return $this->getConstraintError($e);
    }

    return $this->getErrorByExceptionType($e);
}

private function getConstraintError(SqlRequestConstraintException $e)
{
    $code = $e->getCode();

    $invalidFieldDictionary = [
        SqlRequestConstraintException::INVALID_NAME_ERROR => 'name',
        SqlRequestConstraintException::INVALID_SQL_QUERY_ERROR => 'sql',
        SqlRequestConstraintException::MALFORMED_SQL_QUERY_ERROR => 'sql';
    ];

    if (isset($invalidFieldDictionary[$code])) {
        return [
            'key' => 'The %s field is invalid.',
            'parameters' => [
                $invalidFieldDictionary[$code]
            ],
            'domain' => 'Admin.Notifications.Error',
        ];
    }

    return [
        'key' => 'Invalid data supplied.',
        'parameters' => [],
        'domain' => 'Admin.Notifications.Error',
    ];
}

private function getErrorByExceptionType(Exception $e)
{
    $dict = [
        SqlRequestNotFoundException::class => [
            'key' => 'The object cannot be loaded (or found)',
            'parameters' => [],
            'domain' => 'Admin.Notifications.Error',
        ],
        CannotAddSqlRequestException::class => [
            'key' => 'An error occurred while creating an object.',
            'parameters' => [],
            'domain' => 'Admin.Notifications.Error',
        ],
       // ...
   ];

   $exceptionType = get_class($e);
    
    if (isset($dict[$exceptionType])) {
        return $dict[$exceptionType];
    }

    return [
        'key' => 'Unexpected error occurred.',
        'parameters' => [],
        'domain' => 'Admin.Notifications.Error',
    ];
}

im wondering if this could be extracted into separate service, like SqlRequestExceptionHandler or similar as it could be used multiple times?

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

Too bad, let's hope we can upgrade to php 7 soon

isn't too early to use CQRS if we have to use 0.x version of a bundle? Also, if we have PHP 7.1, we coud rely on the official composant of Symfony => https://symfony.com/doc/current/messenger.html

I don't want to rewrite this part in 1.8 :/

Contributor

mickaelandrieu commented Aug 21, 2018

Too bad, let's hope we can upgrade to php 7 soon

isn't too early to use CQRS if we have to use 0.x version of a bundle? Also, if we have PHP 7.1, we coud rely on the official composant of Symfony => https://symfony.com/doc/current/messenger.html

I don't want to rewrite this part in 1.8 :/

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

Don't you think this should be handled in the form data provider? Of course, it would need to have the command bus injected.

I discourage you to do that, you may have to redo/re adapt all the configuration pages we have already migrated, we clearly don't have time to do that to stick with our roadmap. You'd better create new implementations of the current interfaces or rely on new interfaces.

as far as i understand FormDataProviderInterface is meant to be used in "options" forms. The problem with it is getData() method, which does not allow you to specific id of entity which data you want to get, thus imo this interface could better be under Core/Form/Option/FormDataProviderInterface namespace?

Note that the way we handle forms was not done with CQRS/ES in mind: you'd better not rely at all on current interfaces and work on a totally new way to handle forms when - in the same time - you need to ensure that hooks on forms are still dispatched and will react the same way as before.

Adding new architecture always have a cost, it's never easy, and - I repeat myself - I still don't see the gain as we don't have identified entities (in DDD meaning of the term) nor use cases.

Contributor

mickaelandrieu commented Aug 21, 2018

Don't you think this should be handled in the form data provider? Of course, it would need to have the command bus injected.

I discourage you to do that, you may have to redo/re adapt all the configuration pages we have already migrated, we clearly don't have time to do that to stick with our roadmap. You'd better create new implementations of the current interfaces or rely on new interfaces.

as far as i understand FormDataProviderInterface is meant to be used in "options" forms. The problem with it is getData() method, which does not allow you to specific id of entity which data you want to get, thus imo this interface could better be under Core/Form/Option/FormDataProviderInterface namespace?

Note that the way we handle forms was not done with CQRS/ES in mind: you'd better not rely at all on current interfaces and work on a totally new way to handle forms when - in the same time - you need to ensure that hooks on forms are still dispatched and will react the same way as before.

Adding new architecture always have a cost, it's never easy, and - I repeat myself - I still don't see the gain as we don't have identified entities (in DDD meaning of the term) nor use cases.

* International Registered Trademark & Property of PrestaShop SA
*/
namespace PrestaShop\PrestaShop\Core\Domain\SqlManagement\CommandHandler;

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

imo the Core is "our" Domain, we shouldn't have a namespace named "Domain" ^^

wdyt @eternoendless ?

@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

imo the Core is "our" Domain, we shouldn't have a namespace named "Domain" ^^

wdyt @eternoendless ?

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 21, 2018

Member

you may have to redo/re adapt all the configuration pages

from what i understand (i may be wrong) updating options would change only Configuration implementations, for example from:

// StockConfiguration.php

class StockConfiguration implements DataConfigurationInterface
{
    public function getConfiguration()
    {
        return [
            'allow_ordering_oos' => $this->configuration->getBoolean('PS_ORDER_OUT_OF_STOCK'),
            'stock_management' => $this->configuration->getBoolean('PS_STOCK_MANAGEMENT'),
            // ...
        ];
    }

    public function updateConfiguration(array $config)
    {
        $errors = [];

        if ($this->validateConfiguration($config)) {
            $this->configuration->set('PS_ORDER_OUT_OF_STOCK', (int) $config['allow_ordering_oos']);
            $this->configuration->set('PS_PACK_STOCK_TYPE', $config['pack_stock_management']);
            // ...
        }

        return $errors;
    }
}

To:

// StockConfiguration.php

class StockConfiguration implements DataConfigurationInterface
{
    public function getConfiguration()
    {
       $query = new GetProductStockConfigurationQuery();
       $productStockConfiguration = $this->queryBus->handle($query);

       return $productStockConfiguration;
    }

    public function updateConfiguration(array $config)
    {
        $errors = [];

        if ($this->validateConfiguration($config)) {
            $command = new UpdateProductStockConfigurationCommand(
                $config['allow_ordering_oos'],
                // ...
            );

            $this->commandBus->handle($command);
        }

        return $errors;
    }
}

In that case, everything works the same? wdyt @mickaelandrieu ?

Member

sarjon commented Aug 21, 2018

you may have to redo/re adapt all the configuration pages

from what i understand (i may be wrong) updating options would change only Configuration implementations, for example from:

// StockConfiguration.php

class StockConfiguration implements DataConfigurationInterface
{
    public function getConfiguration()
    {
        return [
            'allow_ordering_oos' => $this->configuration->getBoolean('PS_ORDER_OUT_OF_STOCK'),
            'stock_management' => $this->configuration->getBoolean('PS_STOCK_MANAGEMENT'),
            // ...
        ];
    }

    public function updateConfiguration(array $config)
    {
        $errors = [];

        if ($this->validateConfiguration($config)) {
            $this->configuration->set('PS_ORDER_OUT_OF_STOCK', (int) $config['allow_ordering_oos']);
            $this->configuration->set('PS_PACK_STOCK_TYPE', $config['pack_stock_management']);
            // ...
        }

        return $errors;
    }
}

To:

// StockConfiguration.php

class StockConfiguration implements DataConfigurationInterface
{
    public function getConfiguration()
    {
       $query = new GetProductStockConfigurationQuery();
       $productStockConfiguration = $this->queryBus->handle($query);

       return $productStockConfiguration;
    }

    public function updateConfiguration(array $config)
    {
        $errors = [];

        if ($this->validateConfiguration($config)) {
            $command = new UpdateProductStockConfigurationCommand(
                $config['allow_ordering_oos'],
                // ...
            );

            $this->commandBus->handle($command);
        }

        return $errors;
    }
}

In that case, everything works the same? wdyt @mickaelandrieu ?

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 21, 2018

Member

we coud rely on the official composant of Symfony => https://symfony.com/doc/current/messenger.html

Im not sure if thats a problem but: This Component is experimental. Experimental features are not covered by Symfony's BC-break policy. It written in their github repo.

Member

sarjon commented Aug 21, 2018

we coud rely on the official composant of Symfony => https://symfony.com/doc/current/messenger.html

Im not sure if thats a problem but: This Component is experimental. Experimental features are not covered by Symfony's BC-break policy. It written in their github repo.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

In that case, everything works the same? wdyt @mickaelandrieu ?

depends: are a commandBus allowed to return output, here $errors (that may need to be updated)

For instance, is it allowed to do:

$errors = $this->commandBus->handle($command);

This Component is experimental. Experimental features are not covered by Symfony's BC-break policy.

it is for now, but we don't expect to upgrade to PHP 7 now but in months so I guess the Messenger component (or a stable version of tactician) will be used.

We had very bad experience on relying on non stable deps (bootstrap 4), I guess we should put a rule here @eternoendless: NO dev/unstable/unmaintained dependencies added in the core: wdyt?

Contributor

mickaelandrieu commented Aug 21, 2018

In that case, everything works the same? wdyt @mickaelandrieu ?

depends: are a commandBus allowed to return output, here $errors (that may need to be updated)

For instance, is it allowed to do:

$errors = $this->commandBus->handle($command);

This Component is experimental. Experimental features are not covered by Symfony's BC-break policy.

it is for now, but we don't expect to upgrade to PHP 7 now but in months so I guess the Messenger component (or a stable version of tactician) will be used.

We had very bad experience on relying on non stable deps (bootstrap 4), I guess we should put a rule here @eternoendless: NO dev/unstable/unmaintained dependencies added in the core: wdyt?

@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Aug 21, 2018

Contributor

@mickaelandrieu 0.x doesn't mean it's not stable. Many projects begins at 0 and not 1 ( like array :D ) https://github.com/thephpleague/tactician-bundle/releases/tag/v0.4.1 There is nothing said it's a alpha / beta / pre-release in this version.

Contributor

PierreRambaud commented Aug 21, 2018

@mickaelandrieu 0.x doesn't mean it's not stable. Many projects begins at 0 and not 1 ( like array :D ) https://github.com/thephpleague/tactician-bundle/releases/tag/v0.4.1 There is nothing said it's a alpha / beta / pre-release in this version.

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 21, 2018

Member

isn't too early to use CQRS if we have to use 0.x version of a bundle

it is stable version (as they had Pre-0.4.0). By reading their changelog, i see that we dont use anything that was changed with BC break in v1.0.

depends: are a commandBus allowed to return output, here $errors (that may need to be updated)

Regarding errors, yes you can return result from commands, but as far as i know its better to throw exception and handle it where the command was dispatched.

From experience of what i already did using CQRS it is much easier to migrate CRUD using commands.

Member

sarjon commented Aug 21, 2018

isn't too early to use CQRS if we have to use 0.x version of a bundle

it is stable version (as they had Pre-0.4.0). By reading their changelog, i see that we dont use anything that was changed with BC break in v1.0.

depends: are a commandBus allowed to return output, here $errors (that may need to be updated)

Regarding errors, yes you can return result from commands, but as far as i know its better to throw exception and handle it where the command was dispatched.

From experience of what i already did using CQRS it is much easier to migrate CRUD using commands.

@mickaelandrieu

This comment has been minimized.

Show comment
Hide comment
@mickaelandrieu

mickaelandrieu Aug 21, 2018

Contributor

@sarjon as the main interest @eternoendless pointed me was that CQRS make the code easier to test, I'm really interested to see some tests here. I'm not asking them to be done now, but they are important before a potential merge.

Also, as it sounds really complicated (lot of classes for simple use cases), docs needs to be done, at least for us to be able to maintain it without break the architecture and to help us in further migrations 👍

Contributor

mickaelandrieu commented Aug 21, 2018

@sarjon as the main interest @eternoendless pointed me was that CQRS make the code easier to test, I'm really interested to see some tests here. I'm not asking them to be done now, but they are important before a potential merge.

Also, as it sounds really complicated (lot of classes for simple use cases), docs needs to be done, at least for us to be able to maintain it without break the architecture and to help us in further migrations 👍

@eternoendless

I still don't see the gain as we don't have identified entities (in DDD meaning of the term) nor use cases.

@mickaelandrieu CQRS nothing to do with entities. Commands and Queries exist for the sole purpose of separating data fetches from state changes. There are many reasons for having this:

Separating the flow for writes and reads, not only allows multiple optimizations like writing into master and reading from slave, but it also establishes a clear pattern where you can know that a read won't have any side effects.

This also allows us to slowly build up an API of "what the business is capable of doing". We don't need to know all the user cases upfront, because a single Query or Command answers to a single user case. They can be created as we migrate pages, and CRUD is the simplest form of user case that you can find.

And most importantly, this allows us to encapsulate our legacy Entities and move forward with the migration without having to migrate our Entities first.

Reviewed 1 of 2 files at r4, 3 of 13 files at r6, 16 of 22 files at r7, 19 of 19 files at r8, 12 of 12 files at r9.
Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @PierreRambaud, @sarjon, and @eternoendless)


src/Adapter/SqlManager/RequestSqlExporter.php, line 84 at r8 (raw file):

    {
        try {
            $command = new GetSqlRequestExecutionResultQuery($requestSqlId);

This is a query, not a command :)


src/Adapter/SqlManager/RequestSqlExporter.php, line 89 at r8 (raw file):

            $sqlRequestExecutionResult = $this->queryBus->handle($command);
        } catch (SqlRequestException $e) {
            return null;

This will hide errors away from the client


src/Adapter/SqlManager/SqlRequestFormDataValidator.php, line 32 at r8 (raw file):


/**
 * Class RequestSqlFormDataValidator

I think it's good practice to describe what your class does. Often, the sole act of putting thoughts down in words helps you realize about design problems.


src/Adapter/SqlManager/CommandHandler/BulkDeleteSqlRequestHandler.php, line 55 at r8 (raw file):


                if (false === $entity->delete()) {
                    $errorOccurred = true;

I think you should fail fast and throw the exception right away. That way you can know exactly which deletion failed.


src/Core/Cqrs/CommandBusInterface.php, line 27 at r9 (raw file):

Previously, sarjon (Šarūnas Jonušas) wrote…

i guess namespace naming is not the best.. :/

CommandBus is the simplest


src/Core/Cqrs/CommandBusInterface.php, line 32 at r9 (raw file):

Previously, sarjon (Šarūnas Jonušas) wrote…

do we need QueryBus as well?

I don't think so


src/Core/Cqrs/TacticianCommandBusAdapter.php, line 34 at r9 (raw file):

Previously, sarjon (Šarūnas Jonušas) wrote…

@eternoendless wdyt?

Why not make it inherit CommandBus directly?


src/Core/Domain/SqlManagement/CommandHandler/EditSqlRequestHandlerInterface.php, line 27 at r7 (raw file):

Previously, mickaelandrieu (Mickaël Andrieu) wrote…

imo the Core is "our" Domain, we shouldn't have a namespace named "Domain" ^^

wdyt @eternoendless ?

The Domain namespace is meant to make a clear boundary for DD code. In there we can have DTOs/VOs, Commands, Queries, and later, Events. In the long term, I'd want the objects within to be the One And Only Way of interacting with the Core Business, as a sort of "internal API". The rest of the core would be composed of "helper" classes which convey no business logic.


src/Core/Domain/SqlManagement/QueryHandler/GetSqlRequestExecutionResultHandlerInterface.php, line 33 at r8 (raw file):


/**
 * Interface GetSqlRequestResultForViewingHandlerInterface defines contract for getting SqlRequest SQL query result

There's no "ForViewing" anymore


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/RequestSqlController.php, line 446 at r8 (raw file):

     * @return string Error message
     */
    protected function handleException(SqlRequestException $e)

This method could get out of control if you use to display errors in different contexts. You don't expect the same kinds of exceptions when viewing and when deleting, so maybe they should be handled separately.


src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/RequestSqlController.php, line 465 at r8 (raw file):

        ];

        if (CannotDeleteSqlRequestException::class === $exceptionType &&

Please put the && at the start of the line


src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/RequestSqlFormDataProvider.php, line 120 at r5 (raw file):

Previously, sarjon (Šarūnas Jonušas) wrote…

im wondering if this could be extracted into separate service, like SqlRequestExceptionHandler or similar as it could be used multiple times?

The problem as I see it is that the message is context-dependent.

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 24, 2018

Member

src/Core/Cqrs/TacticianCommandBusAdapter.php, line 34 at r9 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why not make it inherit CommandBus directly?

does not work that way, Tactician's command bus is configured in bundle so if i extend it, the code breaks, i could check if its possible to configure your own CommandBus.

Member

sarjon commented Aug 24, 2018


src/Core/Cqrs/TacticianCommandBusAdapter.php, line 34 at r9 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Why not make it inherit CommandBus directly?

does not work that way, Tactician's command bus is configured in bundle so if i extend it, the code breaks, i could check if its possible to configure your own CommandBus.

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Aug 24, 2018

Member

src/Adapter/SqlManager/CommandHandler/BulkDeleteSqlRequestHandler.php, line 55 at r8 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I think you should fail fast and throw the exception right away. That way you can know exactly which deletion failed.

i totally agree, but this is how it was done in legacy, so we still go through every entity and try to delete it even though last one failed, can we change it?

Member

sarjon commented Aug 24, 2018


src/Adapter/SqlManager/CommandHandler/BulkDeleteSqlRequestHandler.php, line 55 at r8 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I think you should fail fast and throw the exception right away. That way you can know exactly which deletion failed.

i totally agree, but this is how it was done in legacy, so we still go through every entity and try to delete it even though last one failed, can we change it?

@eternoendless

Reviewable status: all files reviewed, 16 unresolved discussions (waiting on @PierreRambaud, @sarjon, @mickaelandrieu, and @eternoendless)


src/Adapter/SqlManager/CommandHandler/BulkDeleteSqlRequestHandler.php, line 55 at r8 (raw file):

Previously, sarjon (Šarūnas Jonušas) wrote…

i totally agree, but this is how it was done in legacy, so we still go through every entity and try to delete it even though last one failed, can we change it?

I think it's best to change it.


src/Core/Cqrs/TacticianCommandBusAdapter.php, line 34 at r9 (raw file):

Previously, sarjon (Šarūnas Jonušas) wrote…

does not work that way, Tactician's command bus is configured in bundle so if i extend it, the code breaks, i could check if its possible to configure your own CommandBus.

Ok, see if it's possible.

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 4, 2018

Member

@eternoendless rebase done.

Member

sarjon commented Sep 4, 2018

@eternoendless rebase done.

];
if (isset($domainErrors[$type])) {
return $domainErrors[$type];

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 5, 2018

Contributor

this controller has too many responsibilities! Everything that is not related to HTTP layer should be moved into related services.

@mickaelandrieu

mickaelandrieu Sep 5, 2018

Contributor

this controller has too many responsibilities! Everything that is not related to HTTP layer should be moved into related services.

This comment has been minimized.

@sarjon

sarjon Sep 5, 2018

Member

see comments above regarding that.

@sarjon

sarjon Sep 5, 2018

Member

see comments above regarding that.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 5, 2018

Contributor

I don't block this pull request, but this needs to be refactored: I disagree having to maintain 600+ lines controller /c @eternoendless this is breaking the architecture I try to push: thin controllers, fat services.

The controllers should never contain business logic, only calls to services.

@mickaelandrieu

mickaelandrieu Sep 5, 2018

Contributor

I don't block this pull request, but this needs to be refactored: I disagree having to maintain 600+ lines controller /c @eternoendless this is breaking the architecture I try to push: thin controllers, fat services.

The controllers should never contain business logic, only calls to services.

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Sep 5, 2018

Contributor

Also "How to test" section needs to be filled, please :)

@mickaelandrieu

mickaelandrieu Sep 5, 2018

Contributor

Also "How to test" section needs to be filled, please :)

This comment has been minimized.

@sarjon

sarjon Sep 5, 2018

Member

sorry, done.

@sarjon

sarjon Sep 5, 2018

Member

sorry, done.

This comment has been minimized.

@eternoendless

eternoendless Sep 6, 2018

Member

@mickaelandrieu I think that matching exceptions types to error messages is a controller's responsibility. I'm open to discuss about it if you don't agree.

@eternoendless

eternoendless Sep 6, 2018

Member

@mickaelandrieu I think that matching exceptions types to error messages is a controller's responsibility. I'm open to discuss about it if you don't agree.

@eternoendless

Reviewed 1 of 92 files at r19, 16 of 75 files at r20, 2 of 24 files at r21, 4 of 45 files at r22, 5 of 36 files at r25, 5 of 11 files at r26, 63 of 79 files at r27.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @PierreRambaud, @sarjon, @LouiseBonnard, and @mickaelandrieu)

@eternoendless eternoendless added this to the 1.7.5.0 milestone Sep 5, 2018

@marionf marionf self-assigned this Sep 6, 2018

@marionf marionf added QA ✔️ and removed waiting for QA labels Sep 6, 2018

@marionf marionf removed their assignment Sep 6, 2018

@sarjon

This comment has been minimized.

Show comment
Hide comment
@sarjon

sarjon Sep 6, 2018

Member

ping @eternoendless, merge? ^^

Member

sarjon commented Sep 6, 2018

ping @eternoendless, merge? ^^

@PierreRambaud PierreRambaud merged commit 8659634 into PrestaShop:develop Sep 6, 2018

2 of 3 checks passed

code-review/reviewable 6 discussions left (LouiseBonnard, mickaelandrieu, PierreRambaud, sarjon)
Details
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@PierreRambaud

This comment has been minimized.

Show comment
Hide comment
@PierreRambaud

PierreRambaud Sep 6, 2018

Contributor

Thanks @sarjon and everyone for reviews.

Contributor

PierreRambaud commented Sep 6, 2018

Thanks @sarjon and everyone for reviews.

if (false === $entity->delete()) {
throw new CannotDeleteSqlRequestException(
sprintf('Could not delete SqlRequest with id %s', var_export($entityId)),

This comment has been minimized.

@jolelievre

jolelievre Sep 6, 2018

Contributor

@sarjon isn't there a missing true in this var_export call?

@jolelievre

jolelievre Sep 6, 2018

Contributor

@sarjon isn't there a missing true in this var_export call?

This comment has been minimized.

@sarjon

sarjon Sep 6, 2018

Member

you are totally right @jolelievre

@sarjon

sarjon Sep 6, 2018

Member

you are totally right @jolelievre

@matks matks added the migration label Sep 18, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment