Prestashop: Improve the way unexpected errors in migrated pages are handled

[matks]

_This is a copy of issue https://github.com/PrestaShop/PrestaShop/issues/17809 because there was 2 things to fix in original issue, and the 2 fixes will probably be separated in time._

WARNING: following statements are to give context only

Describe the bug

When we have wrong object value in back-office, in some controllers, we can't tell exactly what's wrong, FormBuilder doesn't give us anything even tho we have try catch

Edit by Khouloud

Steps to reproduce the issue:

1. Go to the FO => Create a new account with an email: [email protected] => OK
2. Go to the BO => Customers page => Edit the last customer-created
3. See error

It is ok with PS1.7.5.2.
It is a regression.

With PS1.7.6.3 and PS1.7.6.0, when we try to create a customer with the email [email protected]
=> we have an error

With PS1.7.5.2 => it is successfully added.

---

To Reproduce
Steps to reproduce the behavior:

1. go to ps_customers
2. change e-mail address for some customer to for example some.[email protected]
3. go to back-office
4. try to edit this customer

check screenshot

Screenshots

Issue to discuss and solve

We should have exact information what's wrong here, not a notice because of failure of FormBuilder.

Additional information
PrestaShop version: N/A
PHP version: N/A

[matks]

Hi @kpodemski !

I can give you some context about why we wrapped all form building statements in the code with a very generic try/catch statement.

So, as you know on modern pages which allow to modify identifiable objects such a customer, we provide hooking capabilities for module developers at 2 steps

• when building the form => this allow module developers to add custom fields to the form
• when receiving the data submitted by user through the form => this allow module developers to process the data of the form, including the data filled in their custom fields

If we do not have the generic try { ... } catch (\Exception $e) { ... } statement, then **any bad module hooked on a form could break the whole page.
For example, you have a shop with a very fine "Edit a customer page". Then merchant installs a crappy module that adds a custom field to "edit a customer" form however it has a major flow and throw a \InvalidArgumentException.

• without the try/catch, the merchant only sees (in production environment !) that the page crashes, and might say that PrestaShop code is a piece of trash 😅
• with the try/catch, the merchant has a viewable page (although it cannot be used) with the "An unexpected error occurred" message (which is actually what really happened: something unexpected happened !) and I guess that since he's a smart person he'll understand this new error message comes from the module

This is the whole idea of this try { ... } catch (\Exception $e) { ... } statement : provide a minimum level of usability to the BO even if a bad module is hooked in it.

What do you think about it ?

So when you say

We should have exact information what's wrong here, not a notice because of failure of FormBuilder.

I answer:

• in development/debug environment, yes ! so I think we could check whether or not PS_DEBUG is on and then re-throw the exception from inside the catch
• but in production, I think it's safer to keep the error message and display the page, just like it does today (remember the pretty Symfony error page disappears in production environment, replaced by a blank page with "Something wrong occurred" only)

[kpodemski]

I agree.

but in production, I think it's safer to keep the error message and display the page, just like it does today (remember the pretty Symfony error page disappears in production environment, replaced by a blank page with "Something wrong occurred" only)

of course, my example was related to mismatch between front-office and back-office e-mail validation, i think that some PR have this removed (email validation in BO) which is "ok" as long as this validation would work when the form is submitted

[khouloudbelguith]

Hi @matks,

Related issues about unclear message were fixed in the PS1.7.6.3

1. https://github.com/PrestaShop/PrestaShop/issues/16725 fixed by https://github.com/PrestaShop/PrestaShop/pull/16906
2. https://github.com/PrestaShop/PrestaShop/issues/16514 fixed by https://github.com/PrestaShop/PrestaShop/pull/17087

The Customers page was migrated in the PS1.7.6.
So, the unclear message is a regression compared to PS1.7.5.
Ping @eternoendless, @matks, @colinegin, @marionf what do you think?

Thanks!

[colinegin]

What was the message in 175 @khouloudbelguith ?

[khouloudbelguith]

Hi @colinegin,

No, with PS1.7.5.2, we don't have any red alert.
With PS1.7.5.2 => the customer is successfully added.

Thanks!

[matks]

@khouloudbelguith Why are we still talking about this ? As you said error messages have already been fixed.

The topic of this issue is now "We should have exact information what's wrong here, not a notice because of failure of FormBuilder." 😄

[khouloudbelguith]

@matks, that's ok.
@colinegin, With PS1.7.6.3, we have a red alert "unclear message".
But with PS1.7.6.4build1 => no alert displayed, the customer is successfully added.

Thanks!

[matks]

Ping @PrestaShop/prestashop-core-developers can we confirm the following behavior is good:

In development/debug environment, we could check whether or not PS_DEBUG is on and then re-throw the exception from inside the catch

The idea is to help developers debug their modules while protecting merchants in production environment for breaking their shops.

Example:

// CustomerController.php

    public function createAction(Request $request) {
    ...
    try {
            $result = $customerFormHandler->handle($customerForm);

            if ($customerId = $result->getIdentifiableObjectId()) {
                $this->addFlash('success', $this->trans('Successful creation.', 'Admin.Notifications.Success'));

                ...

                return $this->redirectToRoute('admin_customers_index');
            }
        } catch (Exception $e) {
            $this->addFlash('error', $this->getErrorMessageForException($e, $this->getErrorMessages($e)));
        }

⬇️

// CustomerController.php

    public function createAction(Request $request) {
    ...
    try {
            $result = $customerFormHandler->handle($customerForm);

            if ($customerId = $result->getIdentifiableObjectId()) {
                $this->addFlash('success', $this->trans('Successful creation.', 'Admin.Notifications.Success'));

                ...

                return $this->redirectToRoute('admin_customers_index');
            }
        } catch (Exception $e) {

            // here !
            if ($debugIsEnabled() {
                throw $e;
            }
            $this->addFlash('error', $this->getErrorMessageForException($e, $this->getErrorMessages($e)));
        }

[elboletaire]

I've just stumbled with this issue (with PS 1.7.6.7), and, at least under the edit method, just adding throw $e below the if checking for CustomerNotFoundException, shows the real issue and doesn't affect the default behavior when debug is set to false:

~~~php
$this->addFlash('error', $this->getErrorMessageForException($e, $this->getErrorMessages($e)));
if ($e instanceof CustomerNotFoundException) {
return $this->redirectToRoute('admin_customers_index');
}

throw $e;
~~~

[PierreRambaud]

@matks DO IT!