Add entity_presave/insert/update sf pull hooks

Thank you for the submission.
There are already several similar patches in the issue queue about adding pre- and post- hooks.
Please review those and comment on those issues, or re-open this one to explain how these hooks address a different concern.

Little suggestion:
module_invoke_all("salesforce_pull_{$sf_mapping->drupal_entity_type}_presave", $wrapper->value(), $sf_object)

So hook implementation will be something like

function MY_MODULE_salesforce_pull_node_presave($node, $sf_object) {
  $node->uid = 1;
}

I appreciate this use case, and we need something like this to get in.

However, it needs at least 2 things:
1. in addition to the mapping object, entity, and entity type, the mapping should be passed to the hook invocation.
implementations may have different logic based on the mapping currently being used to pull an entity.

2. implementations need to be able to stop an entity from being saved.
See #2223669: Add "pull allowed" hooks, where hook implementations can return FALSE to stop / prevent processing of an entity.

Pretty minor changes in this patch.

Added sf mapping as an arg to the hook.

Since we're using a module_invoke_all(), we can't rely on implementations' return values.
And a drupal_alter doesn't really make sense.
So i updated the API with instructions to throw a SalesforcePullException to abort the pull.

lastly, i moved the api docs into the existing salesforce.api.php file, rather than adding a new api.php file.
i'm open for debate on this, but since there are already push and mapping hooks in there, pull hooks should go in there too.

On the subject of pull alterations I think there may be a use case that's not yet addressed here, namely the ability to "prematch" an incoming SF record with an existing Drupal record when a mapping object record does not yet exist linking these records. This is not really part of the specific use case expressed in the OP, but it does seem on-topic with the general utility for this new set of pull hooks.

hook_salesforce_pull_entity_presave() could be a solution, but with the current patch it does not allow altering/swapping the $entity param prior to the save (as that's tied to a protected data property on an inaccessible entity metadata wrapper). On possibility would be to change the signature of these hooks so they pass the whole wrapper ($wrapper instead of $wrapper->value()). That would give more flexibility to custom logic, but it would likely make the invoking logic much more fragile. I can think of a few other options, but they all involve throwing a SalesforcePullException in order to somewhat "hijack" the latter half of salesforce_pull_process_records(), which also feels a little hacky (especially when it leads to a bunch of unecessary watchdog errors for successful pulls).

I guess what I'm getting at is this... do we also need a hook to create and alter object mappings returned from salesforce_mapping_object_load_by_sfid()? Note that hook_entity_load() would be an obvious existing choice, but it's not an option when no matching object maps are found. Something like this could be part of this set of new pull hooks, or it could be more generalized within something like salesforce_mapping_object_load() (and as part of a separate issue). I'd be happy to help with either approach if this seems relevant.

You're right, but it's an easy fix i think.
The pull hooks should be alters, not module_invoke_alls, like kenorb's patch in #2186153: Provide additional Salesforce push/pull hooks for pre_export changes..

I believe the way the metadata wrapper works, changes to the $entity object will be reflected during $wrapper->save().

ie.

$entity = entity_load('entity', 1);
$wrapper = entity_metadata_wrapper('entity', $entity);
$entity = entity_load('entity', 2);
// This will save entity 2:
$wrapper->save();

I believe the way the metadata wrapper works, changes to the $entity object will be reflected during $wrapper->save()

That's initially what I thought as well, but I think it only works for altering a wrapped entity, not actually changing the entity that is wrapped. In other words I don't think the pointer of the wrapper's data property can be changed without actually manipulating the wrapper itself. So alterations made to $entity inside hook_salesforce_pull_entity_presave() would be preserved, but swapping the entity completely would not work.

Thanks for pointing out issue #2186153 as that gives a bit more history. Anyway, I suppose using alters would only provide an advantage if we wanted to allow passed arrays to be modified (like $sf_object), as I think object changes would still be supported with module_invoke_all (simple alterations to $sf_mapping and $entity).

Anyway, I think the use case that I am getting into would really be best served by providing an alteration to the object mapping before these other hooks take place. Interestingly not even issue issue #2186153 (which listed an abundance of hook suggestions) got into that. I wary of turning this back into hookfest 2016, but I could think of several use cases where having better control over the mapping object would be advantageous (more so for pulling, as pushing already has the "upsert" concept going for it).

I definitly don't want to hijack this issue with that concern though, as it looks like this whole pull hook addition discussion has been pending for a while. Maybe I should propose a separate drupal_alter() addition to deal with the mapping alteration in a separate issue so that this one can move forward? Or would you prefer to deal with that here given that it still falls under the umbrella of potential changes to salesforce_pull_process_records()?

Regarding #8-#11 I broke this off into #2745063: Add alter hooks to manipulate mapping objects on pull and push (for custom prematching, etc.). The more I think about it the more it seems like that is all tied to mapping logic that probably has implications on both push and pull operations, so it's probably best to treat it in isolation.

The patch from #7 was missing a semicolon in the hook_salesforce_pull_entity_presave() example in salesforce.api.php. This obviously has no impact on any functionality, but it will trigger lots of warnings in an IDE. Here's an update.

module_invoke_all('salesforce_pull_entity_presave', $wrapper->value(), $sf_mapping->drupal_entity_type, $sf_object, $sf_mapping);

sending entity_type is redundant, since any implementations can grab it from sf_mapping.

other than that, this is ready to go.

sending entity_type is redundant, since any implementations can grab it from sf_mapping

That makes sense. Here's an update without that param.

Counterpoint to the last comment and change... but it's more predictable if it's modelled after hook_entity_presave, and that is hook_entity_presave($entity, $type). Though I guess almost everyone is using an IDE now.

(Also, hello Salesforce Suite people.)

Great, thanks Ryan.

Jeff, thanks for your input.
I'm not super concerned about matching core entity hooks' API since we're already so far differentiated.