The latest version of NAUR gives me strange behaviour:
- when saving the field settings of a 'user reference' field, the Drupal variable_set() is not invoked, so the variable is not changed.
- when showing the field settings again, the saved values are shown again, so it seems they are saved somewhere, but not via variable_get()

I have not checked yet if permissions are broken because of this, but IIRC the variable_set/get() is the way to store the data, as does NANR.

I have compared both modules (which are now very nicely aligned, Thank you!), and the following lines seem suspect to me:

function nodeaccess_XXXXreference_field_update_instance($instance, $prior_instance) {
  . . . 
     if ($old_settings != $new_settings) {
// the following line should exist in NAUR
       nodeaccess_nodereference_field_settings($instance['bundle'], $instance['field_name'], $new_settings);
// the following line is correctly missing NAUR
       nodeaccess_nodereference_check_cross_access();

  . . . 

one minor difference:

function nodeaccess_nodereference_field_settings($bundle_name, $field_name = NULL, $variable = NULL) {
  $data = variable_get('nodeaccess_nodereference', array());

vs.

function nodeaccess_userreference_field_settings($bundle_name, $field_name = NULL, $variable = NULL) {
  $data = variable_get('nodeaccess_userreference', NULL);

This findings still do not explain how the field settings screen recall the previous settings.

Comments

johnv’s picture

Hmm,

In below function, $data holds the field data, and $current holds the variable data.
Then, $current is not used (anymore?) to fill the field settings, but only to set the collapsed state.
- Is the field data then stored redundantly? Isn't this error prone?
- Was the whole task of minifying the variable superfluous, and can we better rely on function field_info_instance($entity_type, $field_name, $bundle_name) ?

function nodeaccess_XXXXreference_form_field_ui_field_edit_form_alter(&$form, $form_state) {
  . . . 
    $data = $form['#instance']['settings']['nodeaccess_userreference'];
    $current = nodeaccess_userreference_field_settings($form['#instance']['bundle'], $form['#instance']['field_name']);
    $form['instance']['settings']['nodeaccess_userreference'] = array(
      '#type' => 'fieldset',
      '#title' => t('Node access user reference'),
      '#tree' => TRUE,
      '#collapsible' => TRUE,
      '#collapsed' => empty($current),
    );

danielb’s picture

There was a line missing which saved the settings, woops, I've committed a fix.

Yes the settings are redundant, it is easier to iterate, and I assume speeds up hook_node_access(). I'd reconsider, but I'd prefer to just get it working now and leave it until a rewrite or something.

johnv’s picture

Status: Active » Fixed

Thanks.
Now that it is working now, its fine. Redundancy is not a bad thing per se.
Perhaps some optimalisation for the D8-version ;-)

Status: Fixed » Closed (fixed)

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