Skip to content
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Form] Add LocaleTypeTest::testInvalidLocaleMessage() #29828

Closed
wants to merge 1 commit into from

Conversation

phansys
Copy link
Contributor

@phansys phansys commented Jan 9, 2019

Q A
Branch 3.4
Bug fix? no
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets n/a
License MIT
Doc PR n/a

Since passing a invalid locale causes the form to be not synchronized, the validation fails because this reason instead of the locale validation.
Since I don't know if this is the expected result, this PR provides a test covering the described behavior in order to clarify the situation.

@phansys phansys changed the title Add LocaleTypeTest::testInvalidLocaleMessage() [Form] Add LocaleTypeTest::testInvalidLocaleMessage() Jan 9, 2019
@xabbuh
Copy link
Member

xabbuh commented Jan 10, 2019

The current behaviour is expected. The LocaleType is a specialised ChoiceType and the ChoiceType is automatically invalid in case an invalid choice is submitted (there have been bugfixes recently to ensure this behaviour).

@phansys
Copy link
Contributor Author

phansys commented Jan 10, 2019

Thank you for the response.
IIUC, based on that premise, when I submit a invalid locale through this form type I can't get the violation from LocaleValidator anymore?

@xabbuh
Copy link
Member

xabbuh commented Jan 10, 2019

Yes, that's right. But you can use the invalid_message option of the form type to reuse the message from the validator.

@phansys
Copy link
Contributor Author

phansys commented Jan 10, 2019

In that case, I should instantiate manually the Locale constraint just to obtain the $message property and pass it as the "invalid_message" option. Moreover, I think this solution doesn't cover (in an easy way) the scenario where the validation constraint has multiple possible messages or when one form field has more than one validation constraint associated to the property which the field represents.

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Jan 24, 2019
@nicolas-grekas
Copy link
Member

Should we merge this PR? If yes, please rebase to account for short arrays. If needed, please open an issue if there is anything to keep track of.

@xabbuh
Copy link
Member

xabbuh commented Feb 1, 2019

The added test is failing as expected. I think for cases where the current behaviour is not sufficient enough, having to configure the invalid_message option yourself is acceptable. You even do not need to instantiate the constraint. Just take a look at the message being used and optionally copy its translations to the messages domain.

@phansys
Copy link
Contributor Author

phansys commented Feb 1, 2019

Thank for your response. I think this behavior was changed between 3.4.20 and 3.4.21. I detected this change thanks to a test that I have in a own project, so I think the change, although minor, could be considered a BC break.
Is there any entry at changelog or upgrade files? If not, should I add a note for this change?
Please, let me know.

@xabbuh
Copy link
Member

xabbuh commented Feb 2, 2019

I think that's because before #29500 was merged, the LocaleType mistakenly was not marked as invalid (not synchronised). Though I do not think we should add a dedicated changelog entry. That PR was just a regular bugfix.

@phansys
Copy link
Contributor Author

phansys commented Feb 2, 2019

Closing based on the given explanation. Thank you!

@phansys phansys closed this Feb 2, 2019
@phansys phansys deleted the form_not_synchronized branch February 2, 2019 12:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants