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 .

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave’s picture

Status: Active » Needs review
FileSize
11.08 KB

Can you review and test this patch?

TR’s picture

Status: Needs review » Needs work

Needs hook_update_N() functions.

longwave’s picture

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

SilviuChingaru’s picture

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.

TR’s picture

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

longwave’s picture

longwave’s picture

Status: Needs review » Fixed

Committed #1.

Status: Fixed » Closed (fixed)

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

SilviuChingaru’s picture

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.

TR’s picture

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.

SilviuChingaru’s picture

Assigned: Unassigned » SilviuChingaru
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.

SilviuChingaru’s picture

Assigned: SilviuChingaru » Unassigned
Status: Needs work » Needs review
FileSize
9.31 KB

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.

SilviuChingaru’s picture

Status: Needs work » Needs review
FileSize
9.67 KB

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

longwave’s picture

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

Not sure this belongs here?

SilviuChingaru’s picture

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?

longwave’s picture

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

longwave’s picture

Status: Needs review » Fixed

Committed #14 minus the change to uc_quote.info.

SilviuChingaru’s picture

Thank's for credits :-(

longwave’s picture

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

SilviuChingaru’s picture

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

SilviuChingaru’s picture

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.