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

Implement form handlers for SqlManager form - part 1 #10974

Merged
merged 28 commits into from Nov 26, 2018

Conversation

@sarjon
Copy link
Member

sarjon commented Oct 11, 2018

Questions Answers
Branch? develop
Description? Introduces form handlers for SqlRequest by following #10285.
Type? new feature
Category? CO
BC breaks? no
Deprecations? no
Fixed ticket? #10285
How to test? Add and Edit actions for SqlManager should for the same.

This change is Reviewable

@sarjon

This comment has been minimized.

Copy link
Member

sarjon commented Oct 12, 2018

@matks i have few comments regarding forms:

  1. I tried using validators (see code) and the errors can be rendered like this https://prnt.sc/l56bwu so it looks much better than showing them all above the form.
  2. There are 3 hooks that should be executed for every form, so i think there should be some kind of wrapper around current interfaces so PrestaShop would be responsible for dispatching them (and not developer).

wdyt?

@matks matks added the migration label Oct 12, 2018

Show resolved Hide resolved src/Core/Form/IdentifiableObjectFormDataHandlerInterface.php Outdated
Show resolved Hide resolved src/Core/SqlManager/Form/SqlRequestFormDataHandler.php Outdated
$this->commandBus->handle($command);
$this->hookDispatcher->dispatchWithParameters('actionSqlRequestSave', [
'id' => $sqlRequestId,

This comment has been minimized.

@matks

matks Oct 15, 2018

Contributor

Do you dispatch it even if it was null ?

This comment has been minimized.

@matks

matks Oct 15, 2018

Contributor

Or do you dispatch the $id once created 😛 ?

This comment has been minimized.

@sarjon

sarjon Oct 16, 2018

Member

its dispatched either way, as both save and edit commands are executed above, we can change it if needed. :)

*/
private function getConstraintError(SqlRequestConstraintException $e)
{
$invalidFieldDictionary = [

This comment has been minimized.

@matks

matks Oct 15, 2018

Contributor

👍

*/
private function getErrorByExceptionType(SqlRequestException $e)
{
$exceptionDictionary = [

This comment has been minimized.

@matks

matks Oct 15, 2018

Contributor

If this list gets long, maybe we can have a dedicated class for it ? Something like a SqlRequestFormDataHandlerExceptionUtils or SqlRequestExceptionsregistry ?

This comment has been minimized.

@sarjon

sarjon Oct 16, 2018

Member

i dont really know if @eternoendless would accept that. :/

This comment has been minimized.

@jolelievre

jolelievre Nov 12, 2018

Contributor

I actually don't think returning an $errors array is goo practice
If I get it right this means if the creation went well you return the ID, but if there are errors you return an array?
but if it's an update and everything went fine you return neither? or an empty array?
it would be cleaner to simply throw exceptions and manage them elsewhere, this clarifies the behavior of the interface
Besides managing the Exception transformation in this class gives it too many responsibilities

HookDispatcherInterface $hookDispatcher,
Request $request = null
) {
$this->request = $request;

This comment has been minimized.

@matks

matks Oct 15, 2018

Contributor

🤔 mmmm ... injecting the request as a service should be kept for very exotic usecases. Can we avoid it here ?

This comment has been minimized.

@jolelievre

jolelievre Nov 12, 2018

Contributor

agreed, Request should not be injected You should rather extract the info into an array beforehand

Show resolved Hide resolved src/Core/SqlManager/Form/SqlRequestFormDataProvider.php Outdated
@matks

This comment has been minimized.

Copy link
Contributor

matks commented Oct 16, 2018

I tried using validators (see code) and the errors can be rendered like this https://prnt.sc/l56bwu so it looks much better than showing them all above the form.
There are 3 hooks that should be executed for every form, so i think there should be some kind of wrapper around current interfaces so PrestaShop would be responsible for dispatching them (and not developer).

Although I cant see the screenshot I will say "yes 👍 " and "yes 👍 " !

By the way you can attach screenshots to github comments 😉

@sarjon

This comment has been minimized.

Copy link
Member

sarjon commented Oct 17, 2018

By the way you can attach screenshots to github comments

yeah, but im lazy. :/

Errors before:
before

Errors after:
after

@matks
Copy link
Contributor

matks left a comment

2nd review :)
Let's handle the "request as a service dependency" debate later. We need to focus on Form Entity interfaces here so we can start migrating other CRUD pages.

'sql' => $editableRequestSql->getSql(),
];
} catch (SqlRequestException $e) {
return [];

This comment has been minimized.

@matks

matks Oct 19, 2018

Contributor

Is that the expected behavior ? If the SQL request fail, we show the form with no data ?

/**
* Class SqlRequestFormFactory creates form used in Back Office for adding/editing SqlRequests.
*/
final class SqlRequestFormFactory implements IdentifiableObjectFormFactoryInterface

This comment has been minimized.

@matks

matks Oct 19, 2018

Contributor

This class looks like PrestaShop\PrestaShop\Core\Form\FormHandler, but not generic. Do you think we can make a generic version for CRUD pages ?

This comment has been minimized.

@sarjon

sarjon Oct 23, 2018

Member

yes, i want to make it more generic as well, but i still didnt figure out what would be the best way to configure it (define form type, add form options, set form data). :/

This comment has been minimized.

@matks

matks Nov 9, 2018

Contributor

What I suggest: we merge this, migrate another CRUD page and compare the FormFactories. If we can, we merge the 2 FormFactories into 1 generic class. Wdyt ?

This comment has been minimized.

@jolelievre

jolelievre Nov 12, 2018

Contributor

the form types worked fine for FormHandler, and its Symfony like, why not use them right now?

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 8, 2018

@sarjon 🆙 😉 there are unhandled code review feedbacks here and unanswered issues

@jolelievre
Copy link
Contributor

jolelievre left a comment

great job @sarjon
but, you're not gonna like me ^^, I have a lot of feedback..

Show resolved Hide resolved src/Core/Form/IdentifiableObjectFormDataHandlerInterface.php Outdated
Show resolved Hide resolved src/Core/Form/IdentifiableObjectFormFactoryInterface.php Outdated
Show resolved Hide resolved src/Core/Form/IdentifiableObjectFormDataProviderInterface.php Outdated
Show resolved Hide resolved src/Core/Form/IdentifiableObjectFormFactoryInterface.php Outdated
Show resolved Hide resolved src/Core/SqlManager/Form/SqlRequestFormDataHandler.php Outdated
Show resolved Hide resolved src/Core/SqlManager/Form/SqlRequestFormFactory.php Outdated
Show resolved Hide resolved src/PrestaShopBundle/Resources/config/services/bundle/form/form_type.yml
arguments:
- '@prestashop.core.query_bus'
- '@prestashop.core.hook.dispatcher'
- '@=service("request_stack").getMasterRequest()'

This comment has been minimized.

@jolelievre

jolelievre Nov 12, 2018

Contributor

mmm, I don't like this very much ^^

This comment has been minimized.

@sarjon

sarjon Nov 13, 2018

Member

me neither. :)

Show resolved Hide resolved ...e/Controller/Admin/Configure/AdvancedParameters/SqlManagerController.php Outdated
Show resolved Hide resolved ...urces/views/Admin/Configure/AdvancedParameters/RequestSql/form.html.twig Outdated
private function handleForm(FormInterface $form, $id = null)
{
if (!$form->isSubmitted()) {
return 0;

This comment has been minimized.

@sarjon

sarjon Nov 13, 2018

Member

im still trying to figure out a better return value, any suggestions?

This comment has been minimized.

@matks

matks Nov 13, 2018

Contributor

I guess you mean "what should I return if there are errors in the form ?" ?

Ideas:

  • return 0, but as a constant so we can use some test like $result = $handler->handle($form); if ($result === FormHandlerInterface::ERRORS_FOUND) {...}
  • return a dedicated exception CannotHandleFormException which contains the array of errors, and catch it in the controller
  • return a FormHandleResult value object with a $hasErrors property or even a hasErrors() functiton

This comment has been minimized.

@sarjon

sarjon Nov 13, 2018

Member

actually if form is not submitted, its fine, we just skip its handling (since handle() is called for both GET and POST methods). throwing exception is also not a best idea i think, as no unexpected situation or anything wrong happened here. so im thinking about some VO, wdyt?

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

even though I'm not a big fan, maybe a FormHandleResult would do the job, this the only way I see to be able to return all the informations: success|invalid|skipped|errors
the only other way would for the FormHandler to keep a reference on the form and have methods to return those values isValid, isSubmitted, ... But it is already accessible vie the $form object 🧐
(sorry I'm thinking out loud 😄)

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

actually if the only job of the FormHandler was to handle the form submit/validation it would be easier, but we would need to change its prototypes a bit:

     /**
       * @var array $data is an array, this way we don't bind it to the Request object (it' the controller job to extract the request params, or another object in case of too complicated transformations)
       * @return bool
      **/
     public function handleForm(FormInterface $form, array $data)
     {
             //Hook to modify the data before setting it to the form
             $form->submit($data);
             //Check demo mode
             //Check isSubmit
             //Check isValid
             //return false when not valid
            
             //If all is good then we call the form creation/update
             //return true if all is good
             //the errors are stored in the Form as it is done here
     }

This comment has been minimized.

@sarjon

sarjon Nov 14, 2018

Member

i had similar idea, maybe even null? so it could get typehinted at somepoint with ?int, wdyt?

This comment has been minimized.

@jolelievre

jolelievre Nov 14, 2018

Contributor

yep null is fine for me to
but what do mean by ?int

This comment has been minimized.

@sarjon

sarjon Nov 14, 2018

Member

its for php 7.1+, public function handle(FormInterface $form): ?int;

This comment has been minimized.

@jolelievre

jolelievre Nov 14, 2018

Contributor

ok, I didn't know it!
too bad it's not available in 7.0

This comment has been minimized.

@matks

matks Nov 19, 2018

Contributor

OK for integer or Value Object 😉

actually if form is not submitted

We had a bug once in the Product page because form was not submitted 🤔 ... so let's keep this as it is now, but be wary of it when we start using it

@matks
Copy link
Contributor

matks left a comment

Looks very good ! 😃

/**
* {@inheritdoc}
*/
public function getForm(array $options = [])

This comment has been minimized.

@matks

matks Nov 13, 2018

Contributor

Could we inject $name and $sql parameters, coming from the request if the "Export to SQL Manager feature is used", here as an option ? That would allow to remove $request from the constructor.

So code would be:

    public function getForm(array $options = [])
    {
        if (isset($options['there_is_some_default_data'])) {
            return $this->buildForm(
                $options['there_is_some_default_data'],
                null,
                $options
            );
        } else {
            return $this->buildForm(
                $this->dataProvider->getDefaultData(),
                null,
                $options
            );
        }

        
    }

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

it forces to implement edge cases in a generic class, which is supposed to be the cleanest possible
less worst idea, imo, would be to allow to preset defaultData on SqlRequestFormDataProvider, be cause this is the exception use case, so the exception should be implement in the part of the code that has particularities
this avoid twisting the generic code which work for 80% of the cases

This comment has been minimized.

@sarjon

sarjon Nov 14, 2018

Member

what is more, $options are suppose to be to configure FormType, so adding additional parameters would break it.

{
if ($this->request && $this->request->request->has('sql')) {
return [
'sql' => $this->request->request->get('sql'),

This comment has been minimized.

@matks

matks Nov 13, 2018

Contributor

OK now I understand why you inject $request in the constructor 😄 .
I'm still worried about it. See suggestion at https://github.com/PrestaShop/PrestaShop/pull/10974/files#diff-6016826a34f6a8d8ff77712f2a1bf4eaR107

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

I'm still not fan of injecting the request
at some point we thought about having getFormFor(array $data) so the input could be an array with the ID or some preset data

This comment has been minimized.

@mickaelandrieu

mickaelandrieu Nov 19, 2018

Contributor

We should inject $this->request->request->get('sql') directly so the injected stuff should be a string named $sqlQuery.

*
* @return FormInterface
*/
private function buildForm($data, $id = null, array $options = [])

This comment has been minimized.

@matks

matks Nov 13, 2018

Contributor

👍

$this->hookDispatcher->dispatchWithParameters('action' . $formBuilder->getName() . 'FormBuilderModifier', [
'form_builder' => $formBuilder,
'data' => &$data,
'id' => $id,

This comment has been minimized.

@matks

matks Nov 13, 2018

Contributor

Is it OK to pass null in a creation context ? Not weird ?

This comment has been minimized.

@sarjon

sarjon Nov 13, 2018

Member

just to be consistent, since this method is used to create form for both create and update actions, i think its okay to pass it as a null as well. it would allow developer to easily check whether it's a create or update action. if we didnt pass it, developer would have to check it by isset($params['id'] method, so i prefer having same parameters all the time, wdyt?

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

another solution would be to have two hooks, one for creation and the other for update

This comment has been minimized.

@sarjon

sarjon Nov 14, 2018

Member

is it really worth it? the only difference would be just id param and developer would be forced to register 2 hooks all the time. :/

This comment has been minimized.

@jolelievre

jolelievre Nov 14, 2018

Contributor

you're right it does not bring much to have two hooks
also good idea to systematically return the id field

private function handleForm(FormInterface $form, $id = null)
{
if (!$form->isSubmitted()) {
return 0;

This comment has been minimized.

@matks

matks Nov 13, 2018

Contributor

I guess you mean "what should I return if there are errors in the form ?" ?

Ideas:

  • return 0, but as a constant so we can use some test like $result = $handler->handle($form); if ($result === FormHandlerInterface::ERRORS_FOUND) {...}
  • return a dedicated exception CannotHandleFormException which contains the array of errors, and catch it in the controller
  • return a FormHandleResult value object with a $hasErrors property or even a hasErrors() functiton
Show resolved Hide resolved ...e/Controller/Admin/Configure/AdvancedParameters/SqlManagerController.php Outdated
* "is_granted(['delete'], request.get('_legacy_controller')~'_')",
* message="You do not have permission to edit this."
* "is_granted(['delete'], request.get('_legacy_controller'))",
* message="You do not have permission to edit this.",

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 13, 2018

Contributor

to delete

* "is_granted(['delete'], request.get('_legacy_controller')~'_')",
* message="You do not have permission to edit this."
* "is_granted(['delete'], request.get('_legacy_controller'))",
* message="You do not have permission to edit this.",

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 13, 2018

Contributor

to delete?

'message' => $this->trans(
'The %s field is required.',
[
sprintf('"%s"', $this->trans('SQL query name', [], 'Admin.Advparameters.Feature')),

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 13, 2018

Contributor

sprintf is useless here.

$this->trans(
    'The "%s" field is required.',
    [
        $this->trans('SQL query name', [], 'Admin.Advparameters.Feature'),
    ],
    'Admin.Notifications.Error'
),

Same below

This comment has been minimized.

@sarjon

sarjon Nov 14, 2018

Member

we would lose translation if we did that. :/

{% import '@PrestaShop/Admin/macros.html.twig' as ps %}

{{ form_start(requestSqlForm) }}
<div class="card">

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 13, 2018

Contributor

Can you add some block here? :)

@@ -0,0 +1,54 @@
{#**
* 2007-2018 PrestaShop

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 13, 2018

Contributor

wrong indent

This comment has been minimized.

@sarjon

sarjon Nov 14, 2018

Member

are you sure? i dont see it.

@@ -87,3 +87,21 @@
{{ label|trans({}, 'Admin.Advparameters.Feature') }}
</a>
{% endmacro %}

{# Show form widget with errors rendered below it #}
{% macro form_widget_with_error(form, vars) %}

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 13, 2018

Contributor

use native form_row or form_widget & form_errors??

This comment has been minimized.

@sarjon

sarjon Nov 13, 2018

Member

if i used it, it works, but it looks ugly. :/

This comment has been minimized.

@PierreRambaud

PierreRambaud Nov 13, 2018

Contributor

We maybe need to improve the default style, it will be easier to everyone to use what twig & symfony allow us to used :)

This comment has been minimized.

@sarjon

sarjon Nov 14, 2018

Member

thats an option. :)

}
if ($this->isDemoModeEnabled) {
$form->addError(

This comment has been minimized.

@sarjon

sarjon Nov 13, 2018

Member

instead of adding error directly in form, we could add it to FlashBag although im not sure about that solution, wdyt?

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

the FlashBag is good for controllers in a CRUD page, but in an API you would not use it
so I think it is the responsibility of the controller to fill the FlashBag not the the FormHandler

This comment has been minimized.

@matks

matks Nov 19, 2018

Contributor

@jolelievre is right, FlashBag it is up to the controllers to choose how errors should be displayed: as Twig rendered blocks or as flash messages in the session.

@jolelievre
Copy link
Contributor

jolelievre left a comment

the beginning of my review ^^

if ($this->configuration->get('_PS_MODE_DEMO_')) {
$form->addError(
new FormError(
$this->translator->trans('This functionality has been disabled.', [], 'Admin.Notifications.Error')

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

seems like the translator service is only used here?
is it really necessary to inject it then since the translation could be managed in the template?
I believe in symfony form errors, the errors contain the keys and are then translated in the views

@@ -800,7 +800,7 @@ public function getAdminLink($controller, $withToken = true, $sfRouteParams = ar
'AdminPaymentPreferences' => 'admin_payment_preferences',
'AdminInvoices' => 'admin_order_invoices',
'AdminEmails' => 'admin_email',
'AdminRequestSql' => 'admin_sql_request',
'AdminRequestSql' => 'admin_sql_requests_index',

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

this array should not exist any more, you need to rebase your branch with the 1.7.5 modifications (which have been merge in develop)
and you should use _legacy_link instead (https://devdocs.prestashop.com/1.7/development/architecture/migration-guide/controller-routing/#more-about-the-legacy-link-property)

This comment has been minimized.

@sarjon

sarjon Nov 21, 2018

Member

ill rebase after code review is done. 👍

Show resolved Hide resolved src/Core/Form/IdentifiableObject/DataHandler/FormDataHandlerInterface.php
*
* @param int $id
*
* @return mixed

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

isn't it an array?

This comment has been minimized.

@sarjon

sarjon Nov 14, 2018

Member

forms accepts any type of data, it can be array, null, object & etc. so i didnt want to limit it to arrays only, wdyt?

This comment has been minimized.

@jolelievre

jolelievre Nov 14, 2018

Contributor

yes I thought so too, I'm hesitating should we "force" an array in order to guide the developers
if you we start using array in some place, objects in others this could get ugly

{
if ($this->request && $this->request->request->has('sql')) {
return [
'sql' => $this->request->request->get('sql'),

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

I'm still not fan of injecting the request
at some point we thought about having getFormFor(array $data) so the input could be an array with the ID or some preset data

Show resolved Hide resolved ...e/Controller/Admin/Configure/AdvancedParameters/SqlManagerController.php Outdated
}
if ($this->isDemoModeEnabled) {
$form->addError(

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

the FlashBag is good for controllers in a CRUD page, but in an API you would not use it
so I think it is the responsibility of the controller to fill the FlashBag not the the FormHandler

private function handleForm(FormInterface $form, $id = null)
{
if (!$form->isSubmitted()) {
return 0;

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

even though I'm not a big fan, maybe a FormHandleResult would do the job, this the only way I see to be able to return all the informations: success|invalid|skipped|errors
the only other way would for the FormHandler to keep a reference on the form and have methods to return those values isValid, isSubmitted, ... But it is already accessible vie the $form object 🧐
(sorry I'm thinking out loud 😄)

private function handleForm(FormInterface $form, $id = null)
{
if (!$form->isSubmitted()) {
return 0;

This comment has been minimized.

@jolelievre

jolelievre Nov 13, 2018

Contributor

actually if the only job of the FormHandler was to handle the form submit/validation it would be easier, but we would need to change its prototypes a bit:

     /**
       * @var array $data is an array, this way we don't bind it to the Request object (it' the controller job to extract the request params, or another object in case of too complicated transformations)
       * @return bool
      **/
     public function handleForm(FormInterface $form, array $data)
     {
             //Hook to modify the data before setting it to the form
             $form->submit($data);
             //Check demo mode
             //Check isSubmit
             //Check isValid
             //return false when not valid
            
             //If all is good then we call the form creation/update
             //return true if all is good
             //the errors are stored in the Form as it is done here
     }
Show resolved Hide resolved ...e/Controller/Admin/Configure/AdvancedParameters/SqlManagerController.php Outdated
@eternoendless
Copy link
Member

eternoendless left a comment

Reviewed 2 of 11 files at r1, 28 of 32 files at r2, 2 of 2 files at r4.
Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @matks, @jolelievre, @sarjon, and @PierreRambaud)


src/Core/Form/IdentifiableObject/FormHandler.php, line 173 at r2 (raw file):

Previously, jolelievre wrote…

ok, I didn't know it!
too bad it's not available in 7.0

Think CQS:

"Every method should either be a command that performs an action, or a query that returns data to the caller, but not both."

This method performs something, and consequently it should return nothing. If we want to retrieve the created id, then it should be a separate method, like PDO does.


src/Core/Form/IdentifiableObject/FormHandler.php, line 40 at r3 (raw file):

Previously, jolelievre wrote…

I'm thinking that this class/interface may have too many responsibilities, it builds the form and it handles the form
Maybe we should split it into FormHandlerInterface and FormBuilderInterface, and maybe that's why it seemed so logical (once you proposed it @sarjon) to have a factory, because maybe the the factory should handle the full build of the form and the the management of the request here

I agree, building and handling are two different responsibilities


src/Core/Form/IdentifiableObject/FormHandlerInterface.php, line 32 at r4 (raw file):


/**
 * Interface FormHandlerInterface

"Interface for classes that build forms and handle when that form is submitted" -> there's an AND there, too many responsibilities.


src/Core/Form/IdentifiableObject/FormHandlerInterface.php, line 39 at r4 (raw file):

     * Get form for object creation with default data.
     *
     * @param array $options

What are the options?


src/Core/Form/IdentifiableObject/DataHandler/SqlRequestFormDataHandler.php, line 35 at r4 (raw file):


/**
 * Class SqlRequestFormDataHandler creates or updates SqlRequest with form data.

No need to repeat the name of the class. Just "Creates or updates SqlRequest objects using form data"


src/Core/Form/IdentifiableObject/DataProvider/SqlRequestFormDataProvider.php, line 90 at r3 (raw file):

Previously, jolelievre wrote…

I'm still not fan of injecting the request
at some point we thought about having getFormFor(array $data) so the input could be an array with the ID or some preset data

+1 this information should be injected some other way.


src/Core/Form/IdentifiableObject/DataProvider/SqlRequestFormDataProvider.php, line 35 at r4 (raw file):


/**
 * Class SqlRequestFormDataProvider

"Provides data to fill out SqlRequest forms"


src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/SqlRequestType.php, line 55 at r3 (raw file):

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

we would lose translation if we did that. :/

I think the translation will have to be changed to add the double quotes. The quote symbol is not the same across languages and it needs to be localized too. Wdyt @LouiseBonnard ?


src/PrestaShopBundle/Resources/views/Admin/macros.html.twig, line 92 at r3 (raw file):

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

thats an option. :)

How can we change the default?


src/PrestaShopBundle/Resources/views/Admin/Configure/AdvancedParameters/RequestSql/create.html.twig, line 53 at r4 (raw file):

  <script src="{{ asset('themes/new-theme/public/sql_manager.bundle.js') }}"></script>
  <script src="{{ asset('themes/default/js/bundle/pagination.js') }}"></script>

This js should be moved to the new theme


src/Core/Form/IdentifiableObject/FormHandlerFactoryInterface.php, line 33 at r4 (raw file):


/**
 * Interface FormHandlerFactoryInterface

Please don't forget to describe your classes/interfaces. Describing is a good exercise because it forces you to think what the class/interface is responsible for. If you find yourself having to use the and word, it's probably because your class/interface does too many things.

In this case: "Interface for classes that create FormHandlers" (ok this one was too easy)


src/Core/Form/IdentifiableObject/DataProvider/FormDataProviderInterface.php, line 30 at r4 (raw file):


/**
 * Interface IdentifiableObjectFormDataProviderInterface.

"Interface for classes that provide data to fill out forms"

@eternoendless
Copy link
Member

eternoendless left a comment

Reviewed 1 of 32 files at r2.
Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @matks, @jolelievre, @sarjon, and @PierreRambaud)

@eternoendless
Copy link
Member

eternoendless left a comment

Reviewed 1 of 32 files at r2, 1 of 1 files at r3.
Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @matks, @jolelievre, @sarjon, and @PierreRambaud)

@sarjon

This comment has been minimized.

Copy link
Member

sarjon commented Nov 15, 2018


src/Core/Form/IdentifiableObject/FormHandler.php, line 173 at r2 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

Think CQS:

"Every method should either be a command that performs an action, or a query that returns data to the caller, but not both."

This method performs something, and consequently it should return nothing. If we want to retrieve the created id, then it should be a separate method, like PDO does.

so what your are saying is: once object is created from form data, client does not need to be provided with newly created object's id?

@LouiseBonnard
Copy link
Contributor

LouiseBonnard left a comment

Reviewable status: all files reviewed, 43 unresolved discussions (waiting on @matks, @jolelievre, @sarjon, @eternoendless, and @PierreRambaud)


src/PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/RequestSql/SqlRequestType.php, line 55 at r3 (raw file):

Previously, eternoendless (Pablo Borowicz) wrote…

I think the translation will have to be changed to add the double quotes. The quote symbol is not the same across languages and it needs to be localized too. Wdyt @LouiseBonnard ?

It should remain without quotes: The %s field is required.

/**
* Class FormHandler
*/
final class FormHandler implements FormHandlerInterface

This comment has been minimized.

@matks

matks Nov 19, 2018

Contributor

After reading again, yes @jolelievre is right 😢
I'm sad because I'm worried (and @eternoendless is worried too) we are making quite a complex system for CRUD pages (complex in comparison of what was required by legacy controllers). But this class indeed does too many things.

private function handleForm(FormInterface $form, $id = null)
{
if (!$form->isSubmitted()) {
return 0;

This comment has been minimized.

@matks

matks Nov 19, 2018

Contributor

OK for integer or Value Object 😉

actually if form is not submitted

We had a bug once in the Product page because form was not submitted 🤔 ... so let's keep this as it is now, but be wary of it when we start using it

}
if ($this->isDemoModeEnabled) {
$form->addError(

This comment has been minimized.

@matks

matks Nov 19, 2018

Contributor

@jolelievre is right, FlashBag it is up to the controllers to choose how errors should be displayed: as Twig rendered blocks or as flash messages in the session.

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 19, 2018


src/Core/Form/IdentifiableObject/FormHandler.php, line 173 at r2 (raw file):

Previously, matks (Mathieu Ferment) wrote…

OK for integer or Value Object 😉

actually if form is not submitted

We had a bug once in the Product page because form was not submitted 🤔 ... so let's keep this as it is now, but be wary of it when we start using it

About what Pablo said: CQRS expects us to provide the unique IDs for our entities. So usecase is "you want to create entity A ? Dispatch a CreateEntityA command that requires you to provide an ID (usually a UUID) and it will be persisted. However the PrestaShop project relies on mysql IDs creation rather than using its own IDs. So I think it cannot apply here :(

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 19, 2018

@sarjon It looks to me that we have 3 main issues (along some minor feedbacks):

  • split the FormHandler into a Form Handler and a Form Builder
  • handle the errors reporting
  • @jolelievre and @eternoendless seem to agree with me: we are really concerned about Request being injected as a constructor argument. We need to find another way

@sarjon sarjon force-pushed the sarjon:introduce-form-handlers branch from f63f343 to 7bf36c4 Nov 26, 2018

@matks

matks approved these changes Nov 26, 2018

@matks

This comment has been minimized.

Copy link
Contributor

matks commented Nov 26, 2018

Thanks @sarjon

@matks matks merged commit 0fa9f23 into PrestaShop:develop Nov 26, 2018

1 of 2 checks passed

code-review/reviewable 28 files, 56 discussions left (eternoendless, jolelievre, matks, PierreRambaud, sarjon)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@matks matks changed the title [WIP] Implement form handlers for SqlManager form Implement form handlers for SqlManager form - part 1 Nov 26, 2018

@matks matks added this to the 1.7.6.0 milestone Nov 26, 2018

@sarjon sarjon deleted the sarjon:introduce-form-handlers branch Nov 26, 2018

@sarjon sarjon referenced this pull request Dec 12, 2018

Open

[SF migration] Templates and actions naming convention #10597

2 of 3 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment