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

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

Files: 
CommentFileSizeAuthor
#7 1569144-target-id-not-null.patch617 bytesandreypaa
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]
#6 1569144-target-id-not-null.patch617 bytesandreypaa
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]
#3 1569144-target-id-not-null.patch1.59 KBDamien Tournoud
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]

Comments

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

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

Status:Active» Needs review
StatusFileSize
new1.59 KB
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]

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

Status:Needs review» Reviewed & tested by the community

Status:Reviewed & tested by the community» Fixed

Merged into 7.x-1.x.

StatusFileSize
new617 bytes
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]

Correct update process.

Status:Fixed» Needs review
StatusFileSize
new617 bytes
PASSED: [[SimpleTest]]: [MySQL] 66 pass(es).
[ View ]

Correct update process.
Sorry for the double comment

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.

Thanks for the patch. Upgrade with drush now works

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.

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.