Replace array-based DB Schema API with a value object structure

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.

Worth noting we're doing something similar but not quite the same over at #3397622: Adding GIN and GIST indexes to PostgreSQL databases.

Per some feedback in Drupal Slack today, it sounds like the ideal state would be to use events a la #3410480: [META] Use events in Database Schema operations but we're postponed enough as it is on related issues. At the very least, this demonstrates how we can use objects in some circumstances inside of "ArrayPI" without breaking BC and enabling innovation.

I like that we add and later change to the use of an object based schema for table definitions. With that said, I do have some questions/objections and hopefully they can be addressed. The main one is that all the new objects are based on classes that are final classes. That makes that they are not extendable. Which is great for MySQL/MariaDB as no new functionality will be added to those databases. However for PostgreSQL we would like to add GIST and GIN indexes (#3397622: Adding GIN and GIST indexes to PostgreSQL databases) and maybe others in the future. I do not see how the PostgreSQL module can do that. It would be great if we could allow database driver modules to extend/override the base classes.

Read through code and it looks nice!
Approach with Enums looks like should give a room for extension, so new kinds of indexes looks doable

I started a draft change record.

I would like to suggest a couple of changes to support database specific stuff like GIST and GIN indexes on PostgreSQL.

1. Add on the class Drupal\Core\Database\SchemaDefinition\Index two new variables. The first is $dbSpecificType and is the same as the one in the class Column. The second is $dbSpecificExtra is the same as $dbSpecificType only extra string with config data for creating the index.
An index that should be a GIST index for PostgreSQL could then be defined as:

   new Index(
     name: 'ages',
     columns: ['age'],
     dbSpecificType: [
       'pgsql' => 'gist',
     ],
     dbSpecificExtra: [
       'pgsql' => 'gist_trgm_ops',
     ],
   );
 

Other database drivers will just ignore the PostgreSQL specific stuff.

2. Lets add an hook call to let the current database driver module to override ConvertDefinition::schemaToArray($schema). When we have a column with something like:

   new Column(
     name: 'db_timestamp',
     description: "The database timestamp",
     dbSpecificType: [
       'mysql' => 'timestamp',
       'pgsql' => 'timestamp',
       'sqlite' => 'datetime',
     ],
     notNull: FALSE,
     default: NULL,
   );
 

We get after calling ConvertDefinition::schemaToArray() for MySQL something like:

   'db_timestamp' => [
     'description' => "The database timestamp",
     'type' => 'timestamp',
     'not null' => FALSE,
     'default' => NULL,
   ];
  

For SQLite the result would be:

   'db_timestamp' => [
     'description' => "The database timestamp",
     'type' => 'datetime',
     'not null' => FALSE,
     'default' => NULL,
   ];
  

@mondrake: What is your opinion?

Edit: Maybe we do not need the hook for point #2.

@mondrake: For #23, yes a very good idea!

For my not reading the meta issue. I should have read the issue and I did not. My bad, and yes I agree with your proposal in the Meta issue. I have removed the tag "Needs subsystem maintainer review".

I have my a couple of remarks on the MR. Let me know what you think of them.

Again good work and a good proposal! Feel free to remove the tag for the framework managers review.

Again good work and a good proposal! Feel free to remove the tag for the framework managers review.

I think this still needs framework manager review, I don't see one.

From comment #22:

The idea is (1) SchemaDefinition (abstract) --> (2) dispatchSchemaDefinitionEvent (abstract) --> (3) subscribeSchemaDefinitionEvent (DBMS-specific) --> Schema (DBMS-specific)

I agree with the idea. The SchemaDefinition should be abstract. For the new object definition I think that is the right solution. However for the array definition we run into problems. With the Columns, Indexes and Tables we have the property $dbSpecificExtra and it is added to the array definition in different ways. The different ways of adding the "extra" data is to me all a bit hacky and we are only doing that to make the array definition also abstract. As we are going to deprecate and drop the array definition anyway, lets make the array definition database driver specific. We will end up with a prettier solution for the object definition and we do not need to do hacky solutions in the array definitions.

For the Column class:

new Column(
  name: 'db_timestamp',
  description: "The database timestamp",
  dbSpecificExtra: [
    'mysql' => [
      'mysql_type' => 'timestamp',
    ],
    'pgsql' => [
      'pgsql_type' => 'timestamp',
    ],
    'sqlite' => [
      'sqlite_type' => 'datetime',
    ],
  ],
  notNull: FALSE,
  default: NULL,
);

With the database specific type we are doubling down with database type. Not very pretty. We can do better with:

new Column(
  name: 'db_timestamp',
  description: "The database timestamp",
  dbSpecificExtra: [
    'mysql' => [
      'type' => 'timestamp',
    ],
    'pgsql' => [
      'type' => 'timestamp',
    ],
    'sqlite' => [
      'type' => 'datetime',
    ],
  ],
  notNull: FALSE,
  default: NULL,
);

The resulting array definition for SQLite:

'db_timestamp' => [
  'description' => "The database timestamp",
  'type' => 'datetime',
  'not null' => FALSE,
  'default' => NULL,
];

For the Table class:

new Table(
  dbSpecificExtra: [
    'mysql' => [
      'mysql_engine' => 'InnoDB',
      'mysql_character_set' => 'utf8mb4',
    ],
  ],
);

The database specific keys are directly added to the array definition. You can only hope that a key setting for one database does not break something in another database.

For the Index class:

new Index(
  name: 'foo',
  columns: ['job'],
  dbSpecificExtra: [
    'pgsql' => [
      'type' => 'gist',
      'operator' => 'gist_trgm_ops',
    ],
    'time_warp' => [
      'jump' => 'to_the_left',
      'step' => 'to_the_right',
    ],
  ],
);

'indexes' => [
  'foo' => ['job'],
],
'indexes extra' => [
  'foo' => [
    'pgsql' => [
      'type' => 'gist',
      'operator' => 'gist_trgm_ops',
    ],
    'time_warp' => [
      'jump' => 'to_the_left',
      'step' => 'to_the_right',
    ],
  ],
],

The database specific stuff is added in a second array with the key "indexes extra". Also not a very pretty solution. With a database specific array definition we can do much better.

The resulting array definition for PostgreSQL:

'indexes' => [
  'foo' => [
    'columns' => ['job'],
    'type' => 'gist',
    'operator' => 'gist_trgm_ops',
  ],    
];

@mondrake: What is your opinion?

Edit: If a framework manager agrees with your solution, we will do your solution.

Why are we introducing the ::Undefined everywhere? Isn't that the same as allowing NULLs?

Re #32 - yeah but that is only for default value. Imo we should have a DefaultValue class rather than Property::Undefined everywhere.

I agree with @mondrake re #28. I think we should not be touching the array definitions here at all. In fact I do not think we should be adding indexes extra support here at all. That belongs in its own issue and deserves its own discussions.

I'm not sure telling people to do \Drupal\Core\Database\Schema\ConvertDefinition::schemaToArray($schema) - in hook_schema. Ie. from the CR:

/**
 * Implements hook_schema().
 */
function system_schema(bool $asArray = TRUE): array {
   $schema['my_table'] = new Table(
   );

   return $asArray ? \Drupal\Core\Database\Schema\ConvertDefinition::schemaToArray($schema) : $schema;
}

Imo this is wrong. The caller should process the definitions and if the schema is not a Table it call the converter for you. In a follow-up we can add a new issue to trigger a deprecation if doing this.

EDIT: Race condition with @alexpott #34. But still relevant for the followup.

@mondrake: +100 to make all this thoroughly typed!

But kittens (and me) suffering a lot from the 'extra' keys bringing back the array-PI that was about to be eradicated in the first place.

class-string pattern to the rescue. I guess you saw it before:

In this issue it is implemented like this:

new Table(
  extra: [
    MySqlTable::class => new MySqlTable(
      engine: MySqlEngine::InnoDB,
      characterSet: CharSet::utf8mb4,
    ),
  ],
);

Keying by class-string allows PhpStan, PhpStorm, and friends to infer types:

  /**
   * @template T
   * @param array<class-string<T>, T> $extra
   */
// Class can now be final as $extra allows infinite extensibility.
final class Table {
  public function __construct(
    private readonly array $extra,
  ) {}
}

Consuming code can now do:

$mySqlTableExtra = $table->extra[MySqlTable::class];
if ($mySqlTableExtra) {
  // This is NOT necessary, because of class-string.
  assert($mySqlTableExtra instanceof MySqlTable);
  ...
}

The thing that bothers me about #35 is that module authors are going to have to make 2 changes to their hook schema in order to navigate through the deprecations. One convert the thing and support the $as_array thing and then another to remove all of that (and potentially a middle step when the default changes). I think we need a deprecation plan posted on this issue and we need to work out how to have the minimum number of steps for a module author to take. In an ideal world... hook schema all arrays and then they convert to use the objects and then do nothing else.

If we've got a difference between a NULL and an empty string value for description - then we shouldn't. That's feels like something completely unnecessary - for the reader of the description there is no difference both are the same.

It all looks good to me.
For me it is RTBC.

Left a comment on the MR.

Also, we have a problem because the BC policy says:

# Function arguments
Named arguments should not be considered part of the public API.

and these value objects surely can NOT change their argument names.

The link in the docs is incorrect.

The link to the docs section needs to be made with @link - see https://project.pages.drupalcode.org/coding_standards/php/documentation/

I think the CR needs an update for the default value changes...

I think we should also file an issue and bake support for this in https://www.drupal.org/project/schema ASAP - we are obviously helped out by the fact there is no 11.x release yet :)

All the changes look good to me.
The followup has been created.
Back to RTBC.

@daffie GitLab is saying to me "Change requests must be approved by the requesting user" followed by your picture. I think you need to resolve the suggested changes.

There's a rebase/merge from 11.x needed again.

The 'See the Schema API documentation for details on schema definition' text should use a @link tag to make a link to another docs section.

Posted a couple of small comments on the MR - once addressed this can go back to RTBC so we can land early in the 11.4.x cycle, Thanks!

> See the Schema API documentation for details on schema definition
* structures.

I keep saying this needs to be made into a proper link to the 'schemaapi' section with a @link tag.

> @joachim can you please suggest the changes you believe are needed and I will include them.

I've already explained what needs to be done -- use the @link tag. I also gave a link to the documentation that explains how to do it.

That needs to be fixed for this to pass the documentation gate.

> #71 I will be waiting for someone with expertise in this area to suggest in detail the changes required

No expertise required. There is an example on the page I linked to. It's this:

* See also @link topic_foo Foo topic, @endlink and the

I think most of @alexpott's review comments are addressed except “This also makes me think that maybe a schema object which implements ArrayAccess might actually be the best BC way to go here.”.

I reviewed database.api.php and function hook_schema does not need to be linked to schemaapi group because it is already in the schemaapi group, @joachim. When navigating on api.drupal.org, I can follow the “Database abstraction layer” link, then follow the “Schema API” link, and hook_schema is listed as a function. When I follow that link, “Schema API” is listed as a related topic.

Seems okay to me, then.

But ConvertDefinitionTest has a failure and static analysis failure for named parameters.

I updated the change record.

There seems to be a random failure in CKEditor FunctionalJavascript test so I reran the pipeline.

I think this should be RTBC since that one is green. Thank you, @mondrake.

I haven't reviewed this in depth yet.

One question I have is about the top level Schema object - I'm not really sure what that gets us that an array of table objects would not.

Looking at comment_schema() in the conversion MR, it results in a fair amount of nesting compared to if we did $tables[] = new Table();

OK I need to think about #95 a bit but I still find the multiple levels of nesting harder to follow reading that MR. We could potentially build the table objects and add them to the schema object rather than all the nested calls?

OK that search one looks more like I was thinking, so I think the change would be using the same pattern for comment then.

What does name do in the Schema object? Can't immediately see how it's used.

Also more thinking: a 'schema' in general DBMS terms can include other assets like views, stored procedures, etc etc. Not saying we should support that, but in case a need appears in the future, a mere array of tables will not be enough.

Doesn't hurt to leave open the possibility.

Issue still tagged with "Needs framework manager review" - Does that mean we waiting on Daffie here or should the tag be removed?

@daffie is not a framework manager, he is the subsystem maintainer.

The backend framework managers are:

effulgentsia, catch, alexpott, and larowlan.

I've not read the issue to see if they've reviewed it with their framework manager hat on. You can find the list of roles in maintainers.txt

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.

Reviewed the MR.. I like the direction in principle, but I think it'll need a bit more thought and discussion :)

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.