Account for fields with column names other than 'value' [#1972150]

In various locations, the array returned by field_get_items() is accessed directly by $item['value'], which is not always correct. The countries module, for example, keeps its value in $item['iso2']. field_view_value() should be used to get the value, instead.

Comment	File	Size	Author
#8	simple_ldap-1972150-8.patch	29.69 KB	q0rban
#8
#6	simple_ldap-1972150-6.patch	18.47 KB	q0rban
#6

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

blc CreditAttribution: blc commented 15 May 2013 at 19:07

Using field_view_value() did not work so well, and it's probably best not to try to use a renderable array anyway.

One thought is to include it in the attribute map. Something like 'drupal' => '#field_country[iso2]'. I'm not sure how the "iso2" value would be discoverable by end users, though.

Comment #2

q0rban CreditAttribution: q0rban commented 15 May 2013 at 21:33

I don't think we want field_view_value() here. There is the case where you want the raw value stored in the column, not the display value. The real problem is that the column names don't match, as described in #1995602: Allow mapping of fields with columns other than value.

Comment #3

blc CreditAttribution: blc commented 15 May 2013 at 21:47

Title:

Use field_view_value() instead of accessing the item array directly

» Account for fields with column names other than 'value'

Comment #4

q0rban CreditAttribution: q0rban commented 28 May 2013 at 14:51

One possibility is to have the field mapping array optionally contain the column names in them.

For instance:


$variables['simple_ldap_user_attribute_map'] = array (
  array (
    'drupal' => '#field_firstname[value]',
    'ldap' => 'givenName',
  ),
  array (
    'drupal' => '#field_lastname[value]',
    'ldap' => 'sn',
  ),
  array (
    'drupal' => '#field_country[iso2]',
    'ldap' => 'country',
  ),
);

[value] would be implied, so this would also work:


$variables['simple_ldap_user_attribute_map'] = array (
  array (
    'drupal' => '#field_firstname',
    'ldap' => 'givenName',
  ),
  array (
    'drupal' => '#field_lastname',
    'ldap' => 'sn',
  ),
  array (
    'drupal' => '#field_country[iso2]',
    'ldap' => 'country',
  ),
);

Comment #5

blc CreditAttribution: blc commented 28 May 2013 at 22:51

I was hoping to avoid explicitly specifying the field column in the config, since that is a little difficult to find. But, after digging into field API more, I think this might be the only reasonable option.

Comment #6

q0rban CreditAttribution: q0rban commented 13 August 2013 at 19:21

Status:

Active

» Needs review

File	Size
simple_ldap-1972150-6.patch	18.47 KB

Ok, here is a patch that moves the mapping into it's own class.

@blc, a few things to note:

I changed that one place where it was only saving values from LDAP if they were different than what was on the user object, as I was thinking that created a hole for other modules to insert values into the edit array for those attributes. So, with this patch, it will always write what is in LDAP to what is in Drupal, no matter if the data already matches on the user. I think this is safer. Let me know what you think.
This still needs to have the simpletests run against it. I'm sure something will pop up from that, so let me know what needs to be fixed when you run those.

Comment #7

q0rban CreditAttribution: q0rban commented 13 August 2013 at 19:22

Also, I would carefully review this patch, as it looked like a few other commits went in while I was working on this, so I want to make sure I didn't break anything you did before.

Comment #8

q0rban CreditAttribution: q0rban commented 19 August 2013 at 17:51

File	Size
simple_ldap-1972150-8.patch	29.69 KB

Whoops, I forgot to git add the SimpleLdapUserMap class. Also, I've added some documentation and strictly declared some arguments since I last tested this, so I may have broken something in that process. Let me know if so, and I will re-roll.