"String data, right truncated" when pulling Salesforce DateTime

Thanks for the bug report.

You're right, ::pull is not a good place for this.
Per the comment there:

// @TODO: The field plugin should be in charge of setting its value on an entity, we should not assume the field plugin's logic as we're doing here.

Instead of what we're doing now, which is essentially setting the raw value directly on the drupal entity, we should delegate it to the mapping field plugin.

This would mean:

• updating the interface Drupal\salesforce_mapping\SalesforceMappingFieldPluginInterface with a new method like "pullValue(SObject, EntityInterface, SalesforceMappingInterface)
• providing a default implementation in Drupal\salesforce_mapping\SalesforceMappingFieldPluginBase
• providing overrides as necessary in Drupal\salesforce_mapping\Plugin\SalesforceMappingField\* classes

Let me know if that's enough to get you started.

Taking a look now.
Let's see what testbot says too.

I'm not clear on whether RelatedProperties and RelatedIDs require an override for the pullValue() method (pushValue() is not overridden). I don't have data handy to test those with but can get something set up if it looks like the default implementation won't work.

These plugins probably need overrides in both directions.
I've been mostly working with Properties, so haven't given them the attention they deserve.

Looks like you're headed in the right direction so far - thanks for the patch!

+    if (!$this->pull() || empty($this->config('salesforce_field'))) {
+      return $value;
+    }

Maybe we should throw an exception here, rather than returning a value, so that the caller knows for sure that this value doesn't make sense.

+    try {
+      $field_definition = $describe->getField($this->config('salesforce_field'));
+    }
+    catch (\Exception $e) {
+      $this->eventDispatcher->dispatch(SalesforceEvents::WARNING, new SalesforceWarningEvent($e, 'Field definition not found for %describe.%field', ['%describe' => $describe->getName(), '%field' => $this->config('salesforce_field')]));
+      // If getField throws, however, just return the raw value.
+      return $value;
+    }

Same here. Let's allow this exception to bubble up so that the caller doesn't think our transformation was successful.

+    if (is_string($value) && $field_definition['length'] > 0 && strlen($value) > $field_definition['length']) {
+      $value = substr($value, 0, $field_definition['length']);
+    }

Let's put this into a default case.
substr() doesn't make sense for the other cases of the switch statements.

In the meantime, I'll fix our broken test coverage...

It looks like SalesforceMappingFieldPluginBase::pullValue() is manipulating the values based on what the Salesforce field definitions are rather than what the Drupal field definitions are. Since the data is intended to end up in Drupal fields shouldn't we be looking at that? The most obvious example of an issue is the default case where the value from Salesforce is trimmed to the max length the Salesforce field defines - in other words nothing would happen.

Agreed that related entities and their properties will need an override to map the SF Id to a Drupal Id.

Updated patch to include RelatedIds override.

+    try {
+      $field_definition = $describe->getField($this->config('salesforce_field'));
+    }
+    catch (\Exception $e) {
+      $this->eventDispatcher->dispatch(SalesforceEvents::WARNING, new SalesforceWarningEvent($e, 'Field definition not found for %describe.%field', ['%describe' => $describe->getName(), '%field' => $this->config('salesforce_field')]));
+
+      throw new SalesforceException(
+        sprintf('Field definition not found for %s.%s', $describe->getName(), $this->config('salesforce_field'))
+      );
+    }

I think we don't need to handle this exception at all.
Just allow it to bubble up, and allow the caller to dispatch an event as applicable.

I'll just echo Tauno's point again, and add some specific detail.
We definitely need some manipulation based on both Drupal and Salesforce fields, e.g. for multipicklist and booleans.

+      case 'date':
+        $tmp = $value;
+        if (!is_int($tmp)) {
+          $tmp = strtotime($tmp);
+        }
+        if (!empty($tmp)) {
+          $value = $this->dateFormatter->format($tmp, 'custom', DATETIME_DATE_STORAGE_FORMAT, 'UTC');
+        }
+        break;
+
+      case 'datetime':
+        $tmp = $value;
+        if (!is_int($tmp)) {
+          $tmp = strtotime($tmp);
+        }
+        if (!empty($tmp)) {
+          $value = $this->dateFormatter->format($tmp, 'custom', DATETIME_DATETIME_STORAGE_FORMAT, 'UTC');
+        }
+        break;

For dates, let's not bother changing the format at this point.
If, for example, i have a SF date field mapped to a Drupal textfield, I don't need to do any manipulation.
If I'm mapping a SF datetime to a Drupal date, I need to strip off the "time" component.

+      case 'double':
+        $value = (double) $value;
+        break;
+
+      case 'integer':
+        $value = (int) $value;
+        break;
+
+      case 'multipicklist':
+        if (!is_array($value)) {
+          $value = explode(';', $value);
+          $value = array_map('trim', $value);
+        }
+        break;
+
+      case 'id':
+      case 'reference':
+        if (empty($value)) {
+          break;
+        }
+        // If value is an SFID, cast to string.
+        if ($value instanceof SFID) {
+          $value = (string) $value;
+        }
+        // Otherwise, send it through SFID constructor & cast to validate.
+        else {
+          $value = (string) (new SFID($value));
+        }
+        break;

These are fine I think, but we also need to ensure they make sense for Drupal.
For example, if a SF double is mapped to a Drupal integer, it needs to be re-cast.
Or, if multi-valued SF multi-picklist is mapped to a single-valued Drupal field, we need to reconcile that as well.

Thanks again for your work on this.

Missed a use statement in the patch in #12.

Okay, so essentially we would need conditional handling inside of each Salesforce type depending on the Drupal field type that it is mapped to. That makes sense in a lot of cases (like the above).

Yeah... this could get messy though, so let's not go too crazy.
IMO if there's anything that's not obvious to map, throw an exception and let the user sort it out.

My concern is that bi-directional sync would cause data loss.

Yeah, there's a lot of ways that folks could set up mappings that would break things badly.
There's only so much we can take responsibility for defending against though, and I'd say this example is out of scope.

A couple changes, biggest of which is to swap the hard-coded "value" with getMainPropertyName()
Also a missing "use" statement (should have been caught by tests - obviously we need more coverage).

Otherwise this is the same.

This is going to change when we update mapping field plugins to accommodate properties: #2899460: Handling of field properties
But I think it's probably fine to get this in now.