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

Add deprecation for StringFilter format option #1061

Merged
merged 1 commit into from
Jul 12, 2020

Conversation

VincentLanglet
Copy link
Member

Subject

I am targeting this branch, because BC.

I already found weird the format option of the StringFilter.
If I look for something containing foo, there is IMHO no multiple way to check for this. It always LIKE %foo%.

Now, this option is weirder since we have the START WITH and an END WITH checks.
Why would these checks be made with %foo and foo% but the CONTAINS one could use another format than %foo% ?

That's why I think we should deprecate the format option and remove it in the next major.

Changelog

### Deprecated
- `format` option of the `StringFilter`.

core23
core23 previously approved these changes Jun 29, 2020
@core23 core23 added the minor label Jun 29, 2020
@VincentLanglet VincentLanglet requested a review from a team June 29, 2020 07:17
@phansys
Copy link
Member

phansys commented Jun 30, 2020

Why would these checks be made with %foo and foo% but the CONTAINS one could use another format than %foo% ?

I still see the single character mask as a valid use for this option. By instance LIKE f_o. Depending on the engine, I think the LIKE syntax is able to support even more masks than % or _.

@VincentLanglet
Copy link
Member Author

Why would these checks be made with %foo and foo% but the CONTAINS one could use another format than %foo% ?

I still see the single character mask as a valid use for this option. By instance LIKE f_o. Depending on the engine, I think the LIKE syntax is able to support even more masks than % or _.

I didn't know there is others mask. Maybe it's a use case but you will have
Contains -> _foo_
Start-> foo%
End -> %foo
This is not consistent. A mask option could be better to change % to _.
Contains -> _foo_
Start-> foo_
End -> _foo
But do you have a use case ?

There shouldn't be multiple way to contain something or start with something.
If you have a special rule, it would be better to have a special filter.

tests/Filter/StringFilterTest.php Outdated Show resolved Hide resolved
src/Filter/StringFilter.php Outdated Show resolved Hide resolved
@phansys
Copy link
Member

phansys commented Jun 30, 2020

It's enough for me. Just pointing that MSSQL has support for other masks.

@VincentLanglet
Copy link
Member Author

Done @phansys

@VincentLanglet VincentLanglet merged commit dd194b0 into sonata-project:3.x Jul 12, 2020
@VincentLanglet VincentLanglet deleted the deprecationFormat branch July 12, 2020 23:06
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.

3 participants