Improved structure for error message generation in configuration pages #29494

JevgenijVisockij · 2022-08-31T14:47:53Z

Questions	Answers
Branch?	develop
Description?	Refactors error generation for configuration pages. With changes error system is more streamlined allowing for pages with same error reuse the code rather then duplicating it, as well as moving code from controllers to separate factories. Also added label provider which takes label from the form, rather then having to duplicate label somewhere else.
Type?	refacto
Category?	BO
BC breaks?	no
Deprecations?	no
Fixed ticket?
Related PRs	#20243 same was done in invoice PR, but separating this, so it hopefully get's faster and is part of 8.0
How to test?	Test that administration page throws errors correctly, when entering wrong info.

zuk3975 · 2022-09-01T07:21:12Z

src/Core/Form/ErrorMessage/Factory/ConfigurationErrorFactoryInterface.php

+use PrestaShop\PrestaShop\Core\Form\ErrorMessage\ConfigurationErrorInterface;
+
+/** Interface for configuration error factories which are responsible for returning error messages for configuration errors */
+interface ConfigurationErrorFactoryInterface


seems more like a message provider.
"ConfigurationErrorMessageProvider".

then it will be less confusing considering we also have src/PrestaShopBundle/Form/ErrorMessage/Factory/ConfigurationErrorFactory.php (because now its named the same, but is different service)

Yeah I guess the name make sense, thanks!

zuk3975 · 2022-09-01T07:24:46Z

src/PrestaShopBundle/Form/ErrorMessage/LabelProvider.php

+        foreach ($view->children as $child) {
+            if (isset($child->vars['name']) && $fieldName === $child->vars['name']) {
+                if (!isset($child->vars['label'])) {
+                    throw new FieldLabelNotFoundException(


do we really need to throw error if label isn't there?
I think It is legit that some fields doesn't have labels, so what happens if such field is violated? then everything will probably break, no?

Maybe we should fallback to field name when label is not set?

Hmm, I guess you are right, I will just return fieldName, probably better then breaking everything, since then only display will be a worse in worst case.

The base branch was changed.

JevgenijVisockij · 2022-10-04T07:59:11Z

Delayed until 9.0

jolelievre

As we discussed we'll try and minimize the breaking changes here so:

we don't remove the initial error class, nor the collection and its associated exception, but we deprecate them
create new Error classes (with your interface) along with a new collection and associated exception

In the end, the only BC is that the validation method triggers a new kind of exception

FabienPapet · 2022-11-14T09:23:18Z

...PrestaShopBundle/Form/Admin/Configure/AdvancedParameters/Administration/FormDataProvider.php

@@ -35,10 +35,6 @@
 */
 final class FormDataProvider implements FormDataProviderInterface
 {
-    public const ERROR_NOT_NUMERIC_OR_LOWER_THAN_ZERO = 1;


are these constants already part of a release ? if yes that is a BC Break

You are right, it's BC break will add them back and depricate

FabienPapet · 2022-11-14T09:25:24Z

src/PrestaShopBundle/Form/Exception/DataProviderException.php

@@ -31,6 +31,7 @@
 use PrestaShop\PrestaShop\Core\Domain\Exception\DomainException;
 use Throwable;

+/** @deprecated and will be removed in 9.0 */


You can call @trigger_error before the class to make sure in will appear in the logs.

src/PrestaShopBundle/Form/Exception/InvalidConfigurationDataError.php

src/PrestaShopBundle/Form/Exception/InvalidConfigurationDataErrorCollection.php

FabienPapet · 2022-11-14T09:26:18Z

src/PrestaShopBundle/Resources/config/services/bundle/form/error_message.yml

@@ -0,0 +1,27 @@
+services:
+  _defaults:
+    public: true


I think these services don't need to be public

As far as I see every single yml file in PS has services as public, I don't remember the exact reason, but I know at least in modules sometimes things don't work if you don't make services public.

We are starting to make private services to optimize the container, I think it is better to stop adding public service, il will ease the work in the future.

Also, you can create your new services with the FQCN now :)

Alright that makes sense. Implemented FQCN.

FabienPapet · 2022-11-14T09:26:33Z

...ShopBundle/Form/ErrorMessage/Factory/AdministrationConfigurationErrorMessageProviderTest.php

+use PrestaShopBundle\Entity\Repository\LangRepository;
+use PrestaShopBundle\Form\Admin\Configure\AdvancedParameters\Administration\GeneralDataProvider;
+use PrestaShopBundle\Form\ErrorMessage\Factory\AdministrationConfigurationErrorMessageProvider;
+use Symfony\Component\Translation\Translator;


Wrong namespace here

You mean the translator? Changed it to interface since that's what class is using

he one from Symfony\Contracts should be used instead.
#30496

FabienPapet · 2022-11-14T09:28:45Z

src/Core/Form/ErrorMessage/AdministrationConfigurationError.php

+    /**
+     * @return int|null
+     */
+    public function getLanguageId(): ?int


Where is this function used ?

It's needed in the future, but after thinking about it yeah it's probably not needed now. Removed it.

FabienPapet · 2022-11-14T09:30:14Z

src/Core/Form/ErrorMessage/ConfigurationErrorInterface.php

+    /**
+     * @return int|null
+     */
+    public function getLanguageId(): ?int;


I don't think the interface should have this method. An error does not have to be aware of the context locale IMO.

It meant not for the context locale, but for when there is mulitlang field, so we know in which language error happened. But yeah it's not needed in interface errors needing that can use the function without it being in interface.

zuk3975 · 2022-11-29T09:38:28Z

src/Core/Form/ErrorMessage/AbstractConfigurationError.php

+     * AdministrationConfigurationError constructor.
+     *


Suggested change

* AdministrationConfigurationError constructor.

*

FabienPapet · 2022-11-29T10:03:23Z

src/PrestaShopBundle/Form/ErrorMessage/Factory/CommonConfigurationErrorMessageProvider.php

+use PrestaShop\PrestaShop\Core\Form\ErrorMessage\ConfigurationErrorInterface;
+use PrestaShop\PrestaShop\Core\Form\ErrorMessage\Factory\ConfigurationErrorMessageProviderInterface;
+use PrestaShopBundle\Entity\Repository\LangRepository;
+use Symfony\Component\Translation\TranslatorInterface;


The translator should come from Symfony/Contracts (in mutliple places)

Changed those

FabienPapet · 2022-11-29T10:05:52Z

src/PrestaShopBundle/Resources/config/services/bundle/form/error_message.yml

@@ -0,0 +1,27 @@
+services:
+  _defaults:
+    public: true


We are starting to make private services to optimize the container, I think it is better to stop adding public service, il will ease the work in the future.

Also, you can create your new services with the FQCN now :)

zuk3975 · 2022-12-05T16:18:41Z

src/PrestaShopBundle/Resources/config/services/bundle/form/error_message.yml

@@ -0,0 +1,23 @@
+services:
+  PrestaShopBundle\Form\ErrorMessage\LabelProvider:
+    class: PrestaShopBundle\Form\ErrorMessage\LabelProvider


When using FQCN, the class: is not needed anymore, as it should follow the namespace and symfony finds it. (on the other hand when the service have multiple implementations, then it is supposed to be defined using snake_case and requires the class: attribute)

Suggested change

class: PrestaShopBundle\Form\ErrorMessage\LabelProvider

check other definitions too.

Changes those, thanks!

lartist · 2023-01-10T09:55:22Z

src/PrestaShopBundle/Controller/Admin/Configure/AdvancedParameters/AdministrationController.php

-            } catch (DataProviderException $e) {
-                $this->flashErrors($this->getErrorMessages($e->getInvalidConfigurationDataErrors()));
+            } catch (FormDataProviderException $e) {
+                $errorMessageFactory = $this->get('prestashop.form.error_message.configuration_error_factory');


I think for new service call, we should fit with https://github.com/PrestaShop/ADR/blob/master/0019-FQCN-Autowiring.md. You probably should create FQCN version of prestashop.form.error_message.configuration_error_factory and use it here.

zuk3975 · 2023-01-18T09:59:01Z

ping @PrestaShop/wording-managers to approve the wording before we go to QA

...estaShopBundle/Form/ErrorMessage/Factory/AdministrationConfigurationErrorMessageProvider.php

l-delin · 2023-01-18T13:22:40Z

src/PrestaShopBundle/Form/ErrorMessage/Factory/ConfigurationErrorFactory.php

+    private function getDefaultErrorMessage(string $label): string
+    {
+        return $this->translator->trans(
+            '%s is invalid.',


Is there more info on the nature of the error? Telling the user "x is invalid" isn't going to help them 🤔

This message is used only for the fallback case, the idea here is to try and match any Exception with the appropriate error message, that is actually what was done in the previous provider classes like CommonConfigurationErrorMessageProvider on a case by case based on the exception

But in case no specific case was handled for an unexpected error we still need to display some default message, it will lack details until the error use case is properly handled for the specific error case.

Oh, ok - I see. Thank you

l-delin · 2023-01-18T13:52:44Z

...estaShopBundle/Form/ErrorMessage/Factory/AdministrationConfigurationErrorMessageProvider.php

+        switch ($error->getErrorCode()) {
+            case AdministrationConfigurationError::ERROR_COOKIE_LIFETIME_MAX_VALUE_EXCEEDED:
+                return $this->translator->trans(
+                    '%s is invalid. Please enter an integer lower than %s.',


Suggested change

'%s is invalid. Please enter an integer lower than %s.',

'Exceeds the maximum lifetime value of a cookie. Please enter an integer lower than %s.',

Wouldn't it be better to add a help text to tell the user what the max lifetime value of a cookie is?
So that we don't have to tell them "No, the value you entered is too high" after they're done typing it in.

Actually @l-delin the message here uses a placeholder %s is invalid the %s part is replaced by the variable $label which will automatically provide the name of the field from the form (which is also translated independently). So the interest here is to use a single wording with placeholder for any label/field

Alright, makes sense. Then, feel free to dismiss this edit :-)

…onErrorMessageProvider.php fixed tests

…onfigurationErrorMessageProvider.php Wording changes Co-authored-by: l-delin <113511191+l-delin@users.noreply.github.com>

JevgenijVisockij · 2023-05-31T09:25:50Z

The SameSite=None attribute is only available in a secure context (HTTPS).

Fixed thie issue

JevgenijVisockij · 2023-06-28T12:46:37Z

Ready for review. Once this is merged #22471, #22139 and #20243 can be finished using the same logic that is updated here.

JevgenijVisockij requested a review from a team as a code owner August 31, 2022 14:47

prestonBot added the Refactoring Type: Refactoring label Aug 31, 2022

zuk3975 reviewed Sep 1, 2022

View reviewed changes

prestonBot added the develop Branch label Sep 1, 2022

zuk3975 previously approved these changes Sep 12, 2022

View reviewed changes

JevgenijVisockij changed the base branch from develop to 8.0.x September 15, 2022 08:26

JevgenijVisockij force-pushed the option-error-improvements branch from 7fe26fa to 24b036c Compare September 15, 2022 08:28

JevgenijVisockij changed the base branch from 8.0.x to develop September 15, 2022 08:32

jolelievre reviewed Oct 17, 2022

View reviewed changes

prestonBot added the Waiting for wording Status: action required, waiting for wording label Oct 17, 2022

FabienPapet reviewed Nov 14, 2022

View reviewed changes

JevgenijVisockij requested a review from a team as a code owner November 23, 2022 14:07

JevgenijVisockij closed this Nov 28, 2022

JevgenijVisockij reopened this Nov 28, 2022

zuk3975 reviewed Nov 29, 2022

View reviewed changes

zuk3975 previously approved these changes Nov 29, 2022

View reviewed changes

FabienPapet reviewed Nov 29, 2022

View reviewed changes

JevgenijVisockij dismissed zuk3975’s stale review via 157bb7d December 5, 2022 16:13

zuk3975 reviewed Dec 5, 2022

View reviewed changes

zuk3975 previously approved these changes Dec 13, 2022

View reviewed changes

lartist reviewed Jan 10, 2023

View reviewed changes

JevgenijVisockij dismissed zuk3975’s stale review via 5301e75 January 11, 2023 07:54

lartist previously approved these changes Jan 17, 2023

View reviewed changes

zuk3975 added this to the 8.1.0 milestone Jan 18, 2023

zuk3975 previously approved these changes Jan 18, 2023

View reviewed changes

l-delin reviewed Jan 18, 2023

View reviewed changes

JevgenijVisockij and others added 22 commits May 31, 2023 12:21

Removed accidentally added exception

ea18922

Removed accidentally added exception

0e07dc8

Adding back const to avoid BC break, change test to use interface

f509c54

Adding back old collection and error and depricating them

f83d050

Added deprication trigger error

2257542

Removed not needed language repository from AdministrationConfigurati…

bd51aef

…onErrorMessageProvider.php fixed tests

Fixing unit tests

059386b

Removed unneeded comment

fd1ec28

Removed unneeded public, changing classes naming to FQCN

2303177

Removing uneeded class

13be1cd

Replacing with symfony contracts

e03ab92

CS fixer fix

53cc57a

Adding autowiring to new services

e79d002

Using the class in AdministrationController.php

8bbd639

Update src/PrestaShopBundle/Form/ErrorMessage/Factory/AdministrationC…

00ef072

…onfigurationErrorMessageProvider.php Wording changes Co-authored-by: l-delin <113511191+l-delin@users.noreply.github.com>

Made ConfigurationErrorFactory public because it was causing fatal

ce7d80f

Refactored yml file to use autowire properly

ae400cc

Adding back label provider and ConfigurationErrorCollection.php

667c29d

Cs Fixer fix

311f56d

Fixing to use MAX_COOKIE_VALUE from cookie options

f9a84a1

Cs Fixer fix

c046cbc

syntax fix

f32ea6d

JevgenijVisockij force-pushed the option-error-improvements branch from 8767a90 to f32ea6d Compare May 31, 2023 09:22

Fixed bug with wrong error

ed8990a

JevgenijVisockij dismissed stale reviews from zuk3975 and lartist via ed8990a May 31, 2023 09:25

prestashop-issue-bot bot removed the Waiting for author Status: action required, waiting for author feedback label May 31, 2023

JevgenijVisockij mentioned this pull request Jun 28, 2023

Simplify invoices form #20243

Open

	'%s is invalid. Please enter an integer lower than %s.',
	'Exceeds the maximum lifetime value of a cookie. Please enter an integer lower than %s.',

Improved structure for error message generation in configuration pages #29494

Are you sure you want to change the base?

Improved structure for error message generation in configuration pages #29494

Conversation

JevgenijVisockij commented Aug 31, 2022 • edited

zuk3975 Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuk3975 Sep 1, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JevgenijVisockij commented Oct 4, 2022

jolelievre left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuk3975 commented Jan 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

l-delin Jan 19, 2023 • edited

Choose a reason for hiding this comment

JevgenijVisockij commented May 31, 2023

JevgenijVisockij commented Jun 28, 2023

JevgenijVisockij commented Aug 31, 2022 •

edited

zuk3975 Sep 1, 2022 •

edited

zuk3975 Sep 1, 2022 •

edited

l-delin Jan 19, 2023 •

edited