Issues when creating new module for PS 1.7.6

[tomas862]

Summary

Needs to be done for module release and 1.7.6.0 release

• How to handle errors happening inside module (translation + display to user) Issues when creating new module for PS 1.7.6 #13490 ✅ fixed by Introduce the usage of Module user-displayable exceptions to handle module errors (part 2) #14239
• Translation is not working in a module Translation is not working in a module #13942 ✅ fixed in the module
• Make symfony forms easier to extend from modules using form_rest #12585 ✅ fixed by Added help property (backported from Symfony 4 framework) #13875
• Add form_data to after update hooks #13944 🚧 ✅ fixed by Adds missing form data arguments in hooks #14008

Can be done after module release (as an improvement)

• How to add a JS extension for your modern module How to add a JS extension for your modern module #13943

Description
I recently created a sample module which uses CQRS and new hooks in its own system and I found several things which a worth to be investigated and fixed:

Link to the module

https://github.com/friends-of-prestashop/demo-cqrs-hooks-usage-module

Problems

1. Can't make the translations to work (transferred to Translation is not working in a module #13942)

• according to this https://github.com/PrestaShop/PrestaShop/blob/develop/tests/Resources/modules/translations/src/Controller/Admin/ExempleController.php#L36

• I should be able to have working transaltions from my module controller but they just don't work here https://github.com/friends-of-prestashop/demo-cqrs-hooks-usage-module/blob/master/src/Controller/Admin/CustomerReviewController.php#L54 . But it works in Translations module

• for both Translations module and my sample module the translations does not work in main module file as well https://github.com/friends-of-prestashop/demo-cqrs-hooks-usage-module/blob/master/ps_democqrshooksusage.php#L68

2. Using JS extension with module (transferred to How to add a JS extension for your modern module #13943)

I have added ToggleColumn in Customers list using the module and it works like a charm. ToggleColumn required javascript extension added to the grid new ColumnTogglingExtension(). To my luck, Customers list has this extension but developers might face the issues when they add column but the list does not have this extension in javascript compiled.

Solution:

• After creating sample doc about module creation we should create a sub-doc with advanced use cases such as adding extension from module - this will display webpack configuration, using hook to add your new javascript to existing list etc...

3. Bad default rendering for added fields (related to Make symfony forms easier to extend from modules using form_rest #12585)

Currently if I add custom form element to the form it is misaligned and I can't make it look consistent like other elements in the form.

as seen in the image my custom switch is in wrong position and the label is incorrect and if my switch would have a help message it would be impossible to display it. It is due to form_rest is rendered and it just adds missing form elements where the form_rest is used.

Solution:

I wish we could make this #13412 appear in 1.7.6 so it will allow us to control rendered html. The only thing the core developers will only need to remember to put form_rest in

next to other form fields so it will render in consistent place

4. Missing useful data in hook (transferred to Add form_data to after update hooks #13944)

When form is submitted and after the main Customer form logic is passed the hook hookactionAfterUpdatecustomerFormHandler is called. In FormHandler actionBeforeUpdate... hook has parameter form_data but actionAfterUpdate does not have anymore. This forces me to access Request in my hook https://github.com/friends-of-prestashop/demo-cqrs-hooks-usage-module/blob/master/ps_democqrshooksusage.php#L245

Solution:
lets just add form_data to actionAfter hooks in form handlers

5. Error handling - when my command fails I raise an exception and catch in my hook and assign user friendly message and finally add the message to the FlashBag.

The problem:

the error happened in module scope but customer data was updated correctly - its weird that both success and error messages are displayed

Solutions:

• In module I could extend CustomerException which is being catched in Customer controller but it will lead to 2 error messages : the first will be mine from module while the second be the fallback error message used in the controllers

• clearing the flashBag messages from module?

• maybe its good after all. Maybe I only need to prefix a message with module name so the client will know that the message has been raised from the module. Maybe even we can automate this by having some kind of Module exception which is caught in certain part of symfony event cycle and it would prepend error, warning, info messages with module names.

[tomas862]

@matks let me know what you think about problems and solutions. Maybe you have some other suggestions?

[mickaelandrieu]

such as adding extension from module

AFAIK @sarjon was asked to document that in august, 2018

Currently if I add custom form element to the form it is misaligned and I can't make it look consistent like other elements in the form.

It's part of form theming improvement initiative started by @sarjon (what a man!), we're aware of that since 1.7.3 but we couldn't manage to prioritize this need in 1.7.6 branch... it's not too late, though as this is clearly a bug for me.

Currently if I add custom form element to the form it is misaligned and I can't make it look consistent like other elements in the form.

I'm not really in favor of using a php hook for rendering while the Form component already provides Form theming system, templates overriding and all that stuff. Sounds like we introduce bad practices to avoid to fix the real issue: the current form theme is incomplete and/or buggy.

lets just add form_data to actionAfter hooks in form handlers

Yup, it's a nice idea (see, I'm not ranting about all here 💃 )

the error happened in module scope but customer data was updated correctly - its weird that both success and error messages are displayed

Agree, but for me, it's not a flash bag issue but a "worflow" issue. Let's fix it at the root level instead of trying to hack the flashbag object: don't you think? If an exception is raised, the update shouldn't be done at all. And if you catch it in your hook, you should dispatch it after imho.

[tomas862]

@mickaelandrieu

Agree, but for me, it's not a flash bag issue but a "worflow" issue. Let's fix it at the root level instead of trying to hack the flashbag object: don't you think? If an exception is raised, the update shouldn't be done at all. And if you catch it in your hook, you should dispatch it after imho.

if I throw my custom exception MyModuleException it won't be catched in the CurrencyController in this case. The only thing I have control of is to throw CurrencyException which is expected in the controller. However as a module developer I would like to control of the messages that something went wrong and the user would see that the problem were caused from module and not from the core. Do you suggest not using flashbag object from the module? 🤔

[tomas862]

🏓 @mickaelandrieu 🏓

[mickaelandrieu]

Do you suggest not using flashbag object from the module?

Yes, I prefer let the core transform an exception into a user message. How about create a specific exception for that and an exception listener able to transform the exception (no matter where it's thrown) to a well formatted user message displayed where it's supposed to be displayed?

[sarjon]

Yes, I prefer let the core transform an exception into a user message.

I don't think it's possible. What if you have custom use case where Currency is associated with Product and then you throw fictional CurrencyAssociationException, in this case Core will not know how to translate exception to user friendly message except for Unexpected error occurred which I don't think is user friendly.

[mickaelandrieu]

I don't think it's possible.

Do you mean it's impossible to throw an exception in your module with a meaningful message but we should be able to do it populating a flashbag message?

In Symfony you can catch specific exception types:

// in your module

...
try {
    // whatever
} catch(WhateverException $e) {
if (usermessageIsNeeded) {
    throw new UserMessageException('What a meaningful message we got, such WOW!');
}
}

Then we hook in every environments using an exception listener and catch the UserMessageException exception, and then we populate the flashbag with error messages.

It works for this need, it works for every needs: a common need => a common and clean solution 👼

[sarjon]

It works for this need, it works for every needs: a common need => a common and clean solution

Well, that would definitely work, but I could argue about it. 😈

I think that exceptions should not carry user friendly message, but rather technical error that is useful for developer. In case of exception in production, it should be logged for developer and converted into user friendly message depending on exception type/error code.

The other reason why exception messages should not be meant for end user is because of translation. If you were to throw exception in your sevice, it would force your service depend on translator, doesn't sound like a good object design to me.

Wdyt?

[kpodemski]

I think that exceptions should not carry user friendly message, but rather technical error that is useful for developer.

THIS

We should be able to control user-friendly messages of all types, legacy AdminController was always aware of content of $this->errors etc.

[PierreRambaud]

The other reason why exception messages should not be meant for end user is because of translation. If you were to throw exception in your sevice, it would force your service depend on translator, doesn't sound like a good object design to me.

Agree 💯 This is why we sent a message for debug, to get information in syslog or anything else, whereas we sent a message to the user to inform him for something he can understand :)

[tomas862]

So right now from your comments I see that there are two possible scenarios:

1. Throw exception with translatable message from module hook and not handle it and let the page go down for regular user
2. Handle the exception in the module hook and add friendly message in the flashbag

which one do you guys prefer or do you have more suggestions? 🤔

[matks]

As the 5th item of this bug list has become a topic of debate, I'm going to open 4 other issues for the 4 other items. Discussion can continue about error messages here.

EDIT:

• Translation is not working in a module #13942
• How to add a JS extension for your modern module #13943
• Make symfony forms easier to extend from modules using form_rest #12585
• Add form_data to after update hooks #13944

[kpodemski]

Handle the exception in the module hook and add friendly message in the flashbag

this seems to be better solution

[matks]

OK sorry for late answer.

Initial issue

When custom action (= provided by module) fails, with current implementation,

• if module throws an Exception, we get a generic error message
• if module writes an error message in flashbag but does not throw the exception, then get the nice error message BUT we also get a success flash message from Controller which did not detect there was an issue (as the mean to detect it is exceptions)

[matks]

Solution we have come up with

We will promote the following ways for modules to dispatch errors:

1) Full control of the error flow

• Add in flashbag the errors to be displayed (the module controls the displayed string so they can translate it if they want)
• Throw a ModuleException: this will prevent further execution of the controller and prevent the controller from adding a success error message into the flashbag

If module wants to display multiple error messages, he can do it using this usecase.

2) Simpler error handling usecase

• Throw a ModuleException

This is a lot simpler but allows only 1 error message to be displayed. Also we dont expect module developers to translate the ModuleException message so it will probably English.

PrestaShop Core will detect that ModuleException has been thrown and check whether the flashbag is empty. If yes, then it will add the ModuleException message into the flashbag. This acts as a fallback to make sure user has at least 1 information message.

---

The previous processing will be performed by a Symfony listener. This will remove the need to perform it in every controller action.

@kpodemski @mickaelandrieu How does it look ?

[tomas862]

I would choose number 1 which is Full control of the error flow but I thought about it and realized that some parts just won't work. So here's in my opinion the only possible solution:

• Add in flashbag the errors to be displayed (the module controls the displayed string so they can translate it if they want)
• Throw a ModuleException: this will prevent further execution of the controller and prevent the controller from adding a success error message into the flashbag
  so for this part the highlighted part is very important - there's no easy way to do it without all our migrated controller action modifications. Even if we use symfony exception listener we need to clear success messages from flashbag and there's no way to identify which success message to clear and which to keep - this can cause issues when we clear too much. So I suggest from this part to do like this:
  • all our controller actions must catch DomainException
  • ModuleException extends DomainException
  • All our migrated controllers must use \PrestaShopBundle\Controller\Admin\FrameworkBundleAdminController::getFallbackErrorMessage to define a default error message.
  • In the function mentioned above we will introduce special cases, such as - if ModuleException is caught and a flashbag contains errors then we prevent from displaying default message. In another scenario we will display message anyway.

@kpodemski @mickaelandrieu @matks @PierreRambaud wdyt? Ofcourse this solution requires to change almost all controller actions which I tried to avoid but I don't see any other way right now. Unless its not a problem in the exception listener clear all the flashbag success messages ( need to test that as well)

[matks]

Unless its not a problem in the exception listener clear all the flashbag success messages ( need to test that as well)

This is what I thought 🤔actually I did not want us to choose between 1) and 2) : I wanted to say that both solutions will be available to module developers.

My idea is just what you said: in Exception listener, when a ModuleException is thrown, check the flashbag. If empty, add the ModuleException message. If not empty, do nothing. We should not mess with the other messages.

What usecase are you worrying about 🤔? Someone adding the error in the Flashbag but forget to throw the ModuleException ?

Now that I think of it, @jolelievre has worked a lot on modules so his opinion could be helpful too

[matks]

Mmmm 🤔now I have doubt as I see that Core/Form/FormHandler has an hookable $errors argument:

$errors = $this->formDataProvider->setData($data);
        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->hookName}Save",
            [
                'errors' => &$errors,
                'form_data' => &$data,
            ]
        );

while our Core/Form/IdentifiableObject/Handler/FormHandler has not.

So the same question that https://github.com/PrestaShop/PrestaShop/pull/14008/files#r288432066 raised appears: should modules be able to:

• modify $errors raised by core form ?
• modify other $errors raised by other modules ?

If yes, then we should go for an $errors array too

[tomas862]

Mmmm thinkingnow I have doubt as I see that Core/Form/FormHandler has an hookable $errors argument:

$errors = $this->formDataProvider->setData($data);
        $this->hookDispatcher->dispatchWithParameters(
            "action{$this->hookName}Save",
            [
                'errors' => &$errors,
                'form_data' => &$data,
            ]
        );

while our Core/Form/IdentifiableObject/Handler/FormHandler has not.

So the same question that https://github.com/PrestaShop/PrestaShop/pull/14008/files#r288432066 raised appears: should modules be able to:

* modify `$errors` raised by core form ?

* modify other `$errors` raised by other modules ?

If yes, then we should go for an $errors array too

Im afraid in identifiable object hooks , error messages are handled only on controller level - only the exceptions can be throwed from identifiable object scenario - I thing we should keep the same workflow as it seems more correct then writting to errors array

[matks]

All remaining issues have been fixed, the last one was #14239

We'll need to handle #13943 but this is not blocking for 1.7.6 release