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

Introduce "force_case_insensitivity" option and deprecate "case_sensitive" in StringFilter #1416

Merged
merged 2 commits into from
Apr 16, 2021

Conversation

phansys
Copy link
Member

@phansys phansys commented Apr 14, 2021

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

### Added
- "force_case_insensitivity" option to `StringFilter` in order to force the database to ignore the case sensitivity when matching filters.

### Deprecated
- "case_sensitive" option in `StringFilter`.

…matching in `StringFilter::filter()`"

This reverts commit 532574c.
@phansys phansys added the minor label Apr 14, 2021
@phansys phansys requested a review from a team April 14, 2021 21:50
@phansys phansys marked this pull request as ready for review April 14, 2021 21:50
@phansys phansys force-pushed the force_case_insensitivity branch 2 times, most recently from 48f9e75 to 7f8df73 Compare April 14, 2021 22:03
'case_sensitive' => null,
'force_case_insensitivity' => null,
Copy link
Member

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

Copy link
Member Author

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?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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 that false will force the case sensitivity)
  • case_sensitive will force the case insensitivity because it's already the behavior of this option

Copy link
Member Author

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 expect

force_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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@phansys phansys force-pushed the force_case_insensitivity branch from 7f8df73 to 2bcbf31 Compare April 14, 2021 22:54
@phansys phansys requested a review from a team April 14, 2021 22:55
VincentLanglet
VincentLanglet previously approved these changes Apr 14, 2021
@VincentLanglet VincentLanglet requested a review from a team April 14, 2021 23:18
@VincentLanglet VincentLanglet merged commit fe62f42 into sonata-project:3.x Apr 16, 2021
@phansys phansys deleted the force_case_insensitivity branch April 17, 2021 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Introduce "force_case_insensitivity" option and deprecate "case_sensitive"
3 participants