Quote parts of the table name by greg0ire · Pull Request #12088 · doctrine/orm

greg0ire · 2025-07-29T20:06:41Z

In #11811, I wrongly assumed that
$tableName would never contain a dot as I was not able to write a test
that caused that to happen.

The secret recipe appears to be to define a schema and to quote the
table name.

Fixes #12041

greg0ire · 2025-07-29T20:13:05Z

Fails on Postgres with:

1) Doctrine\Tests\ORM\Functional\GH12041Test::testSchemaGenerationWithQuotedTableNameAndSchemaName
Doctrine\DBAL\Exception\TableNotFoundException: An exception occurred while executing a query: SQLSTATE[42P01]: Undefined table: 7 ERROR:  relation "myschema.file" does not exist
LINE 1: INSERT INTO "myschema.file" (id) VALUES (DEFAULT)
                    ^

/home/runner/work/orm/orm/vendor/doctrine/dbal/src/Driver/API/PostgreSQL/ExceptionConverter.php:72
/home/runner/work/orm/orm/vendor/doctrine/dbal/src/Connection.php:1456
/home/runner/work/orm/orm/vendor/doctrine/dbal/src/Connection.php:1392
/home/runner/work/orm/orm/vendor/doctrine/dbal/src/Statement.php:108
/home/runner/work/orm/orm/vendor/doctrine/dbal/src/Statement.php:133
/home/runner/work/orm/orm/src/Persisters/Entity/BasicEntityPersister.php:261
/home/runner/work/orm/orm/src/UnitOfWork.php:1057
/home/runner/work/orm/orm/src/UnitOfWork.php:402
/home/runner/work/orm/orm/src/EntityManager.php:268
/home/runner/work/orm/orm/tests/Tests/ORM/Functional/GH12041Test.php:31

https://github.com/doctrine/orm/actions/runs/16606300367/job/46978995125?pr=12088#step:7:162

Now let's attempt a fix.

greg0ire · 2025-07-29T20:22:38Z

Huh so now the test is fixed, bug another test introduced in 54b3c05 is broken.

greg0ire · 2025-07-29T21:20:00Z

I'm temporarily reverting #11811 to see if that gives me a green build, I'm starting to have doubts about that.

greg0ire · 2025-07-29T21:27:02Z

🤯 it doesn't… what the hell?

greg0ire · 2025-07-30T06:10:38Z

Ah no, the revert works, but for some reason, the test I'm adding has a side effect 🤔

greg0ire · 2025-07-30T06:26:18Z

Ah that's because the same schema name (myschema) is used in both tests.

greg0ire · 2025-07-30T06:49:19Z

Instead of altering my new test to change the schema, I'm adding a new test case to the existing test, since it tests more things.

greg0ire · 2025-07-30T06:59:53Z

src/Mapping/DefaultQuoteStrategy.php

@@ -41,9 +44,17 @@ public function getTableName(ClassMetadata $class, AbstractPlatform $platform):
            $tableName = $class->table['schema'] . '.' . $class->table['name'];
        }


Just realized this is what's causing the infamous dot. Instead of inlining quoteIdentifier(), let me try to modify this.

greg0ire · 2025-07-30T07:54:03Z

Reproduced the failure by downgrading DBAL to v3.

morozov · 2025-07-30T23:11:55Z

src/Mapping/DefaultQuoteStrategy.php

    {
        return isset($definition['quoted'])
-            ? $platform->quoteSingleIdentifier($definition['sequenceName'])
+            ? implode('.', array_map(


I have little understanding of what tradeoffs are acceptable here, so I cannot provide a reasonable feedback.

Every implode('.', $name) is a potential bug, which is why quoteIdentifier() was deprecated in the first place.

Assuming that it fixes the reported issue (which is covered by a new test), the patch looks good.

I initially wrote a separate test just for the reported issue, and the other part of the patch fixed it. This is here to fix another issue found when I decided to reuse the existing test, if I recall correctly. In any case, I think that on a patch branch, doing this is the right thing, but that on the next minor, we might want deprecate relying on that implode. This will require further analysis as to the ramification of the deprecation though.

morozov · 2025-07-30T23:15:34Z

tests/Tests/ORM/Functional/Ticket/DDC2825Test.php

        if ($this->_em->getConnection()->getDatabasePlatform()->supportsSchemas()) {
-            $fullTableName = sprintf('%s.%s', $expectedSchemaName, $expectedTableName);
+            $fullTableName = sprintf(
+                $isQuoted ? '"%s"."%s"' : '%s.%s',
+                $expectedSchemaName,
+                $expectedTableName,
+            );
        } else {


This kind of test looks... not really useful. On the one hand, it's an integration test that that covers implementation details, which in turn depend on the environment, so it's hard to reason whether it provides the right feedback or not. On the other, there are two levels of code branching (if and ?), but a proper test shouldn't have any conditional logic.

Would it make sense to convert it to a unit test and make the expected values explicit?

Also, the dependency of the result on supportsSchemas() looks a bit inadequate. The result should depend only on the input (i.e. the class metadata).

Good points. To elaborate on your last point, the test is skipped unless the database supports schemas:

orm/tests/Tests/ORM/Functional/Ticket/DDC2825Test.php

Lines 28 to 30 in 0f32569

if (! $platform->supportsSchemas()) {

self::markTestSkipped('This test is only useful for databases that support schemas or can emulate them.');

}

So it makes no sense to have this if statement:

orm/tests/Tests/ORM/Functional/Ticket/DDC2825Test.php

Lines 44 to 48 in 0f32569

if ($this->_em->getConnection()->getDatabasePlatform()->supportsSchemas()) {

$fullTableName = sprintf('%s.%s', $expectedSchemaName, $expectedTableName);

} else {

$fullTableName = sprintf('%s__%s', $expectedSchemaName, $expectedTableName);

}

, since a41c6d3

This test definitely needs some love, and I will start by removing this if statement.

I will look into doing the improvements you mentioned, but for now, here is a PR that addresses the issue in my last message. #12094

@morozov I did the conversion to unit tests, please take another look.

In aa141bf, I wrongly assumed that $tableName would never contain a dot as I was not able to write a test that caused that to happen. The secret recipe appears to be to define a schema and to quote the table name. To fix it for the table name, I am calling quoteSingleIdentifier() before doing the concatenation between schema name and table name. To fix it for the sequence name, which seems only useful when using DBAL 3 for some reason, I reuse some of the logic of the deprecated method. Fixes doctrine#12041

That test was testing too many thing and not really making it clear what the expected output was, given some output. Instead, let us create 2 tests, each pertaining to the class under test.

This comment is rendered useless by the phpdoc comment below it. Instead, we can comment on what exactly "quoted" means.

morozov

Looks much better. Thanks!

stlrnz · 2025-08-05T05:35:49Z

Hey, coming from #12041, I just wanted to say thank you for your hard work to fix the issue! I really appreciate this :)

greg0ire force-pushed the quote-parts branch from e16a5f6 to 3e308de Compare July 29, 2025 20:16

greg0ire force-pushed the quote-parts branch 2 times, most recently from 27eaf43 to d0d1a71 Compare July 29, 2025 20:55

greg0ire force-pushed the quote-parts branch from f20a937 to 53bf9cb Compare July 30, 2025 06:48

greg0ire commented Jul 30, 2025

View reviewed changes

greg0ire force-pushed the quote-parts branch from 53bf9cb to ba07416 Compare July 30, 2025 07:01

greg0ire force-pushed the quote-parts branch from ba07416 to d192592 Compare July 30, 2025 08:33

greg0ire added the Bug label Jul 30, 2025

greg0ire marked this pull request as ready for review July 30, 2025 08:41

greg0ire requested review from SenseException, derrabus and morozov July 30, 2025 08:41

SenseException previously approved these changes Jul 30, 2025

View reviewed changes

morozov reviewed Jul 30, 2025

View reviewed changes

morozov previously approved these changes Jul 31, 2025

View reviewed changes

greg0ire added 3 commits July 31, 2025 21:49

Convert test into 2 unit tests

1b98be3

That test was testing too many thing and not really making it clear what the expected output was, given some output. Instead, let us create 2 tests, each pertaining to the class under test.

Improve comment

85d66de

This comment is rendered useless by the phpdoc comment below it. Instead, we can comment on what exactly "quoted" means.

greg0ire dismissed stale reviews from morozov and SenseException via 85d66de July 31, 2025 20:55

greg0ire force-pushed the quote-parts branch from d192592 to 85d66de Compare July 31, 2025 20:55

derrabus approved these changes Aug 4, 2025

View reviewed changes

morozov approved these changes Aug 5, 2025

View reviewed changes

greg0ire merged commit 64444dc into doctrine:3.5.x Aug 5, 2025
85 checks passed

greg0ire deleted the quote-parts branch August 5, 2025 06:05

greg0ire added this to the 3.5.1 milestone Aug 5, 2025

github-actions bot mentioned this pull request Aug 5, 2025

Merge release 3.5.1 into 3.6.x #12105

Merged

		@@ -41,9 +44,17 @@ public function getTableName(ClassMetadata $class, AbstractPlatform $platform):
		$tableName = $class->table['schema'] . '.' . $class->table['name'];
		}

	if (! $platform->supportsSchemas()) {
	self::markTestSkipped('This test is only useful for databases that support schemas or can emulate them.');
	}

	if ($this->_em->getConnection()->getDatabasePlatform()->supportsSchemas()) {
	$fullTableName = sprintf('%s.%s', $expectedSchemaName, $expectedTableName);
	} else {
	$fullTableName = sprintf('%s__%s', $expectedSchemaName, $expectedTableName);
	}

Conversation

greg0ire commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

greg0ire commented Jul 29, 2025

Uh oh!

greg0ire commented Jul 29, 2025

Uh oh!

greg0ire commented Jul 29, 2025

Uh oh!

greg0ire commented Jul 29, 2025

Uh oh!

greg0ire commented Jul 30, 2025

Uh oh!

greg0ire commented Jul 30, 2025

Uh oh!

greg0ire commented Jul 30, 2025

Uh oh!

greg0ire Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Jul 30, 2025

Uh oh!

morozov Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

morozov Jul 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Jul 31, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

greg0ire Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

morozov left a comment

Choose a reason for hiding this comment

Uh oh!

stlrnz commented Aug 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

greg0ire commented Jul 29, 2025 •

edited

Loading

greg0ire Jul 30, 2025 •

edited

Loading

morozov Jul 30, 2025 •

edited

Loading

morozov Jul 30, 2025 •

edited

Loading

greg0ire Jul 31, 2025 •

edited

Loading