hook_schema() doesn't support compound foreign keys; inconsistent implementations in core [#880132]

Just talked with DamZ about this one and he agreed that this is a critical issue for D7.

1) The 'foreign keys' syntax is not correct. It provides no way of specifying a compound foreign key.
Instead of the current syntax, we want a name for the foreign key (because all other indexes have names) and two arrays for from and to.

So instead of:

'foreign keys' => array(
  'vid' => array('node_revision' => 'vid'),
),

We get something like:

'foreign keys' => array(
  'my_relation' => array(
    'from' =>  array('field1', 'field2'), 
    'to' => array(
      'node_revision' => array('field1', 'field2')
    ),
  ),
),

2) The implementations of hook_schema() and 'foreign keys' in core are inconsistent.

Here are the foreign keys for node:

'foreign keys' => array(
   'vid' => array('node_revision' => 'vid'),
   'uid' => array('users' => 'uid'),
),

And for node revision (obviously incorrect):

'foreign keys' => array(
  'node' => 'nid',
   'users' => 'uid'
),

Opening this as one issue since #1 affects #2 and it should be possible to hit them both in one go.

Comment	File	Size	Author
#7	880132.patch	22.01 KB	bojanz
#7

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

Damien Tournoud CreditAttribution: Damien Tournoud commented 11 August 2010 at 15:07

There are several possible syntaxes. The most obvious being:

'foreign keys' => array(
  'my_relation' => array(
    'columns' =>  array('field1', 'field2'), 
    'reference table' => 'node_revision',
    'reference keys' => array('field1', 'field2')
  ),
),

Any other suggestions?

Comment #2

bojanz CreditAttribution: bojanz commented 11 August 2010 at 15:17

Now that one sounds much better than my initial proposal.

In any case, once we agree on the approach, I can roll a patch.

Edit: found an explanation for the node_revision syntax, from bjaspan:

Whenever the local and foreign column names are the same and there is exactly one column involved, the array can be omitted; in this case we would have just 'node' => 'nid'.

The full post is here.

Comment #3

Damien Tournoud CreditAttribution: Damien Tournoud commented 11 August 2010 at 15:34

Well, the syntax is completely undocumented, and doesn't seem to follow the pattern Barry wanted to introduce. Some examples:

    'foreign keys' => array(
      'nid' => array('node' => 'nid'),
      'uid' => array('users' => 'uid'),
    ),

    'foreign keys' => array(
      'node' => 'nid',
      'users' => 'uid'
    ),

    'foreign keys' => array('node' => 'nid'),

    'foreign keys' => array(
      'nid' => array('node' => 'nid'),
      'last_comment_uid' => array('users' => 'uid'),
    ),

Erm.

Comment #4

Damien Tournoud CreditAttribution: Damien Tournoud commented 11 August 2010 at 15:37

I suggest we land on:

'foreign keys' => array(
  'my_relation' => array('reference_table', array('source_column' => 'referenced_column', ...)),
)

Which is slightly more compact then #1. We might want to allow skipping the 'source_column' key if it's the same as the referenced column.

Comment #5

Damien Tournoud CreditAttribution: Damien Tournoud commented 11 August 2010 at 15:50

Priority:

Critical

» Major

Demoting to major.

Comment #6

Crell CreditAttribution: Crell commented 11 August 2010 at 22:28

Blurgh. Agreed with major instead of critical, since it's not like we use FK definitions right now anyway...

I think I prefer Damien's syntax from #4. A given FK is only going to ever relate "this table" to "that one table", based on "these field mappings."

My knee-jerk response though is to label the keys:

'foreign keys' => array(
  'my_relation' => array(
    'table' => 'reference_table', 
    'columns' => array(
      'source_column' => 'referenced_column',
    )
  ),
)

That way it's more self-documenting.

Comment #7

bojanz CreditAttribution: bojanz commented 19 August 2010 at 17:40

Status:

Active

» Needs review

File	Size
880132.patch	22.01 KB

Here's a patch.

I've changed the docs, updated the existing usage, and added an example to the hook_schema() in system.api.php (which btw, has a weird coding style. But I'll open a separate issue for that)