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

Remove calls to fixSchemaElementName() #8941

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

derrabus
Copy link
Member

Yet another method that has been removed in DBAL 3.

@derrabus derrabus changed the title Remove calls to fixSchemaElementName() Remove calls to fixSchemaElementName() Aug 22, 2021
@derrabus derrabus force-pushed the remove/fix-schema-element-name branch from 354dc68 to 2226d8c Compare August 22, 2021 16:48
@derrabus derrabus force-pushed the remove/fix-schema-element-name branch from 2226d8c to ad9f67c Compare August 22, 2021 21:20
@derrabus derrabus added this to the 2.10.0 milestone Aug 23, 2021
@derrabus derrabus force-pushed the remove/fix-schema-element-name branch from ad9f67c to 8f8726b Compare August 23, 2021 14:53
greg0ire
greg0ire previously approved these changes Aug 23, 2021
@@ -672,6 +666,22 @@ private function determineIdGeneratorStrategy(AbstractPlatform $platform): int
return ClassMetadata::GENERATOR_TYPE_TABLE;
}

private function truncateSequenceName(string $schemaElementName): string
Copy link
Member

Choose a reason for hiding this comment

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

Codecov has multiple lines marked as uncovered for truncateSequenceName(). Are these lines relevant?

Copy link
Member Author

Choose a reason for hiding this comment

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

Apparently, we're not testing sequences on Oracle. 🤔

Copy link
Member

@greg0ire greg0ire Aug 24, 2021

Choose a reason for hiding this comment

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

We do test part of this method though, so there must be existing test that we might be enable for Oracle. Let's debug_print_backtrace(); to figure this out :)

Copy link
Member

Choose a reason for hiding this comment

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

Apparently it's PostgreSQL (not suprised), but I couldn't get the name of the current test. Let's use --debug.

Copy link
Member

Choose a reason for hiding this comment

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

Ok so it's Doctrine\Tests\ORM\Cache\DefaultCollectionHydratorTest::testImplementsCollectionEntryStructure 🤔 It does use a City with a GeneratedValue, but the end goal does not really seem to be testing sequences.

Copy link
Member

Choose a reason for hiding this comment

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

Apparently, we're not testing sequences anything on Oracle

FTFY 😛

@greg0ire greg0ire force-pushed the remove/fix-schema-element-name branch 2 times, most recently from 423322f to e774c0d Compare August 24, 2021 20:19
Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

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

My attempt at running the tests on Oracle: #8958

It will require more work, in the meantime I think we can merge this.

@derrabus derrabus merged commit b345488 into doctrine:2.10.x Aug 25, 2021
@derrabus derrabus deleted the remove/fix-schema-element-name branch August 25, 2021 13:06
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.

3 participants