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

[Doctrine][Messenger] Oracle sequences are suffixed with _seq #58557

Merged
merged 1 commit into from
Oct 18, 2024

Conversation

devloop42
Copy link

@devloop42 devloop42 commented Oct 14, 2024

Q A
Branch? 6.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #58504
License MIT

Generated sequences, by doctrine or in this case by the auto_setup of messenger, are suffixed with _seq.

@carsonbot
Copy link

Hey!

Thanks for your PR. You are targeting branch "6.4" but it seems your PR description refers to branch "6.4, and 7.1 for bug fixes".
Could you update the PR description or change target branch? This helps core maintainers a lot.

Cheers!

Carsonbot

@carsonbot carsonbot changed the title [Messenger][Doctrine] Oracle sequences are suffixed with _seq [Doctrine][Messenger] Oracle sequences are suffixed with _seq Oct 14, 2024
@devloop42 devloop42 changed the title [Doctrine][Messenger] Oracle sequences are suffixed with _seq [Messenger][Doctrine] Oracle sequences are suffixed with _seq Oct 14, 2024
Copy link
Contributor

@alexandre-daubois alexandre-daubois left a comment

Choose a reason for hiding this comment

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

Could this be covered by tests to avoid regressions?

@@ -542,9 +542,9 @@ private function addTableToSchema(Schema $schema): void

// We need to create a sequence for Oracle and set the id column to get the correct nextval
if ($this->driverConnection->getDatabasePlatform() instanceof OraclePlatform) {
$idColumn->setDefault('seq_'.$this->configuration['table_name'].'.nextval');
$idColumn->setDefault($this->configuration['table_name'].'_seq'.'.nextval');
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
$idColumn->setDefault($this->configuration['table_name'].'_seq'.'.nextval');
$idColumn->setDefault($this->configuration['table_name'].'_seq.nextval');

Copy link
Author

Choose a reason for hiding this comment

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

@alexandre-daubois : I updated that portion of code for consistency.
@rjd22 : Is this if block really needed as messenger in it's auto config process already create a sequence and trigger in case of id not specified ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, for Oracle installations the default block is needed. Else the field won't use the sequence on some installations.

@OskarStark OskarStark changed the title [Messenger][Doctrine] Oracle sequences are suffixed with _seq [Messenger][Doctrine] Oracle sequences are suffixed with _seq Oct 14, 2024
@rjd22
Copy link
Contributor

rjd22 commented Oct 17, 2024

@xabbuh Since you looked at mine can you take a look at this one? It looks good to me. If we merge this before the next release we avoid a breaking change.

Copy link
Member

@xabbuh xabbuh left a comment

Choose a reason for hiding this comment

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

works for me

@carsonbot carsonbot changed the title [Messenger][Doctrine] Oracle sequences are suffixed with _seq [Doctrine][Messenger] Oracle sequences are suffixed with _seq Oct 17, 2024
@fabpot
Copy link
Member

fabpot commented Oct 18, 2024

Thank you @devloop42.

@fabpot fabpot merged commit 5bc387b into symfony:6.4 Oct 18, 2024
7 of 9 checks passed
This was referenced Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants