Fix BelongsToMany relationship bugs when using custom keys #2695
Conversation
# Conflicts: # tests/Models/Experience.php
src/Eloquent/HybridRelations.php
Outdated
| $foreignPivotKey = $foreignPivotKey ?: $this->getForeignKey() . 's'; | ||
| $foreignPivotKey = $foreignPivotKey ?: Str::plural($this->getForeignKey()); | ||
|
|
||
| $instance = new $related(); | ||
|
|
||
| $relatedPivotKey = $relatedPivotKey ?: $instance->getForeignKey() . 's'; | ||
| $relatedPivotKey = $relatedPivotKey ?: Str::plural($instance->getForeignKey()); |
There was a problem hiding this comment.
Pluralizing foreignPivotKey and relatedPivotKey keys in the same way in MorphToMany
There was a problem hiding this comment.
I'm afraid it's a breaking change. This (incorrect) naming convention is here since the beginning and it will break silently for applications that don't use schema validation (what I think is the majority). By changing this rule, they will start having 2 lists (childs and children) and that will be hard to fix.
Let's add a deprecation notice instead, if the pluralization is different.
Please revert in this PR and open an new one with this change.
There was a problem hiding this comment.
I agree. this can be done in 5.x version. devs need to override their foreignPivotKey and relatedPivotKey keys for just existing relationships to upgrade to 5.x version.
src/Eloquent/HybridRelations.php
Outdated
| $foreignPivotKey = $foreignPivotKey ?: $this->getForeignKey() . 's'; | ||
| $foreignPivotKey = $foreignPivotKey ?: Str::plural($this->getForeignKey()); | ||
|
|
||
| $instance = new $related(); | ||
|
|
||
| $relatedPivotKey = $relatedPivotKey ?: $instance->getForeignKey() . 's'; | ||
| $relatedPivotKey = $relatedPivotKey ?: Str::plural($instance->getForeignKey()); |
There was a problem hiding this comment.
I'm afraid it's a breaking change. This (incorrect) naming convention is here since the beginning and it will break silently for applications that don't use schema validation (what I think is the majority). By changing this rule, they will start having 2 lists (childs and children) and that will be hard to fix.
Let's add a deprecation notice instead, if the pluralization is different.
Please revert in this PR and open an new one with this change.
This reverts commit 3626c36.
|
Thank you @hans-thomas for all your work on relationships. |
|
@GromNaN You're welcome buddy. Thank you for sharing your knowledge with me❤️ I learned a lot. |
Hi there,
This PR fixes bugs in BlongsToMany relationship when custom keys defined.
I tested custom
parentKeyandrelatedKeyin #2667, but that was not enough. In this PRforeignPivotKeyandrelatedPivotKeykeys have been tested and previous tests are improved.