The {users}.uid is defined in users.install:

'uid' => array(
  'type' => 'serial',
  'unsigned' => TRUE,
  'not null' => TRUE,
  'description' => 'Primary Key: Unique user ID.',
),

(This corresponds to general way of definition of IDs).

In node.install the schema use field:

'uid' => array(
  'description' => 'The {users}.uid that owns this node; initially, this is the user that created it.',
  'type' => 'int',
  'not null' => TRUE,
  'default' => 0),

The 'unsigned' => TRUE is missing by my opinion. This is not probably the only one type mismatch, but I'm not 100% sure this is a bug.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Liam Morland’s picture

Title: Mismatch of schema definition of uid » Mismatch of schema definition of uid, nid
Version: 6.12 » 8.x-dev
Priority: Minor » Normal
Status: Active » Needs review
FileSize
4 KB

There are several examples of this in various hook_schema() implementations. The attached patch fixes this in D8 by adding

'unsigned' => TRUE

to any definitions of uid or nid that lack them.

Liam Morland’s picture

Component: base system » database system
Liam Morland’s picture

Crell’s picture

Status: Needs review » Reviewed & tested by the community

I'm still not sold on directly leveraging foreign keys in core, but our definitions should at least be consistent so that they could be used.

catch’s picture

Status: Reviewed & tested by the community » Needs work

This needs an update from Drupal 7..

Liam Morland’s picture

Where should such updates go? Do they go in core/includes/update.inc or in a system module update hook implementation?

Crell’s picture

Schema-changing updates commands go in the .install file of the appropriate module. In this case, you'd have a couple of update hooks, one for each module that you're touching, that updates that module's tables. See http://api.drupal.org/api/drupal/core!modules!system!system.api.php/func...

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
8.12 KB

Attached patch includes update functions.

Crell’s picture

Status: Needs review » Needs work

drupal_get_schema() is not reliable within an update function, unfortunately. It uses cached values, not what's in the hook_schema() call. You have to just duplicate the information. :-(

Liam Morland’s picture

Even if $rebuild is set? That is supposed to bypass the cache.

Crell’s picture

Unless that changed and I didn't hear about it, there's some totally wacky race conditions involved between caching, modules being enabled, and the environment you're in. Just don't trust hook_schema or drupal_get_schema() while inside an update function.

Liam Morland’s picture

In e.g. comment_update_8002() can I call comment_schema() directly? I can't see how any caching could interfere with that and they are both in the same PHP file, so the function is sure to be there.

Crell’s picture

That should work, but I think it's discouraged. Catch? (Your call.)

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
8.15 KB

The attached patch calls the various schema functions directly. Both those and the update functions are in the same file and the schema functions do not call any other functions, so I cannot see how they could fail to return the correct information.

Berdir’s picture

Status: Needs review » Needs work

Calling the schema functions is wrong, not just discouraged.

Imagine that at a later time, the schema of those tables is changed in a way that is incompatible with this function. For example when the comment entity schema is changed to a multilingual one with two tables. Then the existing update function will fall apart because the structure it expects isn't there.

Liam Morland’s picture

Status: Needs work » Needs review
FileSize
9.44 KB

Thanks. Make sense. Here is a version that duplicates the relevant schema data in the update hook.

Liam Morland’s picture

FileSize
9.44 KB

Updated patch rolled against latest D8.

Liam Morland’s picture

FileSize
9.44 KB

Reroll.

Crell’s picture

This looks OK on visual inspection. I don't have time to manually test the upgrade tests right now. If those pass, then this is RTBC.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Looks good to me too, tests passed.

Dries’s picture

Status: Reviewed & tested by the community » Fixed

Committed to 8.x. Thanks!

frankye’s picture

Status: Fixed » Needs review

#18: core_unsigned_uid_nid.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, core_unsigned_uid_nid.patch, failed testing.

Liam Morland’s picture

Status: Needs work » Fixed

The patch failed testing because it has already been committed.

Status: Fixed » Closed (fixed)

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