Skip to content

Conversation

soyuka
Copy link
Member

@soyuka soyuka commented May 17, 2016

Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets
License MIT
Doc PR dunglas/doc#5

I refactored the code a bit by removing nested conditions, hope that's ok.

@soyuka soyuka force-pushed the caseinsensitive branch from 61137d2 to 83b19c4 Compare May 17, 2016 16:17
@soyuka soyuka force-pushed the caseinsensitive branch from 83b19c4 to c638d3d Compare May 17, 2016 16:18
@sroze
Copy link
Contributor

sroze commented May 21, 2016

LGTM 👍

@dunglas dunglas merged commit 3f6ca0e into api-platform:master May 22, 2016
@dunglas
Copy link
Member

dunglas commented May 22, 2016

Thank you @soyuka

@teohhanhui
Copy link
Contributor

teohhanhui commented May 25, 2016

I don't understand why the case-sensitivity should be applied at the filter-level instead of per-property.

It's weird that this uses an instance property ($this->caseSensitive).

@teohhanhui
Copy link
Contributor

Also, what of the collations where the matching is already case-insensitive in the first place (like what is most commonly used in MySQL - utf8_unicode_ci or utf8mb4_unicode_ci)? We might want to add a note somewhere...

@soyuka
Copy link
Member Author

soyuka commented May 25, 2016

+1 @teohhanhui can you add this to the docs?

About the property we discussed it in the PR on 1.x, because it's reset here and to avoid lots of repetition to pass the flag to the caseWrap function, I think it's better to keep the instance property.

@soyuka soyuka deleted the caseinsensitive branch June 7, 2016 17:55
magarzon pushed a commit to magarzon/core that referenced this pull request Feb 12, 2017
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.

4 participants