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

Fix notInRangeMessage usage for a few Range validations #14576

Merged

Conversation

diimpp
Copy link
Member

@diimpp diimpp commented Nov 24, 2022

Q A
Branch? 1.11
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? yes
Related tickets fixes #13883
License MIT

Issue
Symfony validation constraint Range instroduced new behavior since 4.4 release -- if both min and max options are defined, then notInRangeMessage will be shown and minMessage/maxMessage will be disregarded.

Example

If you will try to generate coupons for promotion with code length larger than 40, then you will see symfony standard message notInRangeMessage: This value should be between {{ min }} and {{ max }}. instead of sylius maxMessage: Coupon code must not be longer than {{ limit }} characters.
image

Case 1

<constraint name="Range">
<option name="min">1</option>
<option name="minMessage">sylius.promotion_coupon_generator_instruction.code_length.min</option>
<option name="max">40</option>
<option name="maxMessage">sylius.promotion_coupon_generator_instruction.code_length.max</option>

  • Remove minMessage, maxMessage and their translations
  • Introduce notInRangeMessage and new translation
  • Cover case with behat scenario

Case 2

new Range([
'min' => 0,
'max' => 1,
'minMessage' => 'sylius.promotion_action.percentage_discount_configuration.min',
'maxMessage' => 'sylius.promotion_action.percentage_discount_configuration.max',
'groups' => ['sylius'],
]),

  • Remove minMessage, maxMessage usage; Their translations were already removed.
  • Introduce notInRangeMessage; Translation already exists as it's shared between order/item percentage discount
  • Cover case with behat scenario

Case 3

<constraint name="Range">
<option name="min">1</option>
<option name="max">5</option>
<option name="minMessage">sylius.review.rating.range</option>
<option name="maxMessage">sylius.review.rating.range</option>
<option name="notInRangeMessage">sylius.review.rating.range</option>

Questions

  1. Does removal of sylius.promotion_coupon_generator_instruction.code_length.min and ...max messages constitute a deprecation entry? Because those translation strings were inactive (effectively deprecated by symfony upgrade) since project was upgraded to symfony 4.4 and even if somebody has custom translation over there, it has no effect whatsoever.
  2. Please advise on adding/removal of translation strings, do I need to do anything else? I do recall some translation service was in use.

@diimpp diimpp changed the title Set correct notInRangeMessage for coupon generation validation Fix notInRangeMessage usage for a few Range validations Nov 24, 2022
@diimpp diimpp marked this pull request as ready for review November 25, 2022 21:34
@diimpp diimpp requested a review from a team as a code owner November 25, 2022 21:34
Copy link
Member

@Zales0123 Zales0123 left a comment

Choose a reason for hiding this comment

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

Nice change, @diimpp! I would leave these min/max messages, as someone can use them in other places in their application. I would also add an UPGRADE file note about it and it's good to be merged 🏅 🎉

@diimpp
Copy link
Member Author

diimpp commented Dec 13, 2022

@Zales0123 Hi, can we mention removal of translation min,max messages as BC break instead? Or is it a part of hard BC policy?

My point is why leave dead codemessage, if it's unused by the app since 1.6.0. (symfony/*: ^3.4|^4.3)
While it's possible, that somebody made use of Coupon code must be at least {{ limit }} characters long. in their custom promotion validation, it doesn't look likely.

But I will update PR to keep them, if it's required.

P.S. In related #14584 I've changed sylius.review.rating.range to sylius.review.rating.not_in_range -- Do I keep old trans key as well?

@jakubtobiasz
Copy link
Contributor

Hi @diimpp!
Thanks for your contribution. We decided we should leave the message keys. We don't use them, maybe you aren't either. But it doesn't change in some projects they might be used across the system, and we shouldn't break these places. Thanks in advance!

@GSadee GSadee added the Bug Confirmed bugs or bugfixes. label Jan 31, 2023
@GSadee GSadee force-pushed the bugfix/constraint_range_not_in_range_message branch from c2d8d0e to ce986b6 Compare January 31, 2023 09:20
@GSadee GSadee force-pushed the bugfix/constraint_range_not_in_range_message branch from ce986b6 to 63e8b69 Compare January 31, 2023 13:46
@TheMilek TheMilek merged commit c477884 into Sylius:1.11 Jan 31, 2023
@TheMilek
Copy link
Member

Thank you, Dmitri! 🎉

jakubtobiasz added a commit that referenced this pull request May 2, 2023
This PR was merged into the 1.13 branch.

Discussion
----------

| Q               | A                                                            |
|-----------------|--------------------------------------------------------------|
| Branch?         | 1.13                   |
| Bug fix?        | yes                                                       |
| New feature?    | no                                                       |
| BC breaks?      | no                                                       |
| Deprecations?   | yes  |
| Related tickets | partially #14576                     |
| License         | MIT                                                          |

Continuation of #14576 based on 1.13 due deprecation and changes introduced in 1.12

```patch
--- range: Review rating must be an integer in the range 1-5.
+++ not_in_range: Review rating must be between {{ min }} and {{ max }}.
```
[Range](https://symfony.com/doc/current/reference/constraints/Range.html) constraints doesn't validate for integer type, so I've removed mention of this and was forced to remove translated strings from other languages.

> Validates that a given number or DateTime object is between some minimum and maximum.


Commits
-------
  Update product review validation's notInRangeMessage
  Restore sylius.review.rating.range translation key for BC compatibility
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Confirmed bugs or bugfixes.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants