[Form] Improve invalid messages for form types #27142

hiddewie · 2018-05-03T20:10:59Z

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#5946
License	MIT

Improvement of #14106. Added invalid_message fields to all form types. This gives better errors to users instead of the default Invalid value error.

Backwards compatibility

After discussion about BC, the following additions have been made:

An framework option framework.form.improved_validation_messages (default false) for enabling improved validation messages globally.
A form validation extension option improved_validation_messages which enables improved validation messages for that type.
An added parameter for the ValidatorExtension and FormTypeValidatorExtension (default false) to enable/disable the improved validation messages for that extension.
All the form types now check for the enabled opt-in configuration for improved validation messages. Otherwise, the previous default is used which should be the original message.

Upgrade path

These changes should be backwards compatible.
For Symfony 5.0, the improved validation messages configuration in the framework and form should be true (enabled) by default.
The options can be deprecated entirely in Symfony 5.1, to be removed in Symfony 6.0.

fabpot

Looks good to me. We should add all these new error messages in translations as well.

fabpot · 2018-05-18T02:11:48Z

src/Symfony/Component/Form/Extension/Core/Type/MoneyType.php

@@ -58,6 +58,7 @@ public function configureOptions(OptionsResolver $resolver)
            'divisor' => 1,
            'currency' => 'EUR',
            'compound' => false,
+            'invalid_message' => 'The money is invalid.',


money amount?

hiddewie · 2018-05-18T17:36:39Z

@fabpot Thank you for the review. I've added your comment for the translation for the money type. As for the translations, do you mean all the files in src\Symfony\Component\Form\Resources\translations\validators.*.xlf? Of course I can add the translation keys there, but I cannot do translations in all those different languages (except Dutch and maybe German).

src/Symfony/Component/Form/Extension/Core/Type/TimezoneType.php

src/Symfony/Component/Form/Extension/Core/Type/TextareaType.php

hiddewie · 2018-09-09T19:10:12Z

@fabian I've added the translations to the English file. However, I am not sure what the correct precedure is for all the other languages (of which I can only translate 2 myself).

xabbuh · 2018-09-15T09:37:54Z

I am not sure that it is a good idea to change the default value for this option for all built-in form types. This means that custom translations in project wouldn't be working anymore after updating to a Symfony version containing this feature. That's not a good DX IMO.

hiddewie · 2018-09-19T18:45:30Z

@xabbuh You are right, these changes could be seen as backwards incompatible. What do you propose: close the PR or leave it open until there is a major new version?

fabpot · 2018-10-10T11:15:41Z

I didn't thought about the BC issue here. Looks like a deal breaker :( Let's close then. Sorry @hiddewie

ogizanagi · 2018-10-10T12:46:07Z

Perhaps we could reopen and find a proper upgrade path?

For instance, register a better_invalid_messages or [INSERT_BETTER_NAME] option from FormTypeValidatorExtension. Then if, if better_invalid_messages is false, trigger a deprecation.
A framework.form.better_invalid_messages framework option can be used to configure the default value used by FormTypeValidatorExtension globally, so any new project can be free of deprecations.

Then, in 5.1, deprecate better_invalid_messages and framework.form.better_invalid_messages to be removed in 6.0 as it won't have any effect anymore.

This seems a lot, but I find this PR valuable. Actually, IMHO, the BC break here is sensible.

hiddewie · 2018-10-12T18:15:51Z

@ogizanagi That would be possible! Would an option name of improved_validation_messages work?

What is the proper precedure for deprecating something in a far future release? Just open a new issue after this PR is merged (if it is merged at all)?

ogizanagi · 2018-10-12T19:03:37Z

What is the proper procedure for deprecating something in a far future release? Just open a new issue after this PR is merged (if it is merged at all)?

Actually I don't think we have any specific workflow for this.
An issue could be great, but can be easily forgotten.
We could open a note on a project in https://github.com/symfony/symfony/projects in order to not forget this, but this kind of situation doesn't happen really often.
But I'm confident anyone doing the PR to remove the BC layer in 5.0 will think about doing one to deprecate it in 5.1 :)

hiddewie · 2018-10-21T15:17:21Z

@ogizanagi I've made some changes. Review is very welcome. The PR has grown considerably in code and also in complexity, but I hope everything works out.

Also see the updated description of the PR for some notes on the upgrade path. Mostly what you described.

I've enabled the improved messages in most of the tests, to avoid breaking the tests because of the deprecation message. Marking them all as legacy does not seem as a good idea to me. I've made some new tests that check the backwards deprecation notice both for framework and form).

ogizanagi

Great work @hiddewie
Note we could set this to true by default for new projects by adding a form recipe.

src/Symfony/Component/Form/Extension/Core/Type/BirthdayType.php

src/Symfony/Component/Form/Extension/Validator/Type/FormTypeValidatorExtension.php

src/Symfony/Component/Form/Extension/Core/Type/DateTimeType.php

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Form/Extension/Validator/Type/FormTypeValidatorExtension.php

src/Symfony/Component/Form/Extension/Validator/ValidatorExtension.php

src/Symfony/Component/Form/Resources/translations/validators.en.xlf

hiddewie · 2018-10-22T19:08:22Z

@ogizanagi I've processed your comments. Thanks for the input.

I will look into updating the receipe.
I also want to write tests for at least some form types in the Core extension and using the Type Validator Extension which check the old and the new error messages.
I have to update the changelog files for all these new framework/form parameters/options.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/Configuration.php

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Form/Extension/Core/Type/BirthdayType.php

ro0NL · 2018-10-22T19:21:05Z

src/Symfony/Component/Form/Extension/Validator/ValidatorExtension.php

@@ -25,9 +25,12 @@
 class ValidatorExtension extends AbstractExtension
 {
    private $validator;
+    private $improvedValidationMessages;


instead of this integration wouldnt setting e.g. static FormType::$legacyErrorMessages = false be simpler? That's something we can get rid of easily in 5.0 (mark it internal): set it from the bundle extension and access it in each form type

That'd be less flexible (as it'll only be global) if you want to update part of your app step by step...but regarding the number of translations to update, that might be an option to consider.

introducing a new improved_validation_messages form type option, we have to deprecate again in 5.0 and remove in 6.0, is not worth it IMHO

hiddewie · 2018-11-10T14:50:03Z

@ro0NL @ogizanagi Processed some comments.

HeahDude

Thank you for improving this in a BC way!
It would be nice to have other reviews to be sure we cannot improve some wording more before breaking BC later. @weaverryan @javiereguiluz would you mind having a look at this one?

src/Symfony/Component/Form/Extension/Core/Type/ButtonType.php

src/Symfony/Component/Form/Extension/Core/Type/ResetType.php

src/Symfony/Component/Form/Extension/Core/Type/SubmitType.php

hiddewie · 2018-11-19T19:36:16Z

@HeahDude I've processed your comments. Didn't think of the types which cannot be invalid.

HeahDude · 2018-11-24T12:48:45Z

Thank you @hiddewie!

I've reviewed it again and it looks good to me. But we need to adjust some things now that #28731 as been merged. Because the option did not exist in core before, only added by the validator extension (https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Validator/Type/FormTypeValidatorExtension.php#L59).

So here's what I spotted so far:

the option can be handled here now https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/EventListener/TransformationFailureListener.php#L54.
the check here need to be changed https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Core/Type/TransformationFailureExtension.php#L33
Maybe we should rely on the constraints option only.

ping @xabbuh what do you think?

hiddewie · 2018-12-30T11:00:28Z

@HeahDude Thanks. Processed comment.

I'm unsure if I should remove the same code from https://github.com/symfony/symfony/blob/master/src/Symfony/Component/Form/Extension/Validator/Constraints/FormValidator.php#L117 (I don't think so since any forms with the constraints option are excluded from the new extension).

fabpot · 2019-01-01T17:38:15Z

@hiddewie For this pull request to be merged, you need to rebase it (to remove the merge commits).

hiddewie · 2019-01-02T15:14:07Z

@fabpot Done.

HeahDude

Thank you! another round of review :)

src/Symfony/Component/Form/Extension/Core/Type/SubmitType.php

src/Symfony/Component/Form/Extension/Core/Type/FormType.php

src/Symfony/Component/Form/Extension/Validator/Type/FormTypeValidatorExtension.php

src/Symfony/Component/Form/Resources/translations/validators.en.xlf

src/Symfony/Component/Form/Tests/Extension/Core/Type/BirthdayTypeTest.php

src/Symfony/Component/Form/Tests/Extension/Core/Type/FormTypeTest.php

hiddewie · 2019-01-26T12:17:50Z

@HeahDude Thanks for the review. I've added a load of tests for the custom validation messages, including the legacy variant.

I also noticed that the logic for legacy / non-legacy was flipped in the form types. Has been corrected.

Also rebased for new array syntax changes.

src/Symfony/Bundle/FrameworkBundle/DependencyInjection/FrameworkExtension.php

src/Symfony/Component/Form/Extension/Validator/Type/FormTypeValidatorExtension.php

hiddewie · 2019-02-04T17:25:41Z

@xabbuh Done!

xabbuh · 2019-04-06T21:07:15Z

Thank you very much for your pull request!

As part of the Symfony EU funded hackathon (https://symfony.com/blog/the-symfony-and-api-platform-hackathon-is-coming), we were able to assemble most of the core team and big contributors in one place. Our goal is to finish as many open issues and PRs as possible.

Your commits will not be lost and you will therefore get credit for your work. All that will happen is that your commits will be moved to a new PR (see #30931) where all remaining concerns will be addressed.

Without your work this would not have been possible. So thank you once again!

hiddewie · 2019-04-07T07:54:14Z

No problem, thanks for the work!

This PR was merged into the 5.2-dev branch. Discussion ---------- [Form] Improve invalid messages for form types | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | yes | Tests pass? | yes | Fixed tickets | #27142, #5946 | License | MIT | Doc PR | TODO This merge request is a continuation of #27142. Changes done here: * rebased on master * made the error messages friendlier Commits ------- 8728927 Improve invalid messages for form types

carsonbot added the Status: Needs Review label May 3, 2018

hiddewie mentioned this pull request May 3, 2018

[Form] Improve default form error messages #5946

Closed

nicolas-grekas added this to the next milestone May 3, 2018

nicolas-grekas changed the title ~~Improve invalid messages for form types~~ [Form] Improve invalid messages for form types May 4, 2018

xabbuh added Form Feature labels May 4, 2018

carsonbot removed the Status: Needs Review label May 18, 2018

fabpot requested changes May 18, 2018

View reviewed changes

carsonbot added the Status: Needs Work label May 18, 2018

ro0NL reviewed Sep 8, 2018

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/TimezoneType.php Outdated Show resolved Hide resolved

ro0NL reviewed Sep 8, 2018

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/TextareaType.php Outdated Show resolved Hide resolved

fabpot closed this Oct 10, 2018

fabpot reopened this Oct 10, 2018

ogizanagi reviewed Oct 21, 2018

View reviewed changes

ro0NL reviewed Oct 22, 2018

View reviewed changes

HeahDude suggested changes Nov 18, 2018

View reviewed changes

src/Symfony/Component/Form/Extension/Core/Type/ButtonType.php Outdated Show resolved Hide resolved

src/Symfony/Component/Form/Extension/Core/Type/ResetType.php Outdated Show resolved Hide resolved

src/Symfony/Component/Form/Extension/Core/Type/SubmitType.php Outdated Show resolved Hide resolved

HeahDude suggested changes Jan 3, 2019

View reviewed changes

hiddewie added 4 commits January 26, 2019 13:06

Improve invalid messages for form types

0f26f72

Process comments

b1d493c

Process more comments

4a02feb

Fix code style

83e1a5f

xabbuh requested changes Jan 30, 2019

View reviewed changes

Process some more comments for improving deprecation messages

63f2b02

webmozart mentioned this pull request Apr 6, 2019

[Form] Improve invalid messages for form types #30931

Merged

xabbuh closed this Apr 6, 2019

xabbuh added the ⭐️ EUFOSSA Hackathon label Apr 6, 2019

nicolas-grekas modified the milestones: next, 4.3 Apr 30, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Form] Improve invalid messages for form types #27142

[Form] Improve invalid messages for form types #27142

hiddewie commented May 3, 2018 •

edited

Loading

fabpot left a comment

fabpot May 18, 2018

hiddewie commented May 18, 2018 •

edited

Loading

hiddewie commented Sep 9, 2018

xabbuh commented Sep 15, 2018

hiddewie commented Sep 19, 2018

fabpot commented Oct 10, 2018

ogizanagi commented Oct 10, 2018

hiddewie commented Oct 12, 2018

ogizanagi commented Oct 12, 2018

hiddewie commented Oct 21, 2018 •

edited

Loading

ogizanagi left a comment

hiddewie commented Oct 22, 2018 •

edited

Loading

ro0NL Oct 22, 2018

ogizanagi Oct 22, 2018

ro0NL Oct 22, 2018

hiddewie commented Nov 10, 2018

HeahDude left a comment

hiddewie commented Nov 19, 2018

HeahDude commented Nov 24, 2018

hiddewie commented Dec 30, 2018

fabpot commented Jan 1, 2019

hiddewie commented Jan 2, 2019

HeahDude left a comment

hiddewie commented Jan 26, 2019 •

edited

Loading

hiddewie commented Feb 4, 2019

xabbuh commented Apr 6, 2019

hiddewie commented Apr 7, 2019

[Form] Improve invalid messages for form types #27142

[Form] Improve invalid messages for form types #27142

Conversation

hiddewie commented May 3, 2018 • edited Loading

Backwards compatibility

Upgrade path

fabpot left a comment

Choose a reason for hiding this comment

fabpot May 18, 2018

Choose a reason for hiding this comment

hiddewie commented May 18, 2018 • edited Loading

hiddewie commented Sep 9, 2018

xabbuh commented Sep 15, 2018

hiddewie commented Sep 19, 2018

fabpot commented Oct 10, 2018

ogizanagi commented Oct 10, 2018

hiddewie commented Oct 12, 2018

ogizanagi commented Oct 12, 2018

hiddewie commented Oct 21, 2018 • edited Loading

ogizanagi left a comment

Choose a reason for hiding this comment

hiddewie commented Oct 22, 2018 • edited Loading

ro0NL Oct 22, 2018

Choose a reason for hiding this comment

ogizanagi Oct 22, 2018

Choose a reason for hiding this comment

ro0NL Oct 22, 2018

Choose a reason for hiding this comment

hiddewie commented Nov 10, 2018

HeahDude left a comment

Choose a reason for hiding this comment

hiddewie commented Nov 19, 2018

HeahDude commented Nov 24, 2018

hiddewie commented Dec 30, 2018

fabpot commented Jan 1, 2019

hiddewie commented Jan 2, 2019

HeahDude left a comment

Choose a reason for hiding this comment

hiddewie commented Jan 26, 2019 • edited Loading

hiddewie commented Feb 4, 2019

xabbuh commented Apr 6, 2019

hiddewie commented Apr 7, 2019

hiddewie commented May 3, 2018 •

edited

Loading

hiddewie commented May 18, 2018 •

edited

Loading

hiddewie commented Oct 21, 2018 •

edited

Loading

hiddewie commented Oct 22, 2018 •

edited

Loading

hiddewie commented Jan 26, 2019 •

edited

Loading