Before I get into the actual bug report, let me state my assumptions to provide context, because, though I believe otherwise, I'm willing to concede the possibility that A) I may not be doing things the optimal way, and B) the way that I'm doing things may not be the only reasonable way.

I have a boolean value in a custom object in Salesforce (represented by a checkbox widget in the SF UI). On the Drupal side, I have a CCK integer field set to use the "Single on/off checkbox" widget, holding a single value, with "0|off" and "1|on" in the allowed values list. The field is included in the field mapping, which generally seems to work fine.

The issue occurs when editing the object on the Drupal side.

First, the value passes through the export handler for on/off checkboxes in sf_node.module, lines 383-392:

// Assume that a on/off checkbox is a SF boolean, which can never be set to NULL
function _sf_node_export_cck_checkbox($source, $field) {
  $data = $source->$field;
  if (empty($source->{$field}[0]['value'])) {
    return 'false';
  }
  else {
    return 1;
  }
}

Assuming a starting value of 0 for $source->$field (the checkbox is in the off position), the export handler will return the string 'false'.

Next, the value is passed through a switch statement inside salesforce_api_fieldmap_export_create() in salesforce_api.module, starting at line 980:

    switch ($type) {
      case 'boolean':
        if (empty($value)) {
          $object->$sf_fieldname = 0;
        }
        else {
          $object->$sf_fieldname = 1;
        }
        break;
        ...

The relevant bit here is the if condition. Given this particular code path, the "else" will always be executed, because a true input value turns into a 1 which is non-empty, and a false input value turns into the string 'false', which is also non-empty.

So, that was a really long-winded way of saying that I believe the single quotes should be removed from line 387 of sf_node.module, but there are enough assumptions in my reasoning that I'm not sure if doing so would break something elsewhere. That's why you're getting a long bug report, rather than a patch.

CommentFileSizeAuthor
#15 boolean_value_fix-1195824-15.patch879 bytesrjacobs
#10 boolean_value_fix-1195824-10.patch886 bytesaaronbauman
#3 boolean_value_fix-1195824-0.patch483 bytesmdunn

Comments

EvanDonovan’s picture

EvanDonovan commented
Version: 6.x-2.0-alpha7 » 6.x-2.x-dev
Issue tags: +D7 porting

Post bugs against the -dev release. Seems like you may be right. Aaron/Kosta, any thoughts?

mdunn’s picture

mdunn commented

For what it's worth, I've made the change I described in my own installation about a week ago, and it seems to be working fine so far.

In addition to removing the quotes from line 387, I changed 390 to be "return true;" for the sake of consistency.

mdunn’s picture

mdunn commented
StatusFileSize
new boolean_value_fix-1195824-3.patch483 bytes

Here's the patch file.

EvanDonovan’s picture

EvanDonovan commented
Issue tags: -D7 porting +7.x-2.x

Tracking for 7.x-2.x.

kostajh’s picture

kostajh commented
Status: Active » Needs review
EvanDonovan’s picture

EvanDonovan commented

I think Drupal standards are FALSE and TRUE. Is that correct?

There are lots of other places in the Salesforce modules where 1 is used for TRUE. It would be a nice cleanup to fix them all someday, but not necessary for this patch.

kostajh’s picture

kostajh commented

Yes, FALSE and TRUE should be capitalized.

In general I have been trying to run patches through Coder module before committing them and encourage people submitting patches to do the same.

jcarlson34’s picture

jcarlson34 commented

Just ran into this problem myself. The fix in #4 worked great.

EvanDonovan’s picture

EvanDonovan commented
Issue tags: -7.x-2.x

Should be re-rolled with FALSE & TRUE prior to commit.

7.x doesn't have a separate import/export handler for this, so it already behaves correctly.

aaronbauman’s picture

aaronbauman
he/him
commented
StatusFileSize
new boolean_value_fix-1195824-10.patch886 bytes

From the instructions for single on/off checkbox:

For a 'single on/off checkbox' widget, define the 'off' value first, then the 'on' value in the Allowed values section. Note that the checkbox will be labeled with the label of the 'on' value.

Therefore, we should actually be checking to see if the value is equal to either of the allowed values, rather than checking against empty().
This patch does that.

* If the source value does not match any allowed value, this patch returns nothing (which should actually do the same thing).

jcarlson34’s picture

jcarlson34 commented

The patch is working just fine for me so far. Time for RBTC status?

rjacobs’s picture

rjacobs commented
Status: Needs review » Needs work

The patch in #10 uses the following to calculate the on and off keys for a given checkbox field:

$content_field = content_fields($field);
$allowed_values = content_allowed_values($content_field);
$off_key = key($allowed_values);
array_shift($allowed_values);
$on_key = key($allowed_values);

However, I think this will break-down if the off key is 0 and the on key is 1, such as:

0|This is the off option
1|This is the on option

I think this is because array_shift re-numbers numerical keys. From php.net:

array_shift() shifts the first value of the array off and returns it, shortening the array by one element and moving everything down. All numerical array keys will be modified to start counting from zero while literal keys won't be touched.

So perhaps next() would be better than array_shift()?

$content_field = content_fields($field);
$allowed_values = content_allowed_values($content_field);
$off_key = key($allowed_values);
next($allowed_values);
$on_key = key($allowed_values);
rjacobs’s picture

rjacobs commented

Also, I believe that a similar issue exists for importing checkbox values. I've opened-up a separate issue for that at #1438948: Need (better) import handler for CCK checkboxes.

mtbosworth’s picture

mtbosworth commented

Removing the quotes from "false" on line 387 fixed my issue. Drupal 6

rjacobs’s picture

rjacobs commented
Status: Needs work » Needs review
StatusFileSize
new boolean_value_fix-1195824-15.patch879 bytes

I was just running some updates on a couple sites and realized this is still pending. Attached is a tweaked version of the patch from #10 that incorporates my point from #12.

aaronbauman’s picture

aaronbauman
he/him
commented
Component: sf_node » Code
Status: Needs review » Closed (outdated)

closing all 6.x issues