-
-
Notifications
You must be signed in to change notification settings - Fork 344
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
Introduce "force_case_insensitivity" option and deprecate "case_sensitive" in StringFilter
#1416
Introduce "force_case_insensitivity" option and deprecate "case_sensitive" in StringFilter
#1416
Conversation
…matching in `StringFilter::filter()`" This reverts commit 532574c.
48f9e75
to
7f8df73
Compare
'case_sensitive' => null, | ||
'force_case_insensitivity' => null, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why null ?
What's the difference between null and false ?
If I understand the option, the only allowed value would be false
and true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We haven't other way to detect if this option is set or not. Currently, I'm using the value from "case_sensitive" if this option is not set.
I think we have 2 choices: add a hasOption()
method, or leave this value adding a NEXT_MAJOR
comment.
WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do you need to know if the option is set ?
If case_sensitive
is not null, use this (deprecated) value
If case_sensitive
is null, use force_case_insensitivity, which is by default to false.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because I'd like the new option to be respected if it is set, regardless the value of the deprecated option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then we could add a NEXT_MAJOR comment to change the default value to false
and remove the true ===
or true !==
checks. But It feel weird to provide a null
value and a false
value which has the same behavior. And introducing a null
default value which is already deprecated and will be changed to false in the next major...
But when I read force_case_insensitivity => false, case_sensitive => false
:
I would expect
force_case_insensitivity
won't force the case insensitivity so it will do nothing. (It never was said thatfalse
will force the case sensitivity)case_sensitive
will force the case insensitivity because it's already the behavior of this option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But when I read force_case_insensitivity => false, case_sensitive => false:
I would expectforce_case_insensitivity won't force the case insensitivity so it will do nothing. (It never was said that false will force the case sensitivity)
case_sensitive will force the case insensitivity because it's already the behavior of this option
I think the idea transmitted by the name case_sensitive
leads to wrong assumptions. In fact, that's the reason for this change.
That's why I feel like a more natural behavior to completely ignore the option if its counterpart (force_case_insensitivity
) is set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The NEXT_MAJOR
comment about the default value was added.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@VincentLanglet, I've added a comment in the upgrade document to make this behavior explicit.
7f8df73
to
2bcbf31
Compare
…tive" in `StringFilter`
2bcbf31
to
0a62b6e
Compare
Subject
Introduce "force_case_insensitivity" option and deprecate "case_sensitive" in
StringFilter
.I am targeting this branch, because these changes respect BC.
Closes #1415.
Reverts #1395.
Changelog