NotificationsYou must be signed in to change notification settings
Fork9.7k
Star30.8k

[Form] Add ability to clear form errors#27571

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Jump to bottom

Closed

TerjeBr wants to merge2 commits intosymfony:masterfromTerjeBr:clear-form-errors

Closed

[Form] Add ability to clear form errors#27571

TerjeBr wants to merge2 commits intosymfony:masterfromTerjeBr:clear-form-errors

Conversation

Copy link

TerjeBr commentedJun 10, 2018•
edited
Loading

Q	A
Bug fix?	no
New feature?	yes
BC breaks?	yes
Deprecations?	no
Tests pass?	yes
Fixed tickets	#14060
License	MIT
Doc PR	symfony/symfony-docs#9908

This PR adds the ability to manually clear form errors, thus improving the DX issue reported in#14060.
This is a copy of PR#14233

Merge pull request#1from symfony/master

4dcdaf4

Update to latest symfony master

carsonbot added Status: Needs Review Form Feature BC Break labels

Jun 10, 2018

Add ability to clear form errors

b17756a

This was referencedJun 10, 2018

[Form] Ability to clear form errorssymfony/symfony-docs#9908

Closed

[DX] [Form] Ability to reset form validation errors (or prevent them from rendering)#14060

Closed

Copy link

Contributor

colinodell commentedJun 10, 2018

Thank you for re-creating and re-submitting my old commit! 👍

linaori suggested changes

Jun 11, 2018

View reviewed changes

src/Symfony/Component/Form/FormInterface.php

		*
		* @return FormInterface The form instance
		*/
		publicfunctionclearErrors($deep =false);

Copy link

Contributor

linaoriJun 11, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

I don't think this will be possible. You'll have to make this work for 4.x as well and as is, it's a BC break. Probably requires a new interface, which can be merged in this interface in 5.0.

Copy link

Author

TerjeBrJun 11, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Do you mean I should create something likeFormClearErrorsInterface and makeForm implement that?

If we want to merge it intoFormInterface in 5.0, does that mean theFormClearErrorsInterface should be marked as deprecated right away, so that people will be aware that it goes away in version 5.0?

I have not done something like this before, so that is why I would apreciate some guideance in how to do it. Thanks.

Copy link

Author

TerjeBrJun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Also@dosten wrote in#14233 (comment) that the BC break would not be a problem.

Copy link

Contributor

pierredupJun 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Also@dosten wrote in#14233 (comment) that the BC break would not be a problem.

The BC promise states that adding a new method is not acceptablehttp://symfony.com/doc/current/contributing/code/bc.html#changing-interfaces

Copy link

Contributor

jvasseurJun 11, 2018•
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

@TerjeBr The comment you linked dates from 2015, since then the concept of@api interface was removed and we now consider every class as part of the public API (#15977,symfony/symfony-docs#5735). This means we can't add methods to any interface.

carsonbot added Status: Needs Work and removed Status: Needs Review labels

Jun 11, 2018

Copy link

Author

TerjeBr commentedJun 11, 2018

Please correct me if I am misstaken, but is theFormInterface something symfony users would implement?

Is this interfece not more kind of "internal" to the Symfony framework?

Copy link

Contributor

pierredup commentedJun 11, 2018

Only if an interface is marked as@internal, then it's not meant to be implemented (as is not part of the BC promise. Seehttp://symfony.com/doc/current/contributing/code/bc.html#using-our-interfaces for more information)

nicolas-grekas added this to thenext milestone

Jun 11, 2018

Copy link

Author

TerjeBr commentedJun 11, 2018•
edited
Loading

I am trying to find a way forward here. Is it reasonable to want to markFormInterface as@internal? Or is that out of the question? Who else than Symfony maintainersimplements theFormInterface?

Copy link

Author

TerjeBr commentedJun 11, 2018

Or if I do as@iltar suggested, add a new interface, should that interface be created with the tag@deprecated if we aim to remove it again in version 5.0?

colinodell mentioned this pull request

Jun 11, 2018

[Form] Add ability to clear form errors#27580

Merged

Copy link

Member

javiereguiluz commentedJun 19, 2018

Let's ping our Symfony Form expert@vudaltsov to see if he can spot any issue here (besides the interface issue mentioned by Iltar).

Copy link

Contributor

vudaltsov commentedJun 19, 2018

This PR can be closed in favor of#27580.

xabbuh requested changes

Jun 19, 2018

View reviewed changes

src/Symfony/Component/Form/CHANGELOG.md

		* deprecated the`ChoiceLoaderInterface` implementation in`CountryType`,`LanguageType`,`LocaleType` and`CurrencyType`
		* added`input=datetime_immutable` to DateType, TimeType, DateTimeType
		* added`rounding_mode` option to MoneyType
		* added`FormInterface::clearErrors()` method

Copy link

Member

xabbuhJun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

we cannot do this in a minor release as it is a BC break

src/Symfony/Component/Form/FormInterface.php

		*
		* @return FormInterface The form instance
		*/
		publicfunctionclearErrors($deep =false);

Copy link

Member

xabbuhJun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

This is a BC break that needs to be reverted.

src/Symfony/Component/Form/Form.php

		/**
		* {@inheritdoc}
		*/
		publicfunctionclearErrors($deep =false)

Copy link

Member

xabbuhJun 19, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

public function clearErrors(bool $deep = false): void

Copy link

Member

xabbuh commentedJun 19, 2018

Ah sorry, I missed#27580. So I am going to close here in favour of it. Thank you@TerjeBr for bootstrapping the work.

xabbuh closed this

Jun 19, 2018

fabpot added a commit that referenced this pull request

Jun 25, 2018

feature#27580[Form] Add ability to clear form errors (colinodell)

53a39b7

This PR was squashed before being merged into the 4.2-dev branch (closes#27580).Discussion----------[Form] Add ability to clear form errors| Q             | A| ------------- | ---| Branch?       | master| Bug fix?      | no| New feature?  | yes| BC breaks?    | no| Deprecations? | no| Tests pass?   | yes| Fixed tickets |#14060| License       | MIT| Doc PR        |symfony/symfony-docs#9916This PR adds the ability to manually clear form errors, thus improving the DX issue reported in#14060.Unlike my original approach in#14233 and#27571 which break BC, this adds a new `ClearableErrorInterface` which `Form` implements.  (`Button` does not implement it because buttons can't have errors.)Commits-------9eb755c [Form] Add ability to clear form errors