Ubercart is implementint the foreign keys in schema like this:

<?php
...
 
$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:

<?php
...
 
$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 .

Files: 
CommentFileSizeAuthor
#14 ubercart-remaining_foreign_keys_implementation-1785434-10.patch9.67 KBfiftyz
PASSED: [[SimpleTest]]: [MySQL] 2,837 pass(es).
[ View ]
#12 ubercart-remaining_foreign_keys_implementation-1785434-9.patch9.31 KBfiftyz
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/ubercart/uc_product/uc_product.install.
[ View ]
#1 1785434-schema-foreign-keys.patch11.08 KBlongwave
PASSED: [[SimpleTest]]: [MySQL] 2,457 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new11.08 KB
PASSED: [[SimpleTest]]: [MySQL] 2,457 pass(es).
[ View ]

Can you review and test this patch?

Status:Needs review» Needs work

Needs hook_update_N() functions.

Status:Needs work» Needs review

To 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."

There is no need for hook_update_N() because:

'foreign keys': An associative array of foreign keys ('keyname' => specification). Each specification is an array with 'table' and 'columns' elements that form a foreign key for the table.
'table' is a string specifying the foreign table, and 'columns' is an associative array in the format 'source_column' => 'target_column'.
Note: Foreign key definitions were added in Drupal 7 for documentation purposes only, and do not modify the database.

Source: http://drupal.org/node/146939

P.S.: I'll review the patch tomorrow.

Oh, 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

Status:Needs review» Fixed

Committed #1.

Status:Fixed» Closed (fixed)

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

Status:Closed (fixed)» Needs work

Problem is still present, at least in shipping and payment modules. We need a complete patch that fix all this wrong schema implementation.

Status:Needs work» Postponed (maintainer needs more info)

Problem 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.

Assigned:Unassigned» fiftyz
Status:Postponed (maintainer needs more info)» Needs work

Initially I said:

Ubercart is implementint the foreign keys in schema like this

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:

Problem is still present, at least in shipping and payment modules.

...so look in uc_shipping/uc_shipping.install for example at hook_schema_info() and you'll see the problem.

Assigned:fiftyz» Unassigned
Status:Needs work» Needs review
StatusFileSize
new9.31 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in sites/default/modules/ubercart/uc_product/uc_product.install.
[ View ]

The remaining foreign keys implementation and few fixes on longwave implementation.

Status:Needs review» Needs work

The last submitted patch, ubercart-remaining_foreign_keys_implementation-1785434-9.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new9.67 KB
PASSED: [[SimpleTest]]: [MySQL] 2,837 pass(es).
[ View ]

Ups... My mistake, didn't run all tests locally first. Now I ran them and everything seems ok.

+;Wrapper class and helppers
+files[] = uc_quote.wrapper.inc

Not sure this belongs here?

No 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?

No need for an updated patch, as it's noted here I will remember when I get time to review and commit this.

Status:Needs review» Fixed

Committed #14 minus the change to uc_quote.info.

Thank's for credits :-(

You 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

I was talking about credits in release notes... No problem. It was a joke anyway. :-)) I'm happy it was commited and finished!

I was talking about credits in release notes... No problem. It was a joke anyway. :-)) I'm happy it was commited and finished!

Status:Fixed» Closed (fixed)

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