DBTNG should be able to create foreign keys

I support this!

I'm a little skeptical. If you create actual foreign keys, much of Drupal's use of SQL breaks. The idea of adding a feature that we document that you can't use doesn't seem like a win...

Liam,

Could you put this into a contrib module? Just create a new Database Driver and extend the existing class? That way Contrib modules that need foreign keys can require the new DB Driver? (or the module could gracefully disregard the foreign keys when that driver is not in use).

Might be a better route since I highly doubt any new features are going to be in Drupal 8.

Thanks!

Liam,

I suppose since it's backwards-compatible it could get released in a Drupal 8 minor (see #2135189: Proposal to manage the Drupal core release cycle).

I understand that it'd be better to be in core, but I think it'd be nice to have it in a usable, "safe" state (by being in a module) that can be used right now, rather than 1) have to patch core every time or 2) have to wait on core to commit the review/commit the patch. If module creators can demonstrate the effectiveness of using foreign keys in Drupal, I think there's a case for it to be in core, but I just don't think there's enough support for DB-level foreign keys. :(

Thanks!

It looks like the question is whether Crell's concern in #4 is justified or not:

If you create actual foreign keys, much of Drupal's use of SQL breaks. The idea of adding a feature that we document that you can't use doesn't seem like a win...

Say you implemented your own schema with foreign keys, would/could that break any of Drupal's funcionality?

Perhaps an example implementation that works without problems could move things further.

Right, it's clear that nothing would happen if you don't implement FKs. I take Crell's comment to mean that you could break your own module, or interaction between your module and Drupal core, by implementing FKs.

So yes, I was thinking of D8 + your patch and a contrib demo module to show some things that wouldn't break, as an example. Though I suppose that could be a lot of work.

The problem is situations like this:

When you delete a node, Drupal first deletes the record from the {node} table. It then lets contrib modules know (via hook) "by the way, that node has been deleted; you may want to clean stuff up", which then delete whatever records they have in tables that reference the now-deleted node. Fields do the same, just with a single module doing all of it.

If the nid column in the contrib module's table has a FK defined to the nid column of the {node} table, and you delete the record in the {node} table, SQL will (correctly) yell at you for a foreign key violation.

Now, in Drupal 8, ironically, I think this is less of an issue for the simple reason that you should not be writing SQL in Drupal 8. Really. Your module's data should be stored using the Config System, State System, or Entity System. There are extremely few cases where a non-site-specific module should ever be writing SQL anymore, which makes most of this issue moot.

Crell,

In Drupal 8, which API would you use to do something like Comment Tree? It seems overkill that the comment_tree table would be an Entity, but perhaps that's my Drupal 7 concept of Entities, not the Drupal 8 thinking. Would every table in Drupal 8 be an Entity? If this is the case then it might be wise to consider having Foreign Key constrains for Entities rather than for the schema. Would it be possible to have database-level foreign keys (like the Doctrine ORM) on a per-entity basis? or would it be an all or nothing proposition?

Thanks!

That looks like an SQL-specific optimization tool that may not be relevant on other backends. (Say, if you're storing comments in MongoDB, for instance.) That would mean it qualifies as one of the "extremely few cases" (not zero cases) where custom SQL makes sense. In the majority case, however, people should be using EFQv2 for all entity access.

The problem with FKs for entities is that we'd need to completely reverse the order in which things happen when updating or removing an entity. The Entity API team has enough to do yet before we can ship. :-) If we had FKs then we could, arguably, rely on those for deletion. (Delete just the record from the node table and let the DB handle the rest) That may have who knows what other side effects. (Removing log entries? Does everything that references a node really get deleted when the node does?) It would also mean we couldn't use a non-FK-enforcing database table; that really means just MyISAM, which there's work to restore happening right now. :-) (Not that I'd mind losing MyISAM myself, but there are larger implications.)

My overall point is that the use case for this change in D8 is fairly small, and the impact of trying to leverage it quite large. That feels like a bad trade-off to me, especially when we really really really want a beta out soon.

Crell,

Thanks for taking the time to answer that for me. I know you're a busy guy. :)

I think the point of having foreign key constraints, is not to use CASCADE, but rather to have better data integrity. I know that's a big issue and is not solved by simply having FKs, but I think it's an important part of it. Ironically, I would think that Enterprises would care about database-level data integrity, if this really is something that is important to the Enterprise, I would imagine that it's also important to Drupal.

I actually hate using CASCADE for the very problems you mention. It takes control of deleting out of the hands of the program and into the database which may or may not support them (and if it it does, how do you know the user hasn't disabled them?).

Another way of solving this problem is by allowing NULL on fields with database level foreign keys. This way, when data that is related to a node (our example) is deleted, Drupal should first set the FK fields to NULL, then delete the node, then it just needs to clean up any NULL relations. This way the order in which Drupal delets things does not change, the only thing that changes is what Drupal looks for when deleting. It might be weird to have NULLs in other tables (like field tables), but technically this is accurate. The field can either be a legitimate nid or it can be null, it can't be anything else. If you were to delete a node, it should be NULL since the relationship technically no longer exists. Right now, when Drupal deletes a node, it has relationships that are not valid in the database. This is the essence of the problem that should be remedied. You cannot rely on the Drupal database for valid relationships.

As far as your log question, I think that's an edge case, but in that scenario, I simply wouldn't have the constraints on the log table. In that scenario we're saying "It's ok if this relationship is no longer valid, and the relationship should not be relied upon." Right now however, we are saying that is the case with every table in Drupal. :(

Do you think it would be possible to create a module that replaces the standard EFQ in Drupal 8 with one that has FK support? (that way people who want it can at least have it even if Drupal doesn't plan on supporting them?)

Thanks!

Allowing either cascade or set-null would be a hard requirement for us adding this functionality.

Without cascade, though, what data integrity are you getting? Just the "in array" type validation that you don't write a nid that doesn't exist (but can orphan it later)?

I totally get that there are use cases where experts working with their own tables (eg, Liam in #18) would know how to leverage FK relationships properly in intelligent ways. My concern is the unwitting contrib author saying "Oh, I should totally create these, yeah!" without thinking through what the implications are (like, is the site even using SQL for that functionality, or is it using MongoDB or Redis?). That could cause *more* integrity issues on a site using that module than it saves. We would need to figure out how to properly communicate when it's appropriate to use. (And just throwing "Advanced dangerous stuff, beware!" on the key name, as we've occasionally done before, is not going to fly.)

I discussed this issue with Alex Pott at the DrupalCon code sprint. We're OK with adding #20/#21, with the following caveats:

1) As noted above, if you don't specify the "on" behavior the key is not actually created in the database.
2) It would need full test coverage (of course). The tests need to also validate against Postgres and SQLite, at least for as long as those remain in core.
3) Nothing in core would make use of this functionality. It would be purely for custom site-specific modules. Contrib modules that rely on it and break something are SOL.
4) It needs to be in before RC1. If it misses RC1 it would not get into 8.0 as it's a feature addition. However, it's a non-breaking addition so it *would* be eligible for 8.1. So it's not a "now or wait 5 years" type urgency. :-)

Liam, David: Back over to you.

This issue introduces a new feature, so per https://www.drupal.org/core/beta-changes (which was adopted several months after #23), we should postpone it to 8.1.x or later. Thanks all.

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -366,6 +376,18 @@ protected function createKeySql($fields) {
   +   *   The SQL definition of the foreign key or NULL if the definition is invalid.
   

   Line exceeding 80 characters

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -443,6 +454,18 @@ protected function _createKeySql($fields) {
   +   *   The SQL definition of the foreign key or NULL if the definition is invalid.
   

   Line exceeding 80 characters

3. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -691,11 +691,23 @@ protected function tearDown() {
   +      // MySQL does not have a transactional DDL so we must disable foreign keys to
   

   Line exceeding 80 characters

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -366,6 +376,18 @@ protected function createKeySql($fields) {
   +   *   The SQL definition of the foreign key or NULL if the definition is invalid.
   

   Line exceeding 80 characters

2. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -443,6 +454,18 @@ protected function _createKeySql($fields) {
   +   *   The SQL definition of the foreign key or NULL if the definition is invalid.
   

   Line exceeding 80 characters

3. +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -691,11 +691,23 @@ protected function tearDown() {
   +      // MySQL does not have a transactional DDL so we must disable foreign keys to
   

   Line exceeding 80 characters

Re-rolled.

EDIT: Unable to create interdiff due to re-roll (as is often the case):

interdiff: Error applying patch1 to reconstructed file

#40: Please provide an interdiff to illustrate any differences when providing updated patches.

All the minor fixes have been resolved.

For the array's use the short notation [] and not the long version array().

Re-rolled.

Unable to create interdiff due to complexity of re-roll:

interdiff impossible; taking evasive action

I'm glad to see this making its way into core. Hopefully it will be included in 8.4, and I can spend my Christmas break adding foreign keys to my module tables!

Is this just for database table schemas or also for entities?

How do we handle migration/update of existing databases without foreign keys?

Does this work for content/config entities or just for table schemas (= hook_schema() definitions)?

Why not extend it, to work with entities also?

Patch #51: Nice work! Code looks straightforward.

One nit though:

private function fkExists($table, $name) {

I'd write that out like everything else.

The patch is failing the SQLite and PostgreSQL tests. Therefor it needs work.

If the patch is not intended to work with PostgreSQL and SQLite, then it should at least not affect the tests.

Can you prove the same failing tests, when you are testing without any code changes? That would prove your assumption.

@Liam Morland: Thanks for the patch!

Which steps do I need to follow for proving that your patch works on my end? It would be nice, if you could post a "step-by-step instruction" of basically what you did for testing. Afterwards I can set the status to "Reviewed and tested by the community".

Can you post me the instruction for testing your patch?

Can I also create a custom entity with foreign keys on it's relation?

SchemaTest::testSchema() is a huge method. In my opinion it is done wrong anyways. There should not be such a huge function. The test methods should be more granular. May there is a better place for it. But we can keep it as it is for now in my opinion and refactor this in a different issue.

PostgreSQL: May it is an option to implement foreign keys for MySQL only? You have the option to check it via

$is_mysql = Database::getConnection()->databaseType() === 'mysql';

I can confirm that the foreign key creation works.

Even if this issue is "not" about having foreign keys in entities, it is a pity that this code does not support entities. Just creating a DB schema seems so old nowadays. We should support foreign keys in entity as a community. Like Doctrine and any other ORM. I would suggest that we create an new issue, after the patch is being commited into core to enhance entities with foreign keys also. Can your code be re-used for entities in any way?

I saw that the PostgreSQL are also passing now without any needed changes in this issue. It seems that the PostgreSQL tests do not have anything to do with this issue (PHP 7 & PostgreSQL 9.1 23,064 pass). Therefor I am changing the status to RTBC.

I have created an issue for refactoring SchemaTest::testSchema() at https://www.drupal.org/project/drupal/issues/2935506.

@Liam Morland: The idea of adding the functionality for creating and dropping foreign keys is great. I did a review of your patch and I do have remarks:

1. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -382,6 +382,20 @@ public function fieldExists($table, $column) {
   +   * @return bool|null
   +   *   TRUE if the foreign key exists, FALSE if it does not, NULL if the
   +   *   database does not support foreign keys.
   

   Can we make the return value boolean only. If the database does not support foreign keys then the foreign does not exist and should return the null value.

2. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -409,6 +423,68 @@ public function fieldExists($table, $column) {
   +   *     'on update' and 'on delete' => One of 'no action', 'restrict',
   +   *       'cascade', 'set null', or 'set default'.
   

   Can we have some more documentation what these options are and what they do. Are they required values? For both on update and on delete? It is not clear to me how this works.

3. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -409,6 +423,68 @@ public function fieldExists($table, $column) {
   +   * @return true|null
   +   *   TRUE if the foreign key was created, NULL if the database does not
   +   *   support foreign keys.
   ...
   +    if (!method_exists($this, 'createForeignKeySql')) {
   +      return NULL;
   +    }
   

   Can the return value be boolean only. If the database does not support foreign keys then the foreign key could not be created and the return value should be false.

4. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -409,6 +423,68 @@ public function fieldExists($table, $column) {
   +   * @return
   +   *   TRUE if the key was successfully dropped, FALSE if there was no key by
   +   *   that name to begin with, NULL if the database does not support foreign
   +   *   keys.
   

   Can the return value be boolean only. If the database does not support foreign keys then the foreign key does not exist and there for could not be dropped, then the return value should be false.

5. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -409,6 +423,68 @@ public function fieldExists($table, $column) {
   +  public function dropForeignKey($table, $name) {
   +    $foreignKeyExists = $this->foreignKeyExists($table, $name);
   +
   +    if ($foreignKeyExists) {
   +      $this->connection->query('ALTER TABLE {' . $table . '} ' . $this->dropForeignKeySql($table, $name));
   +      return TRUE;
   +    }
   +    return $foreignKeyExists;
   +  }
   

   This can be rewritten to:

   +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -409,6 +423,68 @@ public function fieldExists($table, $column) {
   +  public function dropForeignKey($table, $name) {
   +    if ($this->foreignKeyExists($table, $name)) {
   +      $this->connection->query('ALTER TABLE {' . $table . '} ' . $this->dropForeignKeySql($table, $name));
   +      return TRUE;
   +    }
   +    return FALSE;
   +  }
   

   We could add an extra test to see if the foreign key was really dropped.

6. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -535,7 +582,7 @@ public function dropTable($table) {
   -    $this->connection->query('DROP TABLE {' . $table . '}');
   +    $this->connection->query('DROP TABLE {' . $table . '} CASCADE');
   

   Can there be some documentation added why the "CASCADE" keyword needs to be added.

7. +++ b/core/modules/simpletest/simpletest.module
   @@ -671,6 +671,14 @@ function simpletest_clean_environment() {
   +  // Use transactional DDL if available.
   +  $txn = Database::getConnection()->startTransaction();
   +  // MySQL does not have a transactional DDL so we must disable foreign keys to
   +  // delete tables that have fk relationships between them.
   +  $is_mysql = Database::getConnection()->databaseType() === 'mysql';
   +  if ($is_mysql) {
   +    Database::getConnection()->query('SET foreign_key_checks = 0');
   +  }
   
   @@ -681,6 +689,11 @@ function simpletest_clean_database() {
   +  if ($is_mysql) {
   +    Database::getConnection()->query('SET foreign_key_checks = 1');
   +  }
   +  // Commit the transaction.
   +  unset($txn);
   
   +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
   @@ -634,11 +634,25 @@ protected function tearDown() {
   +      // Use transactional DDL if available.
   +      $txn = Database::getConnection()->startTransaction();
   +      // MySQL does not have a transactional DDL so we must
   +      // disable foreign keys to delete tables that have
   +      // fk relationships between them.
   +      $is_mysql = Database::getConnection()->databaseType() === 'mysql';
   +      if ($is_mysql) {
   +        Database::getConnection()->query('SET foreign_key_checks = 0');
   +      }
   ...
   +      if ($is_mysql) {
   +        Database::getConnection()->query('SET foreign_key_checks = 1');
   +      }
   +      // Commit the transaction.
   +      unset($txn);
   

   For testing purposes it is necessary to keep the testing environment as close as possible to real world usages of Drupal.
   My suggestion would be to create tests with and without these settings.

8. The changes to “core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php” are all to the test testSchema(). This is already a huge test. Let’s not make it any bigger. See #2935506: Refactor SchemaTest::testSchema() to make it more granular. Create new tests for the new functionality. Not only for when things go as they should go, but also when they should fail.

Regarding 7, these are just tear-down and clean methods. The changes ensure that they will work even if FK relationships still exist between tables.

Most Drupal sites will run with the default MySQL settings. For "foreign_key_checks" the default setting is "1". The testing in your patch is done with the setting "0". For adding this functionality to Drupal Core is needs to work with the default setting.

Regarding 8, the other keys are tested in that test. It is more efficient to do it together. Otherwise it is a lot more created, changing, and dropping tables.

The SchemaTest::testSchema() is a very old test and it is not written in the way that we now would like it. Please make new tests for the foreign key functionality. If there is a problem with a test it is much easier to find out where the problem is. With tests like SchemaTest::testSchema() that is a lot harder.

The tests run with foreign_key_checks set to 1. It is only set to 0 for tear-down and clean-up.

You are right.

What is the advantage to always returning FALSE instead of NULL? FALSE suggests that something went wrong. Using NULL means one can easily tell that there isn't an error, it just isn't supported.

Somehow it feels a bit wrong for me. Give me some time to think it over.

If you want to know if the database support foreign keys, should we not just add a method to the Connection class. Something like "Connection::supportForeignKeys()". For MySQL and PostgreSQL it will returns true and for SQLite it will return false.

Looks good. How about commiting it into core?

The test suite should be green on all three database backends before this can go in, so I don't think we're there yet.

I can't really provide much help here as this issue is a bit above my paygrade, but it certainly is pretty encouraging that the tests pass on MySQL without too much heavy-lifting so far. So would be nice to make progress on this.

Are the failing tests related to this patch? I saw them passing.

Looks good to me! Thank you for your work Liam Morland. Much appreciated.

1. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -278,6 +278,16 @@ protected function createKeysSql($spec) {
   +          $fk = $this->createForeignKeySql(NULL, $key, $fields);
   +          if ($fk) {
   +            $keys[] = $fk;
   

   nit: I would prefer we named this $foreign_key for clarity

2. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -370,6 +380,41 @@ protected function createKeySql($fields) {
   +  protected function createForeignKeySql($table, $key_name, array $fields) {
   ...
   +  protected function dropForeignKeySql($table, $name) {
   

   If the signature here was

   protected function createForeignKeySql($key_name, array $fields, $table = NULL);
   

   I think it would make more sense, having a largely optional parameter first feels skewif

   Same story on the drop

3. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -370,6 +380,41 @@ protected function createKeySql($fields) {
   +    if (isset($fields['on update']) && isset($fields['on delete']) && in_array($fields['on update'], $operations)  && in_array($fields['on delete'], $operations)) {
   

   extra space here.

   in_array should use the third argument in both instances, we're checking strings here.

   https://3v4l.org/DWjeK

4. +++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
   @@ -370,6 +380,41 @@ protected function createKeySql($fields) {
   +      return 'CONSTRAINT `' . $key_name . '` FOREIGN KEY (`' . implode('`, `', array_keys($fields['columns'])) . '`) REFERENCES {' . $fields['table'] . '} (`' . implode('`, `', $fields['columns']) . '`) ON DELETE ' . strtoupper($fields['on delete']) . ' ON UPDATE ' . strtoupper($fields['on update']);
   

   in my opinion this would be far more readable if it used sprintf

5. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -442,6 +454,41 @@ protected function _createKeySql($fields) {
   +    $operations = ['no action', 'restrict', 'cascade', 'set null', 'set default'];
   +    if (isset($fields['on update']) && isset($fields['on delete']) && in_array($fields['on update'], $operations)  && in_array($fields['on delete'], $operations)) {
   

   We have this code in multiple places - would it make sense as a protected method on the base class?

6. +++ b/core/lib/Drupal/Core/Database/Driver/pgsql/Schema.php
   @@ -535,7 +582,10 @@ public function dropTable($table) {
   +    $this->connection->query('DROP TABLE {' . $table . '} CASCADE');
   

   this feels like something that would come back to bite us later. Are we really sure that silently wiping dependent tables is the right thing to do?

7. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -382,6 +382,20 @@ public function fieldExists($table, $column) {
   +  public function foreignKeyExists($table, $name) {}
   

   So we're defaulting to 'not supported'?

   Perhaps we should say 'NULL if the database driver' instead of 'database' because for implementations in contrib, the database itself may support it, but the driver may not (until it is updated to reflect this patch)

8. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -409,6 +423,70 @@ public function fieldExists($table, $column) {
   +      throw new SchemaObjectDoesNotExistException(t("Cannot add foreign key @name to table @table: table doesn't exist.", ['@table' => $table, '@name' => $name]));
   ...
   +      throw new SchemaObjectExistsException(t("Cannot add foreign key @name to table @table: foreign key already exists.", ['@table' => $table, '@name' => $name]));
   ...
   +      throw new SchemaException(t("Cannot add foreign key @name to table @table: invalid foreign key definition.", ['@table' => $table, '@name' => $name]));
   

   We need tests for these code paths

9. +++ b/core/lib/Drupal/Core/Database/Schema.php
   @@ -409,6 +423,70 @@ public function fieldExists($table, $column) {
   +    $createForeignKeySql = $this->createForeignKeySql($table, $name, $fields);
   +    if (!$createForeignKeySql) {
   

   we can probably inline this

   if (!$createForeignKeySql = $this->createForeignKeySql($table, $name, $fields)) {
   

10. +++ b/core/lib/Drupal/Core/Database/Schema.php
    @@ -409,6 +423,70 @@ public function fieldExists($table, $column) {
    +    $this->connection->query('ALTER TABLE {' . $table . '} ADD ' . $createForeignKeySql);
    

    Should we check the return of this before we return TRUE?

11. +++ b/core/lib/Drupal/Core/Database/Schema.php
    @@ -409,6 +423,70 @@ public function fieldExists($table, $column) {
    +      $this->connection->query('ALTER TABLE {' . $table . '} ' . $this->dropForeignKeySql($table, $name));
    +      return TRUE;
    

    Same, should we check the return of ->query before returning TRUE?

12. +++ b/core/modules/simpletest/simpletest.module
    @@ -671,6 +671,14 @@ function simpletest_clean_environment() {
    +  // delete tables that have fk relationships between them.
    
    +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
    @@ -634,11 +634,25 @@ protected function tearDown() {
    +      $is_mysql = Database::getConnection()->databaseType() === 'mysql';
    +      if ($is_mysql) {
    ...
    +      if ($is_mysql) {
    

    can we use foreign key here instead of fk, for clarity sake

13. +++ b/core/modules/simpletest/simpletest.module
    @@ -671,6 +671,14 @@ function simpletest_clean_environment() {
    +  $is_mysql = Database::getConnection()->databaseType() === 'mysql';
    +  if ($is_mysql) {
    
    @@ -681,6 +689,11 @@ function simpletest_clean_database() {
    +  if ($is_mysql) {
    
    +++ b/core/tests/Drupal/KernelTests/KernelTestBase.php
    @@ -634,11 +634,25 @@ protected function tearDown() {
    +      $is_mysql = Database::getConnection()->databaseType() === 'mysql';
    +      if ($is_mysql) {
    ...
    +      if ($is_mysql) {
    

    special casing mysql feels wrong, what if other contrib drivers are in the same boat?

    sounds like we're missing a supportsTransactionalDataDefinitionStatements method or similar

14. +++ b/core/tests/Drupal/KernelTests/Core/Database/SchemaTest.php
    @@ -179,6 +192,20 @@ public function testSchema() {
    +          'table' => 'fk_test_table',
    +          'columns' => ['test_field' => 'id'],
    +          'on update' => 'cascade',
    +          'on delete' => 'set null',
    +        ],
    +        // The following should not create anything in the database because
    +        // 'on update' and 'on delete' are not set.
    +        'test_fk_no_create' => [
    +          'table' => 'no_table',
    

    There are no tests of invalid values for 'on update' and 'on delete'

Replace deprecated code.

LGTM

Running the PostgreSQL on 9.5 appeared to work. Im not sure what else needs to be set for the 9.1 container, or if it can even be fixed.

This looks really good, but I'd love to see these small changes:

• Remove the ability to add a foreign key with update or delete set to "no action." While databases may support that, I don't really see the use case, as it basically misleads someone reviewing the schema into assuming the data has properties that aren't actually enforced. I have a strong opinion that we should expose only options with demonstrated need rather than attempting to expose the entire feature set of underlying DBs via the abstraction layer.
• Switch from strings for specifying the effects/enforcement of the foreign key to constants. This is not such a strong opinion, but I think it would be better DX.

RESTRICT and NO ACTION do the same thing in MySQL. In databases such as Postgres, NO ACTION is deferred to the end of the transaction while RESTRICT is not. In most cases, this doesn't make a practical difference, so I don't think anyone would be mislead.

Ah, I misunderstood the constraint's effect by assuming the name was descriptive. That, plus the disparity in function between PostgreSQL and MySQL makes me feel even more resolve that we should not offer "NO ACTION" as an available foreign key constraint. Deferring enforcement to the end of a transaction seems neat given how we handle transactions (really, labels) in conjunction with hooks, but I also suspect it allows sloppiness in the form of violating presumed FK behavior during larger transactions. Meanwhile, code expecting FK constraints to hold may get invoked before the transaction completes (though it seems like PostgreSQL may raise an error if it does).

Technically, NO ACTION is more standard in the SQL spec than RESTRICT, but I feel that the latter is much more descriptive. FWIW, SQL Server seems to support NO ACTION but not RESTRICT, but it behaves like MySQL/PostgreSQL's RESTRICT in the instant enforcement. So, while the terms differ, the only behavior that seems portable is equivalent to MySQL's RESTRICT.

Finally, does this establish default behavior for a foreign key, or does that fall onto the database? MySQL's default behavior is RESTRICT, but PostgreSQL's is NO ACTION. I feel like we should explicitly default to RESTRICT so there aren't silently different default constraints based on the database in use.

I'm looking for feedback about the above patch and about the use of constants, see #139.

I prefer constants over strings for a few reasons:

• It's easier to know early that the options specified are valid, available ones. With constants, PHP basically evaluates this as the schema data structure gets generated. With strings, it's only evaluated whenever the string gets compared against expected options later. (This is a general problem with using special strings for keys or values, though, not just here.)
• Strings for fixed values can be a leaky abstraction (or appear to be). It's unclear to a developer whether they can use any string that their database backend would accept for constraints, even if the option isn't available other other DBs. Because our goal is to have schema be portable, this expectation would be incorrect.
• Even if there's a validated allow list for string values, a string implies direct use of the value in interaction with the DB. Yet, the meanings of the strings in each DB aren't consistent. We may convert a requested constraint to work with a particular DB. For example, we would need to map "RESTRICT" to "NO ACTION" on SQL Server, as it lacks the former but has the latter behave the same as "RESTRICT" on MySQL/PostgreSQL. A constant communicates that the schema reflects the intent of constraints, not a literal string the DB will receive. I think this is the strongest reason to prefer constants. I think schema data structure values specified as strings ought to be be passed literally to the DB (or with the only processing being something like prefixing/escaping).

I don't have a strong opinion on which backing values the constants should have. Using string values for them mostly seems fine, but it has a bit more risk of developers simply using the actual strings in their schema definitions rather than the constants. Integers would have a stronger influence on developers following documented practices, as no one would want their schema constraints specified in their code literally as numbers.

Regarding concrete cases:

I could do it with integers: const SET_DEFAULT = 4;

Then at the point where the SQL is actually generated, I need to have a mapping back to the string 'SET DEFAULT'. Again, I'm not sure this is making things easier.

On SQL Server, Database::RESTRICT would map to the string NO ACTION in the actual schema generation. I think that case makes it easier to see why a mapping step is not just a pointless abstraction.

I could also see an option for PostgreSQL users to configure their DB to relax Database::RESTRICT to being sent as NO ACTION to allow more flexible enforcement during transactions in progress. Given that most code for Drupal is only tested against MySQL, this would be more useful to PostgreSQL users than having a direct ability to specify NO ACTION in schema data structures. Developers testing against MySQL are unlikely to use NO ACTION versus RESTRICT, at least in any coherent, useful way to a PostgreSQL user (given that MySQL doesn't distinguish).

So, I don't see the constants as simply another way to pick the string being sent to the DB.

This patch looks good to me (and has for a while). Is there accounting for what stands between where we are and applying it?

D10 version needed
At this time we would need a D10.1.x patch or MR for this issue.

careful reroll
The patch can be tricky to reroll and some code might get lost in the process. When rerolling this issue please make sure all the parts of the patch are kept (new files, new tests, all the changes to the different parts of the file, changes to es6 files ported to .js files, etc.)