When I create a userreference field with a long machine-name, the variable name ends up being very long. Since the variable names can only be 48 characters long, the defaults don't get loaded and I get an SQL duplicate entry error.

Attached is a patch that cuts the variable names down to 48 characters. I can already see a potential issue with this since now it's possible for two different fields to have the same variable. Maybe it's more an issue with the Drupal variable system that variable names are limited to 48 characters. That's outside the scope of this patch, however.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Shawn DeArmond’s picture

Title: Long field names reveal a bug » Long field names exceed variable 48-character limit
FileSize
1.84 KB

I had to make one more change.

danielb’s picture

Status: Needs review » Needs work

If it just gonna cause more problems I wouldn't bother using the truncating method
I reckon a better way might be to create a single variable for this module just called "nodeaccess_userreference" which is an array where the keys are the field names.

Shawn DeArmond’s picture

Good idea. Here's a patch where it's all stored in a single variable. The default values are also stored in that variable. The variable has the potential to grow quite large, but it works.

Shawn DeArmond’s picture

Status: Needs work » Needs review

Needs review.

Just thought of something else. In order for this to be implemented, the module will need an update function or else the users will lose all their settings.

danielb’s picture

Hmm good point.
I quickly wrote this function (haven't tried it) we could use something like this instead of variable_get and variable_set, it will check for the old data

<?php

/**
 * Set and get nodeaccess userreference field settings.
 *
 * @param $type_name
 *   The node type.
 * @param $field_name
 *   The name of the field.
 * @param $variable
 *   If set will update the value of the settings for this field.
 * @return
 *   The stored or updated value of the settings for this field.
 */

function nodeaccess_userreference_field_settings($type_name, $field_name, $variable = NULL) {

  // 'get' the variable
  $data = variable_get('nodeaccess_userreference', NULL);
  if (!$data || !isset($data[$type_name][$field_name])) {
    // Attempt to get result from old variables.
    $old_data = variable_get($field_name .'_'. $type_name, NULL);
    if (is_array($old_data)) {
      variable_del($field_name .'_'. $type_name);
      $data[$type_name][$field_name] = $old_data;
    }
    else {
      // default
      $data[$type_name][$field_name] = array(
        'view' => 0, 
        'update' => 0, 
        'delete' => 0,
      );
    }
  }

  // change and 'set' the variable
  if ($variable) {
    $data[$type_name][$field_name] = $variable;
    variable_set('nodeaccess_userreference', $data);
  }

  return $data[$type_name][$field_name];
}

?>

To get the settings:

<?php

$data = nodeaccess_userreference_field_settings($field['type_name'], $field['field_name']);

// or

$data = nodeaccess_userreference_field_settings($form['type_name']['#value'], $form['field_name']['#value']);

?>

To update the settings:

<?php

nodeaccess_userreference_field_settings($form['type_name']['#value'], $form['field_name']['#value'], $data);

?>
Shawn DeArmond’s picture

I like the idea of using a separate function to wrap the variable_get and variable_set. It would be a little more intuitive, though, if it was two separate functions. something like:
nodeaccess_userreference_set($type_name, $field_name, $settings = NULL)
and
nodeaccess_userreference_get($type_name, $field_name, $default = NULL)

Also, I'm not sure that incorporating "backwards-compatible" code into the function itself is the best direction. In order to support an older version of the module, wouldn't it be best to just wholesale convert the settings using an update function?

danielb’s picture

I think it's fine

I mean I thought about those when I was doing it, and whilst this isn't perfect, it works and won't be a problem. In terms of being intuitive - it's not an api function for people to use, it does the job of the update without requiring a second function - that's cool. The secondary check shouldn't be too much of a performance hit because variable_get is pretty quick and that part of the code should only ever run once on a particular field anyway, because from then on the default or stored value will be in the new settings - eventually that secondary code can be removed. Doing a mass update would be a little harder too :/

Shawn DeArmond’s picture

Fair point.

Okay, I rolled a patch based on your code, though I did have to change a couple things. I tested it as if I was moving from 2.0 to 2.x-dev and it migrated perfectly.

In fact, an odd (but happy) side-effect of this patch is that it'll upgrade itself the first time node_access_rebuild() is called, since it goes through all the types and fields.

danielb’s picture

Status: Needs review » Fixed

thanks!

Status: Fixed » Closed (fixed)

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