Prestashop: [SF Migration] Entity form management - CRUD form handlers

[sarjon]

type: tech-issue;
status| in-progress;
comment | CRUD form handlers - WIP and 1st pages experiment;

PR Summary

Todo:

• [x] 1st PR which introduces major concepts, interfaces and classes (https://github.com/PrestaShop/PrestaShop/pull/10974)
• [x] Handles latest code review feedbacks such as https://github.com/PrestaShop/PrestaShop/pull/10974/files#r235738277
• [x] Write documentation about CRUD controllers: explain roles involved in a CRUD page (Form Builder, Form Data Provider, Form Data Handler ...), use SQL page as an example, explain how another CRUD Symfony page can be built (for example using factories for services) https://github.com/PrestaShop/docs/pull/184
• [x] Write documentation about CQRS and Command Bus https://github.com/PrestaShop/docs/pull/184
• [x] Add unit tests for these classes

Intro

Following the idea introduce in this PR PrestaShop/PrestaShop#8908 for Configuration forms management, maybe we can have similar workflow for Entity forms as well?

Possible solution

Below is an example workflow of how Edit action of any Entity could look like:

What's missing:

• [Hook is not dispatched when form data is handled. Maybe some middleware between EntityController and EntityFormDataHandler could solve it?
• What would be a correct for form data validation?

So Entity form management would require developer to implement specific EntityFormDataHandler & EntityFormDataProvider for each entity and configure EntityFormFactory (similar as it is wit Options).

Here is PR that defines contracts and EntityFormFactory implementation: https://github.com/PrestaShop/PrestaShop/pull/10286

Possible improvement

It would be cool if we had common EntityController that could be used for every entity so migration could be faster, see example below.

Lets assume we are working with Product entity. First we would configure routes:

# our implemented product listing
admin_product_listing:
  path: /product
  methods: [GET]
  defaults:
    _controller: PrestaShopBundle:Admin/Catalog/Product/Product:listing
    _legacy_controller: AdminProduct

# no need to implement, just configure
admin_product_create:
  path: /product/new
  methods: [GET]
  defaults:
    _controller: PrestaShopBundle:Admin/Common/Entity:showCreate
   template: '@PrestaShop/Admin/Catalog/Products/create.html.twig' # we would have to render form by ourselves
   entity_form_factory: entity_form_factory_service_id

# no need to implement, just configure
admin_product_process_create:
  path: /product/new
  methods: [POST]
  defaults:
    _controller: PrestaShopBundle:Admin/Common/Entity:processCreate
   entity_form_factory: entity_form_factory_service_id
   entity_form_data_handler: entity_form_data_handler_service_id
   redirect_after_success: admin_product_listing
   redirect_after_error: admin_product_create

And reusable EntityController:

class EntityController extends FrameworkAdminBundle
{
    // EntityFormFactoryInterface could be autowired using argument resolver
    public function showCreate(EntityFormFactoryInterface $entityFormfactory, $template)
    {
        $entityCreationForm = $entityFormFactory->getForm();

        return $this->render($template, [
            'form' => $entityCreationForm->createView(),
        ]);
    }

    // services could be autowired
    public function processCreate(
        Request $request,
        EntityFormFactoryInterface $entityFormFactory,
        EntityFormDataHandlerInterface $entityFormDataHandler
    ) {
        $entityCreationForm = $entityFormFactory->getForm();
        $entityCreationForm->handleRequest($request);

        if ($entityCreationForm->isSubmitted()) {
            $errors = $entityFormDataHandler->create(
                $entityCreationForm->getData()
            );

            if (!empty($errors)) {
                $this->flashErrors($errors);

                return $this->redirectToRoute($request->attributes->get('redirect_after_error'));
            }
        }

        $this->addFlash('success', 'Successfully created!');

        return $this->redirectToRoute($request->attributes->get('redirect_after_success'));
    }

    // similar methods for editting
    // ...
}

This would save a lot of copy-paste code between different entity controllers as most of the actions are the same.

What's missing:

• How to persist form data when processing failed? Use session maybe?

Note

_I did not test it and i dont know if that would actually work_

[sarjon]

@mickaelandrieu @jolelievre your insights are more than welcome. :)

[PierreRambaud]

I like the idea =)

[PierreRambaud]

To persist data, you maybe need to use one action for get & post without redirect in case of failure?

[sarjon]

@PierreRambaud that would work for sure, but i thought maybe someone knows how to split show & process actions. :)

[jolelievre]

Hello @sarjon Really nice, I like the idea of a common Controller, this will save a lot of time and help the module developers a lot. Just to be sure I get everything, compared to the previous form architecture we had:
FormHandlerInterface becomes EntityFormFactory
FormDataProviderInterface is split in two interfaces EntityFormDataProvider and EntityFormDataHandler

/**
 * Contains the identifier of an entity, the identifier can be of different
 * types int (ex: ID), string (ex: uuid), associative array (ex: double primary
 * keys, to identify a relation, to target an entity in a special shop/lang context)
 */
interface EntityIdentifierInterface
{
    /**
     * @return int|string|array
     */
    public function getIdentifier();
}

/**
 * Manage Symfony entity forms outside the controllers.
 */
interface EntityFormFactoryInterface
{
    /**
     * @return FormInterface
     */
    public function getForm();

    /**
     * @return FormInterface
     */
    public function getFormFor(EntityIdentifierInterface $identifier);
}

/**
 * Symfony entity forms data provider.
 */
interface EntityFormDataProviderInterface
{
    /**
     * Returns the form default for creation data as an associative array
     *
     * @return array
     */
    public function getDefaultData();

    /**
     * Returns the form data as an associative array
     *
     * @param EntityIdentifier $identifier
     *
     * @return array
     */
    public function getDataFor(EntityIdentifierInterface $identifier);
}

/**
 * Symfony entity forms data handler.
 */
interface EntityFormDataHandlerInterface
{
    /**
     * Updates an entity with the flatten data as associative array
     *
     * @param array            $data
     *
     * @return array
     */
    public function create(array $data);

    /**
     * Updates an entity with the flatten data as associative array
     *
     * @param EntityIdentifier $identifier
     * @param array            $data
     *
     * @return array
     */
    public function update(EntityIdentifierInterface $identifier, array $data);
}

[jolelievre]

About the persist form data, I would keep the two actions, but the process can render the form when there is an error and redirects only on success
About who dispatches the hook when data is handled, couldn't it be managed by the EntityFormDataHandler? In create and update methods, we coud have a
action{entityName}PreCreate
action{entityName}PostCreate
action{entityName}PreUpdate
action{entityName}PostUpdate

[jolelievre]

I was also thinking about the errors returned by the EntityFormDataHandler it would be cleaner to return objects/interfaces rather than arrays And the errors could have an optional propertyPath (kinda like symfony) allowing to display the error messages in-form The errors without propertyPath would still be displayed via flashMessages

[sarjon]

About who dispatches the hook when data is handled, couldn't it be managed by the EntityFormDataHandler

what i dont like about it is that developers would have to dispatch hook by hand every time they implement data handler and im sure that some handlers would do that and some wont (developer forgets to dispatch hook or some other reasons). thats why i would like it to be handled by the core. :)

[jolelievre]

you're right it would be preferable if the core manages it
then maybe EntityFormFactory should do it to avoid introducing another component, just like FormHandler did it in the previous architecture In which case the Handler word is more appropriate

[jolelievre]

So do you think it's better to have two components? One that manages the form creation via the EntityFormDataProviderInterface, and the other which will manage the writing via EntityFormDataHandlerInterface ?

/**
 * Symfony entity forms data handler.
 */
interface EntityFormDataHandlerInterface
{
    /**
     * Updates an entity with the flatten data as associative array
     * Returns an array of error in case of failure, or an EntityIdentifierInterface
     * on succeed
     *
     * @param array $data
     *
     * @return array|EntityIdentifierInterface
     */
    public function create(array $data);

    /**
     * Updates an entity with the flatten data as associative array
     *
     * @param EntityIdentifier $identifier
     * @param array            $data
     *
     * @return array
     */
    public function update(EntityIdentifierInterface $identifier, array $data);
}

/**
 * Symfony form factory, populates the created Form
 */
interface EntityFormFactoryInterface
{
    /**
     * @return Symfony\Component\Form\FormInterface
     */
    public function getForm();

    /**
     * @return Symfony\Component\Form\FormInterface
     */
    public function getFormFor(EntityIdentifierInterface $identifier);
}

/**
 * Symfony form handler, used to save the data from a symfony form
 */
interface EntityFormHandlerInterface
{
    /**
     * @param Symfony\Component\Form\FormInterface $form
     * 
     * @return array|EntityIdentifierInterface
     */
    public function createEntity(FormInterface $form);

    /**
     * @param Symfony\Component\Form\FormInterface $form
     * @param EntityIdentifierInterface            $identifier
     * 
     * @return array
     */
    public function updateEntity(FormInterface $form, EntityIdentifierInterface $identifier);
}

/**
 * This class handle a Symfony form and saves its data via EntityFormDataHandlerInterface
 * It also dispatch before and after creation/update
 */
class EntityFormHandler implements EntityFormHandlerInterface
{
    /**
     * @var EntityFormDataHandlerInterface
     */
    protected $dataHandler;

    /**
     * @var HookDispatcherInterface the event dispatcher
     */
    protected $hookDispatcher;

    /**
     * @var string the hook name
     */
    protected $entityName;

    public function __construct(
        EntityFormDataHandlerInterface $dataHandler,
        HookDispatcherInterface $hookDispatcher,
        $entityName
    ) {
        $this->dataHandler = $dataHandler;
        $this->hookDispatcher = $hookDispatcher;
        $this->entityName = $entityName;
    }

    /**
     * @param Symfony\Component\Form\FormInterface $form
     * 
     * @return array|EntityIdentifierInterface
     */
    public function createEntity(FormInterface $form)
    {
        $formData = $form->getData();
        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->entityName}PreCreate",
            [
                'form_data' => &$formData,
            ]
        );

        $result = $this->dataHandler->create($formData);
        if ($result instanceof EntityIdentifierInterface) {
            $identifier = $result;
            $errors = [];
        } else {
            $identifier = null;
            $errors = $errors;
        }

        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->entityName}PostCreate",
            [
                'identifier' => $identifier,
                'errors' => &$errors,
                'form_data' => &$data,
            ]
        );

        return count($errors) > 0 ? $errors : $identifier;
    }

    /**
     * @param Symfony\Component\Form\FormInterface $form
     * @param EntityIdentifierInterface            $identifier
     * 
     * @return array
     */
    public function updateEntity(FormInterface $form, EntityIdentifierInterface $identifier)
    {
        $formData = $form->getData();
        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->entityName}PreUpdate",
            [
                'identifier' => $identifier,
                'form_data' => &$formData,
            ]
        );

        $errors = $this->dataHandler->update($identifier, $formData);

        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->entityName}PostUpdate",
            [
                'identifier' => $identifier,
                'errors' => &$errors,
                'form_data' => &$data,
            ]
        );

        return $errors;
    }
}

[jolelievre]

although I'm not a big fan of the multi typed return.. maybe errors should be thrown? or maybe add validateEntity method to the EntityFormHandlerInterface

[sarjon]

regarding hooks, i think they are duplicate of these:
https://github.com/PrestaShop/PrestaShop/blob/develop/classes/ObjectModel.php#L530
https://github.com/PrestaShop/PrestaShop/blob/develop/classes/ObjectModel.php#L600

dont you think?

also with methods updateEntity(FormInterface $form, EntityIdentifierInterface $identifier) and createEntity(FormInterface $form) it seems that passing whole $form is not necessary as you only get data from it. So it could expect form data only maybe?

[jolelievre]

About the ObjectModel hooks, I thought the aim of the migration was to stop using them so they wouldn't be sent twice, but we sure should use the same value for backward compatibility
also about using the FormInterface you're probably right, and it avoids coupling the handler with the Symfony FormInterface and it would make it more generic and allow to use it via other inputs (like json for batch inputs, or in an API)
but then maybe it should not be named EntityFormHandlerInterface since it is not necessary linked to a form, any idea on the naming?
EntityUpdaterInterface (not fitting for creation), EntityWriterInterface (not fan of the too generic "writer" name), EntitySaverInterface, EntityQueryInterface ...?

[sarjon]

would it be such a bad idea for EntityFormDataProviderInterface to have save() and update() methods? 🤔 it would be the same as it is with options data providers, they have getData and saveData.

[jolelievre]

you mean only one interface combining EntityFormDataProviderInterface and EntityFormDataHandlerInterface ? with getDefaultData() getDataFor($id) and update($i, $data) create($data) ? (I prefer create rather than save, it is more specific in this case)
I don't see any inconvenience to do this

[sarjon]

lets wait for others opinion, maybe @PierreRambaud @mickaelandrieu @eternoendless or anyone else. :)

[PierreRambaud]

Agreed with @sarjon , thinking too it's a bad idea to have save() and update() when we only save() and we logically don't care if it's an update or a insert. Easier to save mass Entity objects without having to check if it's a new one or an update.

```php

/**

• Symfony entity forms data handler.
  /
  interface EntityFormDataHandlerInterface
  {
  /*
  
  
  
  • Save an entity with the flatten data as associative array
  
  
  • Returns an array of error in case of failure, or an EntityIdentifierInterface
  
  
  • on succeed
    
    *
  
  
  • @param EntityIdentifier $identifier
  
  
  • @param array $data
    
    *
  
  
  • @return array
    
    */
    
    public function save(array $data, EntityIdentifierInterface $identifier = null);
    
    }
  
  

/**

• Symfony form factory, populates the created Form
  /
  interface EntityFormFactoryInterface
  {
  /*
  
  
  
  • @return Symfony\Component\FormFormInterface
    
    */
    
    public function getForm(EntityIdentifierInterface $identifier = null);
    
    }
  
  

/**

• Symfony form handler, used to save the data from a symfony form
  /
  interface EntityFormHandlerInterface
  {
  /*
  
  
  
  • @param Symfony\Component\FormFormInterface $form
  
  
  • @param EntityIdentifierInterface $identifier
  
  
  • 
    
    
    
    
    
    • @return array
      
      
      */
      
      
      public function save(FormInterface $form, EntityIdentifierInterface $identifier = null);
      
      
      }
    
    
    
  
  

/**

• This class handle a Symfony form and saves its data via EntityFormDataHandlerInterface
• It also dispatch before and after creation/update
  /
  class EntityFormHandler implements EntityFormHandlerInterface
  {
  /*
  
  
  
  • @var EntityFormDataHandlerInterface
    
    */
    
    protected $dataHandler;
  
  

/**
 * @var HookDispatcherInterface the event dispatcher
 */
protected $hookDispatcher;

/**
 * @var string the hook name
 */
protected $entityName;

public function __construct(
  EntityFormDataHandlerInterface $dataHandler,
  HookDispatcherInterface $hookDispatcher,
  $entityName
) {
  $this->dataHandler = $dataHandler;
  $this->hookDispatcher = $hookDispatcher;
  $this->entityName = $entityName;
}

/**
 * @param Symfony\Component\Form\FormInterface $form
 * @param EntityIdentifierInterface            $identifier
 * 
 * @return array
 */
public function saveEntity(FormInterface $form, EntityIdentifierInterface $identifier = null)
{
  $type = $identifier ? 'Update' : 'Create';
  $formData = $form->getData();
  $this->hookDispatcher->dispatchWithParameters(
    "action{$this->entityName}Pre${$type}",
    [
      'identifier' => $identifier,
      'form_data' => &$formData,
    ]
  );

  $errors = $this->dataHandler->save($formData, $identifier);

  $this->hookDispatcher->dispatchWithParameters(
    "action{$this->entityName}Post${$type}",
    [
      'identifier' => $identifier,
      'errors' => &$errors,
      'form_data' => &$data,
    ]
  );

  return count($errors) > 0 ? $errors : $identifier;
}

}
```

[jolelievre]

I love that! It looks like what I proposed initially :P But I don't remember who was not a fan of the nullable argument (@eternoendless or @mickaelandrieu ?)

[jolelievre]

hello @sarjon I commented a few lines in your PR https://github.com/PrestaShop/PrestaShop/pull/10286
also I just thought about something to manage the hook dispatching (base on chat @PierreRambaud proposed), we could have a core implementation of the provider interface which would decorate the general interface:

/**
 * Symfony entity forms data provider.
 */
interface EntityFormDataProviderInterface
{
    /**
     * Returns the form data from the Entity when $identifier is provided, or the default one instead
     *
     * @return array
     */
    public function getData(EntityIdentifierInterface $identifier = null);

    /**
     * Saves the provided data, create a new entity if $identifier is null, or updates the matching Entity instead
     *
     * @param EntityIdentifier $identifier
     *
     * @return array|EntityIdentifier
     */
    public function saveData(array $data, EntityIdentifierInterface $identifier = null);
}

/**
 * Wrapper for EntityFormDataProviderInterface used to dispatch hooks during the creation workflow
 */
class HookedEntityFormDataProvider implements EntityFormDataProviderInterface
{
    /**
     * @var EntityFormDataProviderInterface the wrapped $provider
     */
    private $provider;

    /**
     * @var HookDispatcherInterface the event dispatcher
     */
    protected $hookDispatcher;

    public function __construct(
        EntityFormDataProviderInterface $provider,
        HookDispatcherInterface $hookDispatcher
    ) {
        $this->provider = $provider;
        $this->hookDispatcher = $hookDispatcher;
    }

    public function getData(EntityIdentifierInterface $identifier = null)
    {
        $formData = array();
        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->entityName}PreFormData",
            [
                'identifier' => $identifier,
                'form_data' => &$formData,
            ]
        );

        $errors = $this->provider->saveData($formData, $identifier);

        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->entityName}PostFormData",
            [
                'identifier' => $identifier,
                'errors' => &$errors,
                'form_data' => &$data,
            ]
        );
    }

    public function saveData(array $data)
    {
        $type = $identifier ? 'Update' : 'Create';
        $formData = $form->getData();
        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->entityName}Pre${$type}",
            [
                'identifier' => $identifier,
                'form_data' => &$formData,
            ]
        );

        $errors = $this->provider->saveData($formData, $identifier);

        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->entityName}Post${$type}",
            [
                'identifier' => $identifier,
                'errors' => &$errors,
                'form_data' => &$data,
            ]
        );

        return count($errors) > 0 ? $errors : $identifier;
    }
}

This way the developer could focus on what his/her provider actually has to do, without thinking about the hooks, then in our core EntityFormHandler we would automatically wrap the provider:

class EntityFormHandler implements EntityFormHandlerInterface
{
    /**
     * @var HookDispatcherInterface
     */
    private $hookDispatcher;
     /**
     * @var FormFactoryInterface
     */
    private $formFactory;
     /**
     * @var string Fully qualified form type class name
     */
    private $entityFormType;
     /**
     * @var string
     */
    private $entityName;
     /**
     * @var EntityFormDataProviderInterface
     */
    private $entityFormDataProvider;

    public function __construct(
        HookDispatcherInterface $hookDispatcher,
        FormFactoryInterface $formFactory,
        EntityFormDataProviderInterface $entityFormDataProvider,
        $entityFormType,
        $entityName
    ) {
        if ($entityFormDataProvider instanceof HookedEntityFormDataProvider) {
            $this->entityFormDataProvider = $entityFormDataProvider;
        } else {
            $this->entityFormDataProvider = new HookedEntityFormDataProvider($provider, $hookDispatcher);
        }
        $this->hookDispatcher = $hookDispatcher;
        $this->formFactory = $formFactory;
        $this->entityFormType = $entityFormType;
        $this->entityName = $entityName;
    }    
}

[sarjon]

is someone willing to give it a go? @matks @jolelievre :) actually i was thinking about doing some UML so we are all on the same page and can spot issues before starting development, wdyt? :)

[matks]

I'm OK with everything in this Issue.
But we got some disagreements on the POC 😅 https://github.com/PrestaShop/PrestaShop/pull/10286

[sarjon]

regarding validation and form data i have another suggestion, what do you think about using custom data classes for forms instead of dealing with plain arrays? in addition to that, we could add validations for those data classes. In general, the idea is similar to this -> https://blog.martinhujer.cz/symfony-forms-with-request-objects/ @matks wdyt?

[matks]

This pattern (using DTOs for validation) is very good, however this means creating even more classes for forms. We already have FormFactories, FormDataProvides, FormDataHandlers. So even more objects to extend for a prestashop developer ...

[matks]

After a discussion with @eternoendless here are some feedbacks about this concept and the POC:

1) We should stop using the Entity word for class names or concepts as these forms and data handlers might be used to update something else than Entities
2) The EntityIdentifierInterface:: getIdentifier() method should return only strings to reduce possible complex/unreliable code as mixed returns usually means testing the return type to know what to do. We might introduce a getType() method and getRawIdentifier() (this one can have mixed returns). And we should stop calling them EntityIdentifiers 😁 to follow feedback 1.
3) Since these forms will then instantiate Commands, we can perform validation on Commands using Symfony validator on these Commands. The Command is responsible for carrying valid data. However there will always be (at least) 2 validation levels: 1st one is "I create a command with some data in it, is it valid ?" at this level we can perform validation rules and retrieve an array of errors ; 2nd one is data integrity and this can only be done when handling the command, if data integrity validation fails it will throw Exceptions. Some can be catched in the Controller to display a nice error message, other cannot.
4) We have to deal with the following usecases (see below):

These forms must be able to handle the following usecases:

• partial submits: for example in Product Page we might only submit a partial form and we should be able to handle it
• composite forms: a form might control the modification for several data models, this means having multiple EntityIdentifiers ?

[eternoendless]

I'd like to clarify something about (3).

We discussed about a two-tier validation system:

The first one is on the Controller side, powered by symfony validator if you will. It is responsible to prevent the end user from providing invalid data through the form and displaying a validation report containing all the errors in a user-understandable, localized, practical way. We could say that the role of this validation is to _"educate the user"_.

The second one is on the Command side. It should verify the validity of the provided data (eg. type, emptiness, etc), so that we can be sure that if a Command exists, it is valid. It should throw an exception on the first error, because the role of this check is to _"enforce validity"_. If there's one error, the whole data set is invalid.

And we could even add a third tier: on the Command Handler side. It should verify the integrity of the system (eg. an entity exists for the provided id, an object can be put in a certain state, etc) in order to avoid putting the Domain in an inconsistent state.

Some checks may have to be performed more than once, especially in the Controller and Command. This is not necessarily a bad thing, because as explained above, the Controller and the Command serve two different roles. In addition, a Command can be used without going through the Controller.

[eternoendless]

I made a schema to provide a better overview of the system:

[matks]

Here is a summary of the discussions which happened on gitter:

Interfaces

• Form Factories:

interface IdentifiableObjectFormFactoryInterface
{
  public function getForm(array $data, $id = null);
}

(EDITED: removed public function save(array $data, $id = null);

• Form Data Providers:

interface IdentifiableObjectFormDataProviderInterface
{
  public function getData($id = null);
}

• Form Data Handlers:

interface IdentifiableObjectFormDataHandlerInterface
{
  public function save(array $data, $id = null);
}

$id is a nontyped parameter in order to provides flexibility

[matks]

I believe we have enough material to start doing some CRUD pages. There might be some improvements later but we agreed on the main components so even if we change things later, the changes to be made to already-done-CRUD-pages will be minor.

[sarjon]

i have few concerns regarding that:

1. is IdentifiableObjectFormFactoryInterface really responsible for saving data?
2. where hooks are dispatched? for example saveDataHook and modifyFormHook?

[matks]

@sarjon for 1) no, the form factory does not save data, it must be a small mistake indeed 😄

[sarjon]

(EDITED: removed public function getForm(array $data, $id = null);

i think you should have kept this and removed save(array $data, $id = null) instead for form factory. :smile:

[matks]

@sarjon Argh 😅 yes indeed

[matks]

Status: https://github.com/PrestaShop/PrestaShop/pull/10974/files#r235738277 can be done
Other tasks require a few CRUD pages to be merged so we can have some experience about this system

[sarjon]

Closing as this was already implemented!

[matks]

@sarjon Do we have enough unit test coverage 🤔 ?