Skip to content

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

Merged
fabpot merged 1 commit intosymfony:4.4from
l-vo:add_bc_layer_range_message_change
Aug 12, 2020
Merged

[Validator] Add BC layer for notInRangeMessage when min and max are set#36140
fabpot merged 1 commit intosymfony:4.4from
l-vo:add_bc_layer_range_message_change

Conversation

@l-vo
Copy link
Copy Markdown
Contributor

@l-vo 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
Copy link
Copy Markdown
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 add_bc_layer_range_message_change branch 2 times, most recently from 2d50ed1 to f12794e Compare March 19, 2020 12:07
@nicolas-grekas nicolas-grekas added this to the next milestone Mar 19, 2020
nicolas-grekas
nicolas-grekas reviewed Mar 19, 2020
View reviewed changes
@l-vo l-vo force-pushed the add_bc_layer_range_message_change branch 2 times, most recently from d8f0c3f to 1136014 Compare March 19, 2020 21:05
@chalasr
Copy link
Copy Markdown
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 reviewed Mar 21, 2020
View reviewed changes
@l-vo l-vo force-pushed the add_bc_layer_range_message_change branch from 1136014 to dc9a366 Compare March 21, 2020 09:58
@l-vo l-vo force-pushed the add_bc_layer_range_message_change branch from dc9a366 to 092d85c Compare March 21, 2020 10:00
@nicolas-grekas
Copy link
Copy Markdown
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
HeahDude
HeahDude reviewed Apr 1, 2020
View reviewed changes
@fabpot
Copy link
Copy Markdown
Member

fabpot commented Aug 12, 2020

@l-vo What's the status here?

@l-vo
Copy link
Copy Markdown
Contributor Author

l-vo commented Aug 12, 2020

@fabpot from my point of view the fix itself is finished, the blocking point is more a discussion between contributors/core members about what should be done here.

@fabpot
Copy link
Copy Markdown
Member

fabpot commented Aug 12, 2020

Thank you @l-vo.

@fabpot fabpot merged commit f3753e9 into symfony:4.4 Aug 12, 2020
@fabpot
Copy link
Copy Markdown
Member

fabpot commented Aug 12, 2020

@l-vo I've merge 4.4 into 5.1, can you create a PR for 5.1 where you remove the BC layer?

@l-vo
Copy link
Copy Markdown
Contributor Author

l-vo commented Aug 12, 2020

Of course, I'm AFAK today but I'm doing it tomorrow.

@ogizanagi ogizanagi mentioned this pull request Aug 12, 2020
Merged
fabpot added a commit that referenced this pull request Aug 12, 2020
@fabpot
minor #37810 [Validator] RangeTest: fix expected deprecation (ogizanagi)
969b902 
This PR was merged into the 4.4 branch.

Discussion
----------

[Validator] RangeTest: fix expected deprecation

| Q             | A
| ------------- | ---
| Branch?       | 4.4
| Bug fix?      | no
| New feature?  | no <!-- please update src/**/CHANGELOG.md files -->
| Deprecations? | no <!-- please update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tickets       | #36140 <!-- prefix each issue number with "Fix #", no need to create an issue if none exist, explain below instead -->
| License       | MIT
| Doc PR        | N/A

Fixes https://travis-ci.org/github/symfony/symfony/jobs/717184009#L5830-L5831

Commits
-------

33c8c3a [Validator] RangeTest: fix expected deprecation
@l-vo l-vo deleted the add_bc_layer_range_message_change branch August 12, 2020 17:12
@l-vo
Copy link
Copy Markdown
Contributor Author

l-vo commented Aug 12, 2020

@fabpot Do we really want to remove the BC layer in 5.x ? In the early 5.0 and 5.1 versions, the behavior of Range changed but no deprecations was triggered. If the BC layer is removed now, some users of 5.0 and 5.1 will never be noticed that the behavior has changed. Removing it in 6.0 will allow all the users to be noticed. What is your point of view about that please ? All users must be noticed or it's and edge case with minor impact and it can be removed right now ?

@fabpot
Copy link
Copy Markdown
Member

fabpot commented Aug 13, 2020

@l-vo Right, let's remove it for 6.0.

This was referenced Aug 31, 2020
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nicolas-grekas nicolas-grekas nicolas-grekas left review comments

@stof stof stof left review comments

@xabbuh xabbuh xabbuh left review comments

@fabpot fabpot fabpot approved these changes

+2 more reviewers

@dmaicher dmaicher dmaicher left review comments

@HeahDude HeahDude HeahDude left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Deprecation Status: Reviewed Validator

Projects

None yet

Milestone

4.4

Development

Successfully merging this pull request may close these issues.

9 participants

@l-vo @dmaicher @chalasr @nicolas-grekas @fabpot @stof @xabbuh @HeahDude @carsonbot