schema.module report with following message with latest 7.x-1.x GIT checkout:

  • column revision_id - difference on: not null
  • declared: array('description' => 'TODO: please describe this field!', 'type' => 'int', 'unsigned' => TRUE, 'not null' => FALSE, 'default' => 0)
  • actual: array('description' => 'The entity id revision_id this data is attached to', 'type' => 'int', 'unsigned' => TRUE, 'not null' => TRUE, 'default' => 0)
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

hswong3i’s picture

Status: Active » Needs review
FileSize
417 bytes
DamienMcKenna’s picture

Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: +D7 stable release blocker

This needs an update script to fix the field for any existing installs.

DamienMcKenna’s picture

FileSize
2.6 KB

This may work, I need to test it out.

DamienMcKenna’s picture

Status: Needs work » Needs review
DamienMcKenna’s picture

FileSize
2.87 KB

Updated patch that just includes some drupal_set_message comments.

DamienMcKenna’s picture

Assigned: hswong3i » Unassigned

I tested this on MySQL and it worked, but I don't have PostgreSQL (or anything else) available to test it there.

DamienMcKenna’s picture

Status: Needs review » Fixed

Committed. Thanks hswong3i!

DamienMcKenna’s picture

Actually, the revision_id field needs to allow NULL values, as e.g. the user entity saves a NULL for the revision_id in all field records.

DamienMcKenna’s picture

Status: Fixed » Closed (works as designed)

I've reverted this patch and will not be committing it, but I do very much appreciate the time you put into the patch.

rob_johnston’s picture

I'm going to agree with the OP. We had an upgrade on MS SQL Server fail this morning and we fixed it by implement the patch in #1 (and then later found this issue). I've gathered some evidence to support my argument, so here goes...

For MySql, from http://dev.mysql.com/doc/refman/5.7/en/create-table.html

A PRIMARY KEY is a unique index where all key columns must be defined as NOT NULL. If they are not explicitly declared as NOT NULL, MySQL declares them so implicitly (and silently).

For PostGres, from http://www.postgresql.org/docs/9.3/interactive/ddl-constraints.html#DDL-...

Technically, a primary key constraint is simply a combination of a unique constraint and a not-null constraint.

For Oracle, from http://docs.oracle.com/cd/B28359_01/server.111/b28318/glossary.htm#CNCPT...

PRIMARY KEY constraint = Integrity constraint that disallows duplicate values and nulls in a column or set of columns.

MS SQL Server, from http://technet.microsoft.com/en-us/library/aa933112(v=SQL.80).aspx

A table can have only one PRIMARY KEY constraint, and a column that participates in the PRIMARY KEY constraint cannot accept null values.

So there are 4 common RDBMS's that are used with Drupal and none of them allow a null value in a primary key. I think the solution here is to not create a primary key, but rather a unique constraint. See db_add_unique_key() function.

rob_johnston’s picture

Status: Closed (works as designed) » Needs work
DamienMcKenna’s picture

@rob_johnston: Thanks for the detailed analysis.

I believe my mistake was examining the a field_data_field_* table rather than field_revision_field_* table - in the former case the table only keeps track of the published revision_id so it isn't part of the primary key, whereas in the latter case there are multiple records and revision_id disallows NULLs and is a primary key.

So yeah, this needs to be applied.

I think the logic around it needs to be verified that a zero is saved as the revision_id.

DamienMcKenna’s picture

Status: Needs work » Needs review
FileSize
2.33 KB

This is the most recent patch rerolled.

DamienMcKenna’s picture

FileSize
3.63 KB

This is an updated version of #13 that updates calls to metatag_metatags_save() so that the revision_id defaults to a zero if there's no actual value.

HyperGlide’s picture

Can you provide a testing sequence or guidance to help move this patch to RTBC.

DamienMcKenna’s picture

Status: Needs review » Fixed

I committed patch #13 instead of #14 as I'm going to leave the fixes for revision_id CRUD logic to #1572474: Add Revisions to Metatags for Entities.

DamienMcKenna’s picture

Status: Fixed » Needs review
FileSize
1.32 KB

Doing some further consideration of this problem, we need to run an update to ensure all revision_id values are numeric before restructuring the schema.

DamienMcKenna’s picture

Status: Needs review » Fixed

Committed. FYI the new update is 7016 and the previous one was bumped to 7017.

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