This issue brought me a bit of deja vu, probably due to my memories of #1096712: Null fields are not synchronized, however, I think this is unique.

What we are seeing is that fields mapped to a Salesforce field of type "number" are being nullified if they are set to zero (0) in Drupal (instead of the proper value of 0 being passed). This seems to relate to salesforce_api_fieldmap_export_create() where we have a test to set the $fieldsToNull[] variable:

elseif (!$value && $nillable && !empty($map->fields[$sf_fieldname])) {
  $fieldsToNull[] = $sf_fieldname;
  continue;
}

In the case of a Salesforce field type "number" it seems $nillable is TRUE, but of course !$value will also be TRUE for a value of 0. As a result, number values of 0 are not handled correctly. I think this would also lead to a problem with text fields with a value of "0", but I have not checked that.

When re-reading #1096712: Null fields are not synchronized Aaron said (in comment #9):

I have to use !$value instead of is_null($value) to correctly grab all the fields that should be set to NULL in Salesforce

So I figure there was a good reason is_null($value) was not used instead. If there is no way to use is_null() perhaps there needs to be some kind of added conditions like:

elseif (!$value && $nillable && !empty($map->fields[$sf_fieldname])) {
  // Additional check for nillable cases where zero is explicitly set and we
  // should not nullify
  if ($value !== 0 && $value !== "0") {
    $fieldsToNull[] = $sf_fieldname;
    continue;
  }
}

Or perhaps $nillable is not being set correctly?

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

AaronBauman’s picture

Version: 6.x-2.x-dev » 7.x-2.x-dev

For the record, that comment was Kosta's.
I assume he wanted to use !$value instead of is_null to capture the empty string.
(is_null checks for strict equality to NULL -- i had to look it up myself.)

In theory I'm in favor of checking for strict equality to NULL.
Something like:

elseif ($nillable && is_null($map->fields[$sf_fieldname])) {
  $fieldsToNull[] = $sf_fieldname;
  continue;
}

(Looks like that was the case up until #1096712.
Re-reading that thread, looks like it was closed without addressing this same concern which you brought up there).

However in practice this means there'd be no way to set SF fields to null without implementing a pre_export hook.
Anyone else have input?
Why did we get rid of is_null()?

For the time being, I can think of a number of workarounds - the simplest being either implementing a pre_export hook or setting the SF field to non-nillable.
I'm bumping this to 7.x because it's the exact same logic.

rjacobs’s picture

Ah, you are right... that wasn't you that said that. I was confused by all the queue references I was scanning last night.

In your snippit in #1 you wrote:

elseif ($nillable && is_null($map->fields[$sf_fieldname])) {

but did you mean?:

elseif (is_null($value) && $nillable && !empty($map->fields[$sf_fieldname])) {

This is the logic that generally makes the most sense in my mind, and would solve this, though Kosta seemed to be saying that this would cause issues. It would be interesting to hear what those issues were. It could be that some field handlers somehow just "empty" values (is_null($value) would be FALSE) that should be explicitly set to NULL when no input is captured?

kostajh’s picture

I assume he wanted to use !$value instead of is_null to capture the empty string.

As best as I can recall, yes this was the motivation for changing it. I'm fine with using is_null() as long as empty strings are handled. So we could check for an empty string and set that field to NULL before running through the check in #2.

rjacobs’s picture

Status: Active » Needs review
Issue tags: +6.x-2.x
FileSize
1.25 KB

So how about something like the attached patch? This patch is against D7 but I tested a D6 version of this on an active site and it seems to take care of things. I have not exhaustively tested all field types though.

kostajh’s picture

That looks good to me, thank you. Aaron what do you think?

AaronBauman’s picture

Status: Needs review » Reviewed & tested by the community

Yeah, looks good to me.
Is there somewhere appropriate in the documentation to indicate that you have to use NULL in order to set fieldsToNull?

rjacobs’s picture

Good point about noting this somewhere in the docs. Would this be most relevant to people working with the export callback in hook_fieldmap_objects()? I see that the D7 version does not have a hooks.php yet though.

rjacobs’s picture

Thinking more about the documentation... perhaps this is not that critical to officially document outside of the code itself? After all, with this patch I think the api would generally behave as expected (only NULL and "" items would get sent to $fieldsToNull).

rjacobs’s picture

I just wanted to check-in on this. Even without any documentation changes would it make sense to go ahead and commit this into dev?

kostajh’s picture

Status: Reviewed & tested by the community » Fixed

Committed, thanks @rjacobs

rjacobs’s picture

Version: 7.x-2.x-dev » 6.x-2.x-dev

Thanks! Now I think we just need to get this fix into D6...

rjacobs’s picture

Status: Fixed » Reviewed & tested by the community
FileSize
1.25 KB

D6 patch is attached. I'm making this RTBC again as the patch is almost identical (just different line numbers), and the change is no different in concept than what was reviewed and tested for D7. If a review is still required please feel free to change.

I just wanted to be sure to follow-through on this for D6 and D7. I (like others I assume) have active projects in both and won't be making a move to 7.x-3.x for a little while. Also note that the patches are git-formatted patches with include my authorship info. If this gets committed, I'd certainly love it if those author details were also in the commit (http://drupal.org/node/1146430). Cheers!

kostajh’s picture

Status: Reviewed & tested by the community » Fixed

Committed, 8a62c1d4d in 6.x-2.x. Thanks @rjacobs

rjacobs’s picture

Thanks Kosta for giving this such quick attention!

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

Anonymous’s picture

Issue summary: View changes

typo