- 
                Notifications
    
You must be signed in to change notification settings  - Fork 144
 
Use primary key instead of binding key in case of BelongsToMany association #711
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
base: 6.x
Are you sure you want to change the base?
Conversation
| if ($association instanceof Association\BelongsToMany) { | ||
| return [ | ||
| 'keyField' => $association->getPrimaryKey(), | ||
| ]; | ||
| } | ||
| 
               | 
          ||
| return [ | ||
| 'keyField' => $association->getBindingKey(), | ||
| ]; | 
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.
Alternatively, the code could look like this:
| if ($association instanceof Association\BelongsToMany) { | |
| return [ | |
| 'keyField' => $association->getPrimaryKey(), | |
| ]; | |
| } | |
| return [ | |
| 'keyField' => $association->getBindingKey(), | |
| ]; | |
| $keyField = $association->getBindingKey(); | |
| if ($association instanceof Association\BelongsToMany) { | |
| $keyField = $association->getPrimaryKey(); | |
| } | |
| return [ | |
| 'keyField' =>$keyField, | |
| ]; | 
          Codecov ReportAttention:  
 
 
 Additional details and impacted files@@             Coverage Diff              @@
##             master     #711      +/-   ##
============================================
+ Coverage     89.24%   90.14%   +0.90%     
+ Complexity      381      370      -11     
============================================
  Files            36       36              
  Lines          1181     1157      -24     
============================================
- Hits           1054     1043      -11     
+ Misses          127      114      -13     ☔ View full report in Codecov by Sentry.  | 
    
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.
Test case required.
BTW the docs say it's the column of the "current" table, not junction table.
bindingKey: The name of the column in the current table, that will be used for matching the foreignKey. Defaults to the primary key.
P.S. The master branch is for CakePHP 5, I can backport it later.
Adjust tests where necessary with users fixture or setting the new `user_id` field.
| 
           ping @ravage84  | 
    
| 
           I believe so, yes.  | 
    
Belongs to many associations have a junction table or through model:
https://book.cakephp.org/4/en/orm/associations.html#belongstomany-associations
A field named like the binding key does not necessarily exist on the target table but only on the junction table.
This change is necessary to prevent the exception introduced by the backport of cakephp/cakephp#17340 to be thrown in such cases.
cakephp/collection@3897169#diff-81970ebd43ab052712b24c8315dfd428fea23a3dcea21316e27d9ce9a2551b83R596-R599
I encountered this after upgrading an application from CakePHP 4.4.x. to 4.5.x.
The reason why this has worked before 4.5 is mentioned at cakephp/cakephp#17340 (comment).
This change is likely also necessary for the CRUD branch for CakePHP 5.x.