Not sure why, but all the way up to the initial commit of this module, we have had target_id defined as:

        'target_id' => array(
          'description' => 'The id of the target entity',
          'type' => 'int',
          'unsigned' => TRUE,
          'not null' => FALSE,
        ),

The column accepts NULL as a value, which is legitimate in some corner cases (the Tree module uses this to store roots of the tree, but it already can modify the schema itself), but most of the cases it just hides obscure bugs.

For example in #1569046: Handling NULL values during the migration of Entity Reference fields, we have a case of a migration storing NULL values in there incorrectly, which result in:

The target_id => NULL causes all kinds of EntityMalformedExceptions, hinders indexing by Solr and causes this kind of warnings: Warning: array_flip() [function.array-flip]: Can only flip STRING and INTEGER values! in DrupalDefaultEntityController->load()

I think we should just consider moving to NOT NULL and this type of issue would have been caught way sooner.

CommentFileSizeAuthor
#7 1569144-target-id-not-null.patch617 bytesandreypaa
#6 1569144-target-id-not-null.patch617 bytesandreypaa
#3 1569144-target-id-not-null.patch1.59 KBDamien Tournoud
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

> we have a case of a migration storing NULL values in there incorrectly

I think we should follow core here, and keep it not NULL:

  $schema['taxonomy_term_data'] = array(
    'description' => 'Stores term information.',
    'fields' => array(
      'tid' => array(
        'type' => 'serial',
        'unsigned' => TRUE,
        'not null' => TRUE,
        'description' => 'Primary Key: Unique term ID.',
      ),

although maybe I miss a case where it's indeed needed to be NULL?

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented

oh, I've misread the title -- yes it should be 'not NULL' => TRUE

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Active » Needs review
FileSize
1569144-target-id-not-null.patch1.59 KB

Ok, changing this is relatively trivial. Here is a patch.

amitaibu’s picture

amitaibu
Primary language Hebrew
Location Israel
CreditAttribution: amitaibu commented
Status: Needs review » Reviewed & tested by the community
Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Reviewed & tested by the community » Fixed

Merged into 7.x-1.x.

andreypaa’s picture

andreypaa CreditAttribution: andreypaa commented
FileSize
1569144-target-id-not-null.patch617 bytes

Correct update process.

andreypaa’s picture

andreypaa CreditAttribution: andreypaa commented
Status: Fixed » Needs review
FileSize
1569144-target-id-not-null.patch617 bytes

Correct update process.
Sorry for the double comment

gaele’s picture

gaele CreditAttribution: gaele commented
Priority: Normal » Critical
Status: Needs review » Reviewed & tested by the community

Right, update fails in rc2 (Missing argument 4 for db_change_field()). Patch from #7 solves this.

pandaPowder’s picture

pandaPowder CreditAttribution: pandaPowder commented

Thanks for the patch. Upgrade with drush now works

Damien Tournoud’s picture

Damien Tournoud CreditAttribution: Damien Tournoud commented
Status: Reviewed & tested by the community » Fixed

Shame. I was certain I tested that. Let's roll a rc3 :(

Status: Fixed » Closed (fixed)

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

j0rd’s picture

j0rd CreditAttribution: j0rd commented

I run into cases where node_save's happen on nodes that were created programatically with out initializing the entityfields in the creation.

Then if I load and attempt to programatically save, I run into "field_entity_reference" cannot be null database fails.

So I think if we're going to make these not null, then perhaps we need to make sure on node_load or node_presave that entity_reference will not cause these NULL inserts.

UPDATE: Actually perhaps these nodes were created before I added this particular field_entity_reference to them. Thus the fields are not initialized. Either way, need to make sure we can save them.