fix for AR case insensitive relation #17595
Conversation
framework/db/ActiveRelationTrait.php
Outdated
| * For example, mysql can join records by strings with different cases (Key = key), but AR doesn't | ||
| * So if this param is true, AR will populate these records | ||
| */ | ||
| public $caseInsensitiveKeys = false; |
There was a problem hiding this comment.
I think "caseSensitiveKeys" is better than "caseInsensitiveKeys" to avoid double negative
There was a problem hiding this comment.
After thinking about it, it seems that the whole issue could be considered a bug and the option isn't needed. Case insensitive comparison should be always used.
There was a problem hiding this comment.
@samdark AFAIK it depends on DBMS. If comparison is case sensitive on DB layer, then it should be case sensitive also in AR.
There was a problem hiding this comment.
Not really. In case of case sensitive comparison on DB layer JOIN would not return anything so there will be no population to do.
There was a problem hiding this comment.
Are you sure about that? I thought that related models are always populated by separate query, so there is no JOIN to filter results. With eager loading you can populate records for both key1 and Key1 in one query - they will mix each other if you ignore case.
There was a problem hiding this comment.
@samdark MySQL tests failed. These tests are run only on MySQL.
@onmotion This is result of eager loading optimization. Such case is handled by 2 queries:
- Find all products (
select * from product) - Find all related attributes for products selected in first query (
select * from product_attribute where product_sku in ('ARTi01', 'ARTI01')).
If you ignore case on processing related records, attributes for 'ARTi01' and 'ARTI01' will be mixed and attributes from ARTi01 will appear in ARTI01.
There was a problem hiding this comment.
Mb we can use caseSensitiveKeys = true option by default? Any proposals by @rob006 ?
There was a problem hiding this comment.
Yes. That might be a good thing to do.
There was a problem hiding this comment.
To be honest, I don't really like this solution. Whether this key should be case-sensitive depends on column collation (which could be overwritten by query). ActiveRecord does not look like a good place for caseInsensitiveKeys setting. After this change AR assumes which DBMS is used and with what collation - neither of it should be AR concern.
framework/db/ActiveRelationTrait.php
Outdated
| * For example, mysql can join records by strings with different cases (Key = key), but AR doesn't | ||
| * So if this param is true, AR will populate these records | ||
| */ | ||
| public $caseInsensitiveKeys = false; |
There was a problem hiding this comment.
After thinking about it, it seems that the whole issue could be considered a bug and the option isn't needed. Case insensitive comparison should be always used.
…aseInsensitiveKeys
samdark
added
status:code review
|
Thought more about it. While it may fix the issue in your concrete case, it is potentially adding more trouble than it's solving. Thanks for trying to solve it but I don't think we'll merge the pull request. Sorry. |
Related to yiisoft/active-record#22