hook_civicrm_post() implementation results in DB Error: already exists for custom field values
Overview
When an implementation of hook_civicrm_post()
creates custom field values for a newly created entity (e.g. a contribution), saving the entity results in
DB Error: already exists
with details
Duplicate entry '123' for key 'unique_entity_id', 1062
with 123 being the entity ID.
Reproduction steps
- Install an extension with an implementation of
hook_civicrm_post()
that creates a custom value for a newly created entity e.g.:/** * Implements hook_civicrm_post(). */ function test_civicrm_post( $op, $objectName, $objectId, &$objectRef ) { if ($op=='create' && $objectName=='Contribution') { civicrm_api3('CustomValue', 'create', array( 'entity_id' => $objectId, 'custom_123' => 'foobar', )); } }
- Try to create an entity of that type (UI or API)
- Notice the error
Environment information
CiviCRM >= 5.16.0
Technical background
Commit 46fe0a66 added a condition for the ON DUPLICATE KEY UPDATE
clause to only be added when the $parentOperation
is not 'create'
. However, hook_civicrm_post()
is being invoked before the transaction that creates the entity is committed and without any $parentOperation
. Thus, the hook implementation is the first to create a record in the respective custom group table. When the entity creation transaction is about to be committed, there already is a record and the INSERT
clause without ON DUPLICATE KEY UPDATE
fails with DB Error: already exists.
Proposed solution
The condition for not adding the ON DUPLICATE KEY UPDATE
is of course there for a reason. So maybe removing it is not the best solution. My feeling is that hook_civicrm_post()
should be invoked later, which might not be that easy, since adding custom field values may not apply to all entity types.
Handing down the $parentOperation
into the hook will not work, since anything can happen in there and the API (as in the example code above) will not respect it anyway.
Comments
Maybe there should be an extension in the testing framework that adds this scenario as a test case, so that changes to code that's invoking hooks can be more sensitive in respect to common or known use cases.