-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
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
fix for AR case insensitive relation #17595
Closed
Closed
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit
Hold shift + click to select a range
4587bdb
fix for AR case insensitive relation
3499f75
Merge branch 'master' of git://github.com/yiisoft/yii2 into caseInsen…
onmotion e605caf
Update ActiveRelationTrait.php
samdark c10180a
Merge branch 'caseInsensitiveKeys' of github.com:onmotion/yii2 into c…
onmotion ca5ee3d
rm caseInsensitiveKeys option and test changes
onmotion File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
<?php | ||
|
||
|
||
namespace yiiunit\data\ar; | ||
|
||
|
||
/** | ||
* Class Product | ||
* @package yiiunit\data\ar | ||
* | ||
* @property string $sku | ||
* @property string $title | ||
* @property ProductAttribute[] $productAttributes | ||
*/ | ||
class Product extends ActiveRecord | ||
{ | ||
public static function tableName() | ||
{ | ||
return 'product'; | ||
} | ||
|
||
/** | ||
* @return ProductAttribute[]|\yii\db\ActiveQuery | ||
*/ | ||
public function getProductAttributes() | ||
{ | ||
return $this->hasMany(ProductAttribute::className(), ['product_sku' => 'sku']); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
<?php | ||
|
||
|
||
namespace yiiunit\data\ar; | ||
|
||
|
||
/** | ||
* Class Product | ||
* @package yiiunit\data\ar | ||
* | ||
* @property int $id | ||
* @property string $product_sku | ||
* @property string $value | ||
* @property Product $product | ||
*/ | ||
class ProductAttribute extends ActiveRecord | ||
{ | ||
public static function tableName() | ||
{ | ||
return 'product_attribute'; | ||
} | ||
|
||
/** | ||
* @return Product|\yii\db\ActiveQuery | ||
*/ | ||
public function getProduct() | ||
{ | ||
return $this->hasOne(Product::className(), ['sku' => 'product_sku'])->inverseOf('productAttributes'); | ||
} | ||
|
||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think "caseSensitiveKeys" is better than "caseInsensitiveKeys" to avoid double negative
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andKey1
in one query - they will mix each other if you ignore case.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.
@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:
select * from product
)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 inARTI01
.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.
@onmotion what do you think?
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.
Mb we can use caseSensitiveKeys = true option by default? Any proposals by @rob006 ?
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.
Yes. That might be a good thing to do.
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.
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.