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

Test quoted qualified name #11812

Closed
wants to merge 1 commit into from

Conversation

greg0ire
Copy link
Member

The current behavior is that a quote in the table name should result in the name being considered "quoted" and be passed to DBAL's quoteIdentifier() method, which should quote each identifier individually (so the dot is still interpreted as a schema separator).

This is prompted by doctrine/dbal#6590: let's make sure we don't break the current behavior when addressing the deprecation.

@greg0ire greg0ire force-pushed the test-quote-identifiers branch 9 times, most recently from 7dc00e1 to ac953ad Compare January 26, 2025 19:59
@greg0ire greg0ire marked this pull request as draft January 26, 2025 20:12
@greg0ire greg0ire force-pushed the test-quote-identifiers branch 3 times, most recently from b9099df to 601b42e Compare January 27, 2025 17:41
The current behavior is that a quote in the table name should result in
the name being considered "quoted" and be passed to DBAL's
quoteIdentifier() method, which should quote each identifier
individually (so the dot is still interpreted as a schema separator).
@greg0ire greg0ire force-pushed the test-quote-identifiers branch from 601b42e to 53a4ebc Compare January 27, 2025 17:53
@greg0ire greg0ire marked this pull request as ready for review January 27, 2025 18:06
self::assertContains(
'inventory',
$schemaManager->listSchemaNames(),
'"inventory" should be interpreted as a schema name, even when quoted.'
Copy link
Member

Choose a reason for hiding this comment

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

Not sure how to interpret the "even when quoted" clause. When the identifiers are unquoted, then it's not clear if the dot is part of the unqualified name or it's a delimiter between the qualifier and the unqualified name. With quoted identifier, it's clear that these are individual identifiers.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, right, I lost track of the goal while trying to get to something that passes the tests, but I think the test might be pointless. At first, I tried using ticks around the whole name, like so:

`inventory.Products`

My goal was to demonstrate that we would still get an inventory schema, and not an inventory.Products table in the public schema even if the name was quoted (with backticks, as documented here). It didn't work out, and I noticed that the schema and the table name got separated before getting to this method:

$tableName = $class->table['name'];
if (! empty($class->table['schema'])) {

As specified here:

* @phpstan-var array{
* name: string,
* schema?: string,
* indexes?: array,
* uniqueConstraints?: array,
* options?: array<string, mixed>,
* quoted?: bool
* }
*/
public array $table;

Copy link
Member

@morozov morozov Jan 28, 2025

Choose a reason for hiding this comment

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

I see. I think, the upshot of this is that historically both ORM and DBAL didn't respect the semantics of quoted identifiers in full. Specifically, when parsing names. Quoted identifiers are/were only used to represent the names that are keywords (to render them as quoted in SQL). It was the user's responsibility to quote keywords like "table".

It looks like it's impossible at this moment (in neither of ORM and DBAL) to define a name that contains a dot. It will be always interpreted as a delimiter between identifiers.

@greg0ire greg0ire marked this pull request as draft January 28, 2025 08:01
@greg0ire greg0ire closed this Jan 28, 2025
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