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 for the GT, GTE, LT, LTE comparison operators not comparing strings, arrays, or files to a numeric parameter #40309

Closed
wants to merge 1 commit into from
Closed

Conversation

sparxooo
Copy link

@sparxooo sparxooo commented Jan 8, 2022

Fix for the GT, GTE, LT, LTE operators not comparing to a numeric value (i.e. characters in strings, file length or array length). Fixes my issue #40308

For proof of bug, run the tests included in this PR without the changes to ValidatesAttributes.php file.

Removes the numeric check for value (which may not be numeric if comparing strings, files or arrays). This is the case if you do not pass another attribute to compare to (i.e. just passing in a number to compare to). getSize has the logic required to deal with what type it is.

This means that the operators work correctly, as per the Laravel documentation.

Tests fully pass on my machine.

…les to a numeric value.

Fix for the GT, GTE, LT, LTE operators not comparing to a numeric value (i.e. characters in strings, file length or array length).
@driesvints
Copy link
Member

You misunderstand how this works. These rules don't accept numbers but references to other fields.

@sparxooo
Copy link
Author

You misunderstand how this works. These rules don't accept numbers but references to other fields.

You replied to my issue, but never actually said this. I didn't realise I'd misread the docs (and even then, it will still work with numeric values when it shouldn't?).

Is this likely to break any existing logic if it were to be implemented as a new feature rather than a "bug fix" as it were?

@sparxooo sparxooo deleted the gte-lte-gt-lt-comparison-fixes branch January 11, 2022 18:21
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