The problem is in DatabaseSchema::createKeysSql (it's inheritors anyway).

Actually the bigger problem is:

#111011: Add foreign keys to core

Which I recently renamed due to the amazing confusion that hit me this morning when I discovered that the foreign keys array actually doesn't do anything.

Here's the code I was writing which triggered the problem:

$new_sig_field = array(
    'type' => 'int',
    'size' => 'small',
    'not null' => TRUE,
    'default' => 0,
    'description' => 'The {filter_format}.format of the signature.',
  );
  
  $new_keys = array(
    'foreign keys' => array(
      'signature_format' => array('filter_format' => 'format'),
    ),
  );
  db_add_field('users', 'signature_format', $new_sig_field, $new_keys);

Here is the root of the issue:

protected function createKeysSql($spec) {
    $keys = array();

    if (!empty($spec['primary key'])) {
      $keys[] = 'PRIMARY KEY (' . $this->createKeysSqlHelper($spec['primary key']) . ')';
    }
    if (!empty($spec['unique keys'])) {
      foreach ($spec['unique keys'] as $key => $fields) {
        $keys[] = 'UNIQUE KEY `' . $key . '` (' . $this->createKeysSqlHelper($fields) . ')';
      }
    }
    if (!empty($spec['indexes'])) {
      foreach ($spec['indexes'] as $index => $fields) {
        $keys[] = 'INDEX `' . $index . '` (' . $this->createKeysSqlHelper($fields) . ')';
      }
    }

    return $keys;
  }
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Damien Tournoud’s picture

This is foreign key array is meant for documentation only.

Implementing real database foreign keys was never on the table for Drupal 7.

JacobSingh’s picture

Even so, if you follow the API, this doesn't work and breaks with a nasty SQL error.

That's not acceptable is it?

Personally, I think it is harmfull to add "foreign keys" to the schema API if they are not implemented. If they are pointless, let's just add them when they do something.

Until then, if we want documentation, we should document it. Like:

'indexes' => array(),
// 'foreign_keys' => array()...

The docs for this function says it should take the format from hook_schema without the fields array. Following the docs, this is a bug.

JacobSingh’s picture

Even so, if you follow the API, this doesn't work and breaks with a nasty SQL error.

That's not acceptable is it?

Personally, I think it is harmfull to add "foreign keys" to the schema API if they are not implemented. If they are pointless, let's just add them when they do something.

Until then, if we want documentation, we should document it. Like:

'indexes' => array(),
// 'foreign_keys' => array()...

The docs for this function says it should take the format from hook_schema without the fields array. Following the docs, this is a bug.

Damien Tournoud’s picture

Status: Active » Needs review
FileSize
1.4 KB

This is definitely a bug: the DatabaseSchema_mysql implementation incorrectly outputs SQL as soon as $new_keys is not empty.

JacobSingh’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, that'll do it.

I still think it's awful to offer functionality in an API and never tell people it isn't working though. I'd like to get a nasty exception message if I tried to pass 'foreign keys' in, letting me know that it's not going to do anything.

Allowing people to pass in values which will not have an impact no matter what is a recipe for disaster IMHO.

neilnz’s picture

@JacobSingh, I know some of the people who pushed for the inclusion of the keys in #111011: Add foreign keys to core and the rationale was basically that it helps for Drupal (and especially contrib) to be aware of the relationships between records when it makes sense for it to be.

One example might be a contrib module to scan for "database corruption", by locating records that a contrib module failed to clean up properly.

Another (more advanced) use might be an implementation of database sharding, which may well require certain tables to be clustered so that any JOINs that may be present will still work (although I realise this can be achieved in other ways).

It also allows for ongoing research into referential integrity in core, possibly for D8. Getting people declaring this stuff early means it doesn't become a chicken and egg problem later on.

So while it's not actually used for anything (yet), the data itself isn't useless because it can still be read by anything that wants to know about it, just like PHPDoc comments or CHECK contraints in MySQL ;) [1]

[1] Seriously! Check out http://dev.mysql.com/doc/refman/5.1/en/create-table.html "The CHECK clause is parsed but ignored by all storage engines."

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.