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

[Validator] Add BC layer for notInRangeMessage when min and max are set #36140

Open
wants to merge 1 commit into
base: 4.4
from

Conversation

@l-vo
Copy link
Contributor

l-vo commented Mar 19, 2020

Q A
Branch? 4.4
Bug fix? yes
New feature? no
Deprecations? no
Tickets Fix #36133
License MIT
Doc PR

According to #36133, the improvement added in #32435 may lead to a BC break when the developer pass min and max options and a custom minMessage or maxMessage. In this case it's expected to receive a minMessage/maxMessage in the violation but a notInRangeMessage is received instead.

So in the following conditions:

  • min and max options are set
  • minMessage or maxMessage is set

A deprecation is triggered. If a limit is violated and matches to the min/max message passed as option (minMessage for min violated and maxMessage for max violated), minMessage/maxMessage is used in the violation instead of notInRangeMessage.

@dmaicher

This comment has been minimized.

Copy link
Contributor

dmaicher commented Mar 19, 2020

Also see #33700

I also noticed that and back then we decided to document this small behavior change.

@l-vo l-vo force-pushed the l-vo:add_bc_layer_range_message_change branch 2 times, most recently from 2d50ed1 to f12794e Mar 19, 2020
@nicolas-grekas nicolas-grekas added this to the next milestone Mar 19, 2020
@l-vo l-vo force-pushed the l-vo:add_bc_layer_range_message_change branch 2 times, most recently from d8f0c3f to 1136014 Mar 19, 2020
@chalasr chalasr added the Deprecation label Mar 19, 2020
@chalasr

This comment has been minimized.

Copy link
Member

chalasr commented Mar 19, 2020

Adding a BC layer on a maintenance branch feels wrong to me, you shouldn't get new deprecation notices in a patch release. The BC break should either be reverted or documented instead.

@xabbuh xabbuh added the Validator label Mar 21, 2020
@l-vo l-vo force-pushed the l-vo:add_bc_layer_range_message_change branch from 1136014 to dc9a366 Mar 21, 2020
@l-vo l-vo force-pushed the l-vo:add_bc_layer_range_message_change branch from dc9a366 to 092d85c Mar 21, 2020
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Mar 31, 2020

The BC break should either be reverted or documented instead

Reverting would mean breaking BC on 5.0 - Documenting is what this PR is about.
If we missed triggering a deprecation while something has been deprecated, adding this deprecation is a bug fix to me.

Note that I didn't get the patch so I'm just commenting on the process :)

@xabbuh xabbuh modified the milestones: next, 4.4 Apr 1, 2020
$this->deprecatedMaxMessageSet = isset($options['maxMessage']);

if ($this->deprecatedMinMessageSet || $this->deprecatedMaxMessageSet) {
@trigger_error('Since symfony/validator 4.4: "minMessage" and "maxMessage" are deprecated when the "min" and "max" options are both set. Use "notInRangeMessage" instead.', E_USER_DEPRECATED);

This comment has been minimized.

Copy link
@HeahDude

HeahDude Apr 1, 2020

Member

It means we also miss an exception here in 5.0, and maybe that should be part of the message.

This comment has been minimized.

Copy link
@l-vo

l-vo Apr 2, 2020

Author Contributor

I'm not sure to understand, what should be part of the message please ?

This comment has been minimized.

Copy link
@HeahDude

HeahDude Apr 2, 2020

Member

Since symfony/validator 4.4: "minMessage" and "maxMessage" are deprecated when the "min" and "max" options are both set. You should use "notInRangeMessage" instead or an exception will be thrown in 5.0.

This comment has been minimized.

Copy link
@dmaicher

dmaicher Apr 2, 2020

Contributor

But we cannot add an Exception here in a patch release of 5.0.x, right? This would be quite a BC break

This comment has been minimized.

Copy link
@stof

stof Apr 2, 2020

Member

well, we have a default notInRangeMessage in 5.0. So 5.0 will not throw an exception. It will use the built-in message even you you might have intended to use a custom one.

This comment has been minimized.

Copy link
@HeahDude

HeahDude Apr 2, 2020

Member

even if you might have intended to use a custom one

Sounds like a silent failure, the deprecation should be replaced by an exception in 5.0 or master IMO because the intended message won't be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants
You can’t perform that action at this time.