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.

Sign up for GitHub

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

Jump to bottom

fix: pass join column options to FK #11499

Open
wants to merge 5 commits into
base: 3.3.x
Choose a base branch
from
Open

fix: pass join column options to FK #11499

wants to merge 5 commits into from

Conversation

vencakrecl
Copy link

@vencakrecl vencakrecl commented Jun 14, 2024 •
edited
Loading

Summary

Pass options to FK because of \Doctrine\DBAL\Platforms\PostgreSQLPlatform::getAdvancedForeignKeyOptionsSQL

How to use

#[ORM\JoinColumn(name: 'customer_id', options: ['deferrable' => true, 'deferred' => true])]

@vencakrecl vencakrecl force-pushed the join-column-option branch 3 times, most recently from fa25d4b to cc2b82f Compare June 14, 2024 11:22
@derrabus
Copy link
Member

Thank you. Can you please add a functional test that actually creates such a foreign key?

@derrabus derrabus changed the base branch from 3.2.x to 3.3.x June 18, 2024 12:26
@derrabus derrabus changed the base branch from 3.3.x to 3.2.x June 18, 2024 12:27
@vencakrecl
Copy link
Author

vencakrecl commented Jun 18, 2024 •
edited
Loading

@derrabus done, see f68b5e3

@derrabus
Copy link
Member

No, your test compares the generated SQL. But that SQL still does not hit a database.

@vencakrecl
Copy link
Author

@derrabus If I understood, should I add the test \Doctrine\Tests\ORM\Functional\SchemaTool\PostgreSqlSchemaToolTest here?

@derrabus
Copy link
Member

Sounds about right to me. 🙂

@derrabus derrabus changed the base branch from 3.2.x to 3.3.x June 19, 2024 10:42
@vencakrecl vencakrecl force-pushed the join-column-option branch 2 times, most recently from 6fcc563 to 21f5d1b Compare July 7, 2024 20:25
@vencakrecl
Copy link
Author

vencakrecl commented Jul 7, 2024 •
edited
Loading

@derrabus I added a functional test, see 6dfe0f3

@github-actions GitHub Actions
Copy link
Contributor

There hasn't been any activity on this pull request in the past 90 days, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days.
If you want to continue working on it, please leave a comment.

@github-actions github-actions bot added the Stale label Oct 15, 2024
@derrabus
Copy link
Member

Thanks, dear bot. This is on me. I'll try to conduct a review soon'ish. 🤞

@derrabus derrabus removed the Stale label Oct 15, 2024
prochac
prochac reviewed Nov 4, 2024
View reviewed changes
src/Tools/SchemaTool.php
@@ -712,6 +713,10 @@ private function gatherRelationJoinColumns(
$uniqueConstraints[] = ['columns' => [$quotedColumnName]];
}

if ($joinColumn->options) {
$fkOptions = array_merge($fkOptions, $joinColumn->options);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the correct behavior if key collision occurs? Keep $fkOptions or overwrite by $joinColumn-options?

https://www.amitmerchant.com/two-ways-to-merge-arrays-in-php/#gotchas

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@prochac prochac prochac left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@vencakrecl @derrabus @prochac