- Merge request !373Issue #3200743: Decouple identifier management from database Connection, introduce an IdentifierHandler class → (Open) created by mondrake
- Open on Drupal.org →Environment: PHP 8.2 & MySQL 8last update
almost 2 years ago Not currently mergeable. - last update
almost 2 years ago Custom Commands Failed - last update
almost 2 years ago 19,868 pass, 2,915 fail - last update
almost 2 years ago 19,868 pass, 2,915 fail - last update
almost 2 years ago 29,780 pass, 9 fail - last update
almost 2 years ago Custom Commands Failed - last update
almost 2 years ago 20,010 pass, 2,905 fail - last update
almost 2 years ago 29,901 pass, 1 fail - last update
almost 2 years ago 29,892 pass, 5 fail - Status changed to Active
2 months ago 9:40pm 6 March 2025 - 🇮🇹Italy mondrake 🇮🇹
So, here is the idea.
Developers should not be worried by exceeding identifiers length or using wrong identifiers; the database API should be self-sufficient in receiving input and cleansing/transforming it in machine usable information; failing as a last resort.
Therefore:
- Table names, database name, column names, index names in a db are 'identifiers'.
- 'Identifiers' are subject to db-specific limitations (length, chars allowed, etc)
- In Drupal, 'Identifiers' are value objects. Each identifier has:
- a type (e.g. table, column, alias, etc)
- a raw value (e.g. 'config', 'key_value', 'mydb."key_value"', 'with$$invalidchar', etc)
- a canonical value, i.e. the raw value cleansed of unallowed characters (e.g. from the above 'config', 'key_value', 'mydb.key_value', 'withinvalidchar', etc)
- a machine value, i.e. the canonical value transformed to be used on the db-specific SQL query - including prefixing and/or shortening (e.g. from the above, assuming a prefix 'abba': '"abbaconfig"', '"abbakey_value"', '"mydb"."key_value"', '"abbawithinvalidchar"', etc)
- each db-driver implements an 'IdentifierHandling' class that parses/transforms a raw value into the relevant value object, including a local cache to prevent processing again and again each raw value.
This works now for MySql and SQLite; the MySql implementation shortens table names that are longer than 64 characters using hashing techniques.
PgSql will follow with its own 63 chars max limitation.
In follow ups, this approach could be extended from tables to columns, aliases, indexes etc.
- 🇮🇹Italy mondrake 🇮🇹
Added pgsql conversion. Tests are now green on all 3 core drivers. The concept is in and implemented; now to cleaning, BC and documenting.
- 🇮🇹Italy mondrake 🇮🇹
The machineName in the value objects should be stored unquoted. On that.
- 🇳🇱Netherlands daffie
I support the basic solution. Great work!
-
The DX can be improved. Do we really need to deprecate methods like Connection::escapeTable() and Connection::getPrefix(). Can we keep them? Changes like:
- $this->connection->getPrefix() + $this->connection->identifiers->tablePrefixor
- $this->connection->escapeTable($this->table) + $this->connection->identifiers->table($this->table)->forMachine()Those changes do not make for an improved developer experience. Lets keep those methods as a wrapper for the new implementation. Maybe we need to add some more wrapper methods to improve the DX. It is great that we have an improved identifier management solution. Now lets hide it from them as much as possible. It works just a little bit better then before. For most of them that is all they need to know and maybe they do not need to know at all.
-
The default database driver for Drupal is MySQL/MariaDB. Let the default implementation in the namespace
Drupal\Core\Database\Identifierbe for just MySQL/MariaDB. The specific changes that are needed for PostgreSQL and SQLite need to be done in their database driver modules. Like no "schema" stuff in the namespaceDrupal\Core\Database\Identifier. Other database drivers that are not supported by Drupal core, have their own specific stuff they need to override. Let use the database drivers for PostgreSQL and SQLite as an example for those contrib database drivers. The current solution is limited to making it work for the by Drupal core supported database drivers.
-
The DX can be improved. Do we really need to deprecate methods like Connection::escapeTable() and Connection::getPrefix(). Can we keep them? Changes like:
- 🇮🇹Italy mondrake 🇮🇹
Filed follow-up 📌 [PP-1] Use Table identifier value objects instead of table name strings in database Schema classes Postponed
- 🇮🇹Italy mondrake 🇮🇹
#24 thanks Daffie for your review!
1) for sure we can undeprecate those, or postpone removal to a later major (D13+). But why would that be desirable? Besides that those 'wrappers' would be redundant, what would someone call those for, given that the value objects encapsulate the result of those wrappers? Besides, once we move to value objects then we could pass them as arguments for methods instead of strings and the reason for such wrappers would simply not hold anymore - see for example [ #3514117] and how sqlite's Schema class would benefit.
Supposing a prefix 'foo' is required, given that the Table identifier implements \Stringable and it returns the machine name identifier:
$table = $this->connection->identifier->table('try_me'); echo "SELECT * FROM $table";would return, without any further processing:
for SQLite:
SELECT * FROM "foo"."try_me"for MySQL
SELECT * FROM "footry_me"for PostgreSql (supposing a 'bar' schema is in use)
SELECT * FROM "bar"."footry_me"2) that's exactly what the MR is doing no?
Identifier/Database,Identifier/Schema,Identifier/Tableare value objects, defined final - they only carry the values. The values themselves are determined by each driver'sIdentifierHandlerclass, that parses the 'raw value' string identifier and instantiates the value objects with the needed values. Each driver'sIdentifierHandlerextends from the abstractIdentifierHandlerBasewhich has the 'regular' implementation - and all drivers need diversions one way or another, say for the different max lenght, or for how tables are prefixed, or for adding schemas etc. The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇮🇹Italy mondrake 🇮🇹
File follow up 📌 [PP-1] Allow using Table identifier value objects in database Query classes Postponed .
- Status changed to Needs work
2 days ago 1:53pm 12 May 2025 The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".
This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.
Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.
- 🇨🇦Canada Liam Morland Ontario, CA 🇨🇦
liam morland → made their first commit to this issue’s fork.
- 🇺🇸United States mradcliffe USA
Switching to Needs work for phpunit and code quality failures.
- 🇬🇧United Kingdom joachim
I feel this might clash a bit with ✨ add long table name abbreviation to Database API Active .
Is the MR here just rejecting names which are too long? Could it leave space for that issue to handle them?
- 🇮🇹Italy mondrake 🇮🇹
#35: this is a totally alternative approach, that removes the need for the other issue.
Too long names are automatically shortened here to match the db limitations.
- 🇺🇸United States mradcliffe USA
I came up with a rough outline for the change record after reviewing the changes.
- Adds a new nullable argument to the base Connection __construct method that takes an IdentifierHandler.
- Database drivers should implement IdentifierHandler (details) and pass this into Connection::parent::__construct().
- Implement at least getMaxLength if extending IdentifierHandlerBase.
- Method calls and properties that need to be changed:
- getPrefix() should use identifiers->tablePrefix
- $identifierQuotes should use identifiers->identifierQuotes
- $prefix should use identiferies->tablePrefix
- $escapedTables is no longer used
- $tablePlaceholderReplacements should use ????
- escapeDatabase() should use identifiers->database()
- Any time need to use a table or prefix in code this should be identifiers->table
- escapeTable() should be identifiers->table. escapeTable() is no longer needed when using connection->query()? A database drive may need to use schema()->quotedMachineName . $tableIdentifier->quotedMachineName?
It looks like this also applies to any usages of
querythat use escapeTable manually in addition to database driver modules.Finally a question. If a site provided a prefix and used a non-core database driver would that site stop working because $prefix isn't set anywhere by that database driver (if it relied on the protected property)?