Consider making target_id NOT NULL [#1569144]

Posted by Damien Tournoud on May 9, 2012 at 1:23pm

8 followers

Issue Summary

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.

Comments

#1

Posted by Amitaibu on May 9, 2012 at 1:27pm

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