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 |
Comments
Comment #1
Damien Tournoud CreditAttribution: Damien Tournoud commentedThere are several possible syntaxes. The most obvious being:
Any other suggestions?
Comment #2
bojanz CreditAttribution: bojanz commentedNow 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:
The full post is here.
Comment #3
Damien Tournoud CreditAttribution: Damien Tournoud commentedWell, the syntax is completely undocumented, and doesn't seem to follow the pattern Barry wanted to introduce. Some examples:
Erm.
Comment #4
Damien Tournoud CreditAttribution: Damien Tournoud commentedI suggest we land on:
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 commentedDemoting to major.
Comment #6
Crell CreditAttribution: Crell commentedBlurgh. 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:
That way it's more self-documenting.
Comment #7
bojanz CreditAttribution: bojanz commentedHere'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)
Comment #9
bojanz CreditAttribution: bojanz commented#7: 880132.patch queued for re-testing.
Comment #10
Crell CreditAttribution: Crell commentedLooks good to me. Thanks!
Comment #11
Dries CreditAttribution: Dries commentedAlright, looks good to me. Committed to CVS HEAD.