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

[5.6] Rename comparison validation rules ("gte" to "greater_than_or_equal", etc.) #24120

Closed
wants to merge 5 commits into from

Conversation

Alymosul
Copy link
Contributor

@Alymosul Alymosul commented May 5, 2018

This pull request introduces small modification to the newly added comparison validation rules (gt, lt, gte, lte) added previously in #24091 as follows:

First, it seems that people favour the long expressive form of the rules instead of the short names, therefore i changed the rules from gt to greater_than, lt to less_than, gte to greater_than_or_equal and lte to less_than_or_equal

Also, the previous implementation added the rules to the numericRules array in the Validator class by default which will cause strange output to the validation messages being translated always with the numeric type since the validation messages in the laravel/laravel repository will be something as follows:

    'greater_than_or_equal'                  => [
        'numeric' => 'The :attribute must be greater than or equal :value.',
        'file'    => 'The :attribute must be greater than or equal :value kilobytes.',
        'string'  => 'The :attribute must be greater than or equal :value characters.',
        'array'   => 'The :attribute must have :value items or more.',
    ],

the proposed modification will add the current rule to the numeric rules if and only if the attribute under comparison is a passed numerical value, therefore the rule automatically detects if the attribute should be numeric or not instead of passing and additional numeric rule like that "Numeric|greater_than".

Finally, the PR add replacements methods for the place holders of the newly added rules.

…rt names to long names and implemented replacement of place holders for the new rules.
@tillkruss tillkruss changed the title [5.6] Small modifications on the new comparison validation rules (gt, lt, gte, lte) [5.6] Rename comparison validation rules (gte > greater_than_or_equal) May 5, 2018
@tillkruss tillkruss changed the title [5.6] Rename comparison validation rules (gte > greater_than_or_equal) [5.6] Rename comparison validation rules ("gte to greater_than_or_equal) May 5, 2018
@tillkruss tillkruss changed the title [5.6] Rename comparison validation rules ("gte to greater_than_or_equal) [5.6] Rename comparison validation rules ("gte" to "greater_than_or_equal", etc.) May 5, 2018
@laurencei
Copy link
Contributor

laurencei commented May 6, 2018

You would have to either provide support for both gte and greater_than_or_equal etc - or put this to 5.7 branch.

Because it's a breaking change if anyone has started using the new feature already.

edit: disregard

@@ -175,14 +175,14 @@ class Validator implements ValidatorContract
*
* @var array
*/
protected $sizeRules = ['Size', 'Between', 'Min', 'Max'];
protected $sizeRules = ['Size', 'Between', 'Min', 'Max', 'Gt', 'Lt', 'Gte', 'Lte'];
Copy link
Contributor

Choose a reason for hiding this comment

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

is this right? Should it be greater_than_or_equal' etc? Or is Gte` etc correct here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes you're right, should be greater_than_or_equal, etc

@Alymosul
Copy link
Contributor Author

Alymosul commented May 6, 2018

As for supporting two versions of the rules, i can do it but I think nobody has yet used the rules since the feature was added yesterday and still yet not released in any of the 5.6.x versions, therefore i think we can merge this change safely

@laurencei
Copy link
Contributor

...and still yet not released in any of the 5.6.x versions

Sorry - your right - it's not tagged yet so this could be made safely.

@@ -175,14 +175,14 @@ class Validator implements ValidatorContract
*
* @var array
*/
protected $sizeRules = ['Size', 'Between', 'Min', 'Max'];
protected $sizeRules = ['Size', 'Between', 'Min', 'Max', 'greater_than', 'less_than', 'greater_than_or_equal', 'less_than_or_equal'];
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this right again? Because it is Size (notice the upper case first letter) - but you have greater_than.

Shouldnt it be GreaterThan etc?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right again, sorry it was 4 am here and i was sleepy :D

@taylorotwell
Copy link
Member

taylorotwell commented May 6, 2018

It's pretty unlikely I will use the long forms for these rules to be honest. The short names of these are used in other popular libraries: Carbon, Lodash.js, etc.

@Alymosul
Copy link
Contributor Author

Alymosul commented May 6, 2018

I'll change it back to the short names in the same PR since it contains more than renaming

@taylorotwell
Copy link
Member

I don't necessarily like this magical detection of numerics and keeping this array of rule numeric state on the instance either. If it's numeric then mark it as numeric in your validation rules.

@taylorotwell
Copy link
Member

Tests are failing when I modify that magic stuff. I don't know what to do with this. These simple additions are already giving me a headache.

@Alymosul
Copy link
Contributor Author

Alymosul commented May 7, 2018

Will submit another PR to remove the magical detection of numerics as well as adding replacements

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

Successfully merging this pull request may close these issues.

None yet

3 participants