Skip to content
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

Use primary key instead of binding key in case of BelongsToMany association #711

Draft
wants to merge 3 commits into
base: 6.x
Choose a base branch
from

Conversation

ravage84
Copy link

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.

Comment on lines 121 to 129
if ($association instanceof Association\BelongsToMany) {
return [
'keyField' => $association->getPrimaryKey(),
];
}

return [
'keyField' => $association->getBindingKey(),
];
Copy link
Author

@ravage84 ravage84 Jan 18, 2024

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:

Suggested change
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,
];

@ravage84 ravage84 marked this pull request as ready for review January 18, 2024 18:33
Copy link

codecov bot commented Jan 19, 2024

Codecov Report

Attention: 5 lines in your changes are missing coverage. Please review.

Comparison is base (39a7cff) 89.24% compared to head (537cd38) 90.14%.
Report is 29 commits behind head on master.

❗ Current head 537cd38 differs from pull request most recent head d4c42f9. Consider uploading reports for the commit d4c42f9 to get more accurate results

Files Patch % Lines
src/Listener/RelatedModelsListener.php 0.00% 5 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

Copy link
Member

@ADmad ADmad left a 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.
@ravage84 ravage84 marked this pull request as draft January 22, 2024 16:33
@ravage84 ravage84 changed the base branch from master to 6.x November 27, 2024 17:12
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.

2 participants