Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Ubercart is implementint the foreign keys in schema like this:
...
$schema['uc_order_products'] = array(
...
'foreign keys' => array(
'order_id' => array('uc_orders' => 'order_id'),
'nid' => array('node' => 'nid'),
),
);
...
and according to Schema API they should be implemented like this:
...
$schema['uc_order_products'] = array(
...
'foreign keys' => array(
'uc_orders' => array(
'table' => 'uc_orders',
'columns' => array('order_id' => 'order_id'),
),
'node' => array(
'table' => 'node',
'columns' =>array('nid' => 'nid'),
),
),
);
...
I'll mark this bug to critical because may lead to unknown errors in any other module.
I found that this by applying this patch: #1565484: Product custom fields not available under Orders/Ordered Products type views but there are also other topics on this wrong implementation in activity module for example: #1292760: Causes undefined index _node_query_node_access_alter .
Comments
Comment #1
longwaveCan you review and test this patch?
Comment #2
TR CreditAttribution: TR commentedNeeds hook_update_N() functions.
Comment #3
longwaveTo do what? http://drupal.org/node/150215 doesn't mention foreign keys and there are no db_* functions that deal with them. Drupal core doesn't appear to do anything with them directly in any supported database platform.
edit: From http://drupal.org/node/146939 - "Foreign key definitions were added in Drupal 7 for documentation purposes only, and do not modify the database."
Comment #4
SilviuChingaru CreditAttribution: SilviuChingaru commentedThere is no need for hook_update_N() because:
Source: http://drupal.org/node/146939
P.S.: I'll review the patch tomorrow.
Comment #5
TR CreditAttribution: TR commentedOh, OK. I assumed it affected the schema in the DB.
Note that the documentation was written 6 months after these elements were added to the Ubercart .install files, and before the D7 official release, so I'm going to give Lyle the benefit of the doubt and assume the format was initially correct and that the spec changed during the D7 development process.
In fact, the way Ubercart does it agrees with the documentation at http://drupal.org/node/224333#foreign-keys-added . That should probably be edited, assuming it's wrong and the above-referenced documentation is right. Do we know for sure? The original issue is #111011: Add foreign keys to core
Comment #6
longwave#224333 is wrong; the syntax was extended in #880132: hook_schema() doesn't support compound foreign keys; inconsistent implementations in core, see core for examples: http://api.drupal.org/api/drupal/modules!node!node.install/function/node...
Comment #7
longwaveCommitted #1.
Comment #9
SilviuChingaru CreditAttribution: SilviuChingaru commentedProblem is still present, at least in shipping and payment modules. We need a complete patch that fix all this wrong schema implementation.
Comment #10
TR CreditAttribution: TR commentedProblem is not "still present" - you initially reported a problem only with the uc_order_products table, which was fixed. Now you are saying there are other tables that have this problem. Please tell us specifically what those tables are. And since your original post shows you know what needs to be done, please supply a patch.
Comment #11
SilviuChingaru CreditAttribution: SilviuChingaru commentedInitially I said:
So I mean by Ubercart all of it not a specific module.
I will supply a patch. I reopened it to remember that here a patch is needed too...
As i said:
...so look in uc_shipping/uc_shipping.install for example at hook_schema_info() and you'll see the problem.
Comment #12
SilviuChingaru CreditAttribution: SilviuChingaru commentedThe remaining foreign keys implementation and few fixes on longwave implementation.
Comment #14
SilviuChingaru CreditAttribution: SilviuChingaru commentedUps... My mistake, didn't run all tests locally first. Now I ran them and everything seems ok.
Comment #15
longwaveNot sure this belongs here?
Comment #16
SilviuChingaru CreditAttribution: SilviuChingaru commentedNo it doesn't. It seems that I've forgot to change the branch but the rest of the patch is ok.
That is from another patch I'm working on. Do you need me to repost updated patch or you 'll edit it?
Comment #17
longwaveNo need for an updated patch, as it's noted here I will remember when I get time to review and commit this.
Comment #18
longwaveCommitted #14 minus the change to uc_quote.info.
Comment #19
SilviuChingaru CreditAttribution: SilviuChingaru commentedThank's for credits :-(
Comment #20
longwaveYou have author credit in git? http://drupalcode.org/project/ubercart.git/commit/b815b3d1c5a1c1a16dc31b...
And it shows up under "projects" on your profile: http://drupal.org/user/352564
Comment #21
SilviuChingaru CreditAttribution: SilviuChingaru commentedI was talking about credits in release notes... No problem. It was a joke anyway. :-)) I'm happy it was commited and finished!
Comment #22
SilviuChingaru CreditAttribution: SilviuChingaru commentedI was talking about credits in release notes... No problem. It was a joke anyway. :-)) I'm happy it was commited and finished!