Prestashop: Issues when creating new module for PS 1.7.6

[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 :trollface:

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 :dancer: )

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? :thinking:

[tomas862]

:ping_pong: @mickaelandrieu :ping_pong:

[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. :smiling_imp:

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? :thinking:

[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:

• https://github.com/PrestaShop/PrestaShop/issues/13942
• https://github.com/PrestaShop/PrestaShop/issues/13943
• https://github.com/PrestaShop/PrestaShop/issues/12585
• https://github.com/PrestaShop/PrestaShop/issues/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 https://github.com/PrestaShop/PrestaShop/pull/14239

We'll need to handle https://github.com/PrestaShop/PrestaShop/issues/13943 but this is not blocking for 1.7.6 release