Skip to content

[Validator] Added {{ min }} and {{ max }} placeholder to Range constraint for minMessage and maxMessage #14785

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: 4.4
Choose a base branch
from
Open

[Validator] Added {{ min }} and {{ max }} placeholder to Range constraint for minMessage and maxMessage #14785

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8ec2e68
Added `{{ min }}` and `{{ max }}` parameters to Range constraint if b…
Jan 5, 2021
f15143c
updated description according
Jan 18, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 10 additions & 6 deletions reference/constraints/Range.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,15 +346,17 @@ maxMessage
**type**: ``string`` **default**: ``This value should be {{ limit }} or less.``

The message that will be shown if the underlying value is more than the
`max`_ option, and no `min`_ option has been defined (if both are defined, use
`notInRangeMessage`_).
`max`_ option, and **no** `min`_ option has been defined.

If **both** are defined, use `notInRangeMessage`_ or use the parameter ``{{ max }}`` instead.
Copy link
Member

Choose a reason for hiding this comment

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

The "or use the parameter {{ max }} instead" part does not look right to me.

Copy link
Author

Choose a reason for hiding this comment

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

what do you suggest instead?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe I am misreading the code, but is there actually an alternative to using notInRangeMessage?

Copy link
Author

Choose a reason for hiding this comment

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

yes exactly. That's what I am trying to say with this MR. It is just not documented..
You can still choose to use minMessage and maxMessage but with with {{ min }} respectively {{ max }} instead of {{ limit }} if you choose to set both parameters.

This is confusing, for sure. And maybe this could be changed (the behavior of the validator) but this is just status quo since 4.4

Copy link
Member

Choose a reason for hiding this comment

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

I think we should not document deprecated usages.

If both options are defined, use notInRangeMessage_ instead.

Shortening the sentence to this should IMO be enough.

Copy link
Author

Choose a reason for hiding this comment

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

Updated the text. I didn't know that the usage was deprecated. Deprecating this feature doesn't really make sense to me as I think it's a pretty useful feature to either show one of the messages and not always both.. Currently the user has the choice, in future the user is forced to always show "min" and "max" in one sentence. Not really an improvement imho.

Copy link
Contributor

Choose a reason for hiding this comment

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

If you think the deprecation should be reverted for good reasons, can you please open an issue on symfony/symfony and explain the use cases there? Thanks

In this case we should not merge this PR for now and wait the decisions to avoid back and forth here 👍🏻

@xabbuh id this is deprecated, shouldn't this be mentioned in the docs? 🧐

@cabb


You can use the following parameters in this message:

=============== ==============================================================
Parameter Description
=============== ==============================================================
``{{ limit }}`` The upper limit
``{{ limit }}`` The upper limit (if only `max`_ is defined)
``{{ max }}`` The upper limit (if both `min`_ and `max`_ are defined)
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't look right to me. I don't see where {{ max }} is actually used.

Copy link
Author

Choose a reason for hiding this comment

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

it's used here https://github.com/symfony/validator/blob/d3a98390b77b2a8854c7b5d9058588d4b45794fb/Constraints/RangeValidator.php#L114

Copy link
Member

Choose a reason for hiding this comment

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

There it's only used if both min and max are configured where you shouldn't use the maxMessage, but should use the notInRangeMessage instead.

Copy link
Author

Choose a reason for hiding this comment

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

Yes that's exactly the reason I opened this MR. Because I thought this is missing in documentation. Not knowing that one should not use it anymore. The reason for that is not clear to me though.

Copy link
Member

Choose a reason for hiding this comment

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

I would have expected that you would see a deprecation message if you were using both the min and max option together with one or both of minMessage or maxMessage. Wasn't that the case for you?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, that makes sense. I just checked my project but did not get any deprecation messages - at least when the form is submitted.

Copy link
Member

Choose a reason for hiding this comment

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

Would be interesting to see why that's the case. Which exact version do you use? Would you be able to create a small project that allows to reproduce it?

Copy link
Author

Choose a reason for hiding this comment

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

Symfony version is 5.2.0. I see what I can do to give you a full example. For now, this is the assertion I am using if this helps in any way:

    /**
     * @var mixed
     * @Assert\Sequentially(
     *     @Assert\NotNull(
     *          message="Bitte geben Sie die Anzahl an.",
     *          groups={"invoice_address"}
     *      ),
     *     @Assert\Type(
     *          type="integer",
     *          message="Die Anzahl muss eine ganze Zahl sein.",
     *          groups={"invoice_address"}
     *     ),
     *     @Assert\Range(
     *          min=5,
     *          max=1000,
     *          minMessage="Die Anzahl muss größer als {{ min }} sein.",
     *          maxMessage="Die Anzahl darf max. {{ max }} betragen",
     *          groups={"invoice_address"}
     *     )
     * )
     */
    public $quantity;
    ```

Copy link
Author

Choose a reason for hiding this comment

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

I created a sample project here https://github.com/bartrail/symfony-range-deprecation
you can start it up using vagrant and then run the curl command posted in the README.md
if you open the profiler afterwards, you will see that there is no deprecation message shown.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks, that helped to understand what is going on. In fact the deprecation is triggered. But the constraint is only instantiated once and then cached. That's why the deprecation is not displayed in the profiler. I don't know yet how we can fix that. Could you please open an issue in the code repository so that we can keep track of this?

``{{ value }}`` The current (invalid) value
=============== ==============================================================

Expand Down Expand Up @@ -394,15 +396,17 @@ minMessage
**type**: ``string`` **default**: ``This value should be {{ limit }} or more.``

The message that will be shown if the underlying value is less than the
`min`_ option, and no `max`_ option has been defined (if both are defined, use
`notInRangeMessage`_).
`min`_ option, and **no** `max`_ option has been defined.

If **both** are defined, use `notInRangeMessage`_ or use the parameter ``{{ min }}`` instead.

You can use the following parameters in this message:

=============== ==============================================================
Parameter Description
=============== ==============================================================
``{{ limit }}`` The lower limit
``{{ limit }}`` The lower limit (if only `min`_ is defined)
``{{ min }}`` The lower limit (if both `min`_ and `max`_ are defined)
``{{ value }}`` The current (invalid) value
=============== ==============================================================

Expand Down