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?
Comment | File | Size | Author |
---|---|---|---|
#12 | D6_salesforce-dont_nillify_all_empty_fields-1780874-12.patch | 1.25 KB | rjacobs |
#4 | salesforce-dont_nillify_all_empty_fields-1780874-4.patch | 1.25 KB | rjacobs |
Comments
Comment #1
AaronBaumanFor 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:
(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.
Comment #2
rjacobs CreditAttribution: rjacobs commentedAh, 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?
Comment #3
kostajh CreditAttribution: kostajh commentedAs 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.
Comment #4
rjacobs CreditAttribution: rjacobs commentedSo 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.
Comment #5
kostajh CreditAttribution: kostajh commentedThat looks good to me, thank you. Aaron what do you think?
Comment #6
AaronBaumanYeah, looks good to me.
Is there somewhere appropriate in the documentation to indicate that you have to use NULL in order to set fieldsToNull?
Comment #7
rjacobs CreditAttribution: rjacobs commentedGood 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.
Comment #8
rjacobs CreditAttribution: rjacobs commentedThinking 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).
Comment #9
rjacobs CreditAttribution: rjacobs commentedI just wanted to check-in on this. Even without any documentation changes would it make sense to go ahead and commit this into dev?
Comment #10
kostajh CreditAttribution: kostajh commentedCommitted, thanks @rjacobs
Comment #11
rjacobs CreditAttribution: rjacobs commentedThanks! Now I think we just need to get this fix into D6...
Comment #12
rjacobs CreditAttribution: rjacobs commentedD6 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!
Comment #13
kostajh CreditAttribution: kostajh commentedCommitted, 8a62c1d4d in 6.x-2.x. Thanks @rjacobs
Comment #14
rjacobs CreditAttribution: rjacobs commentedThanks Kosta for giving this such quick attention!
Comment #15.0
(not verified) CreditAttribution: commentedtypo