Skip to content

(dev/core#1433) move post hook to avoid errors with custom fields created in the hook #16061

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 9, 2019
Merged

(dev/core#1433) move post hook to avoid errors with custom fields created in the hook #16061

merged 1 commit into from
Dec 9, 2019

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Dec 9, 2019 •
edited
Loading

Overview

Fixes a regression from around 5.16 - Per dev/core#1460 a fatal error can occur if custom data is created in the post hook as we now assume we can INSERT on new contributions. From a WMF point of view the change significantly reduced deadlocks but it causes an issue if someone creates custom fields in the post hook and then also in the main flow as it is no longer new

Before

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

After

No error

Technical Details

I think this makes sense - I only found 2 places that call add directly & I don't think they will be hurt however some testing on the main contribution page flow is needed as one is in 'Confim.php'.

I think makes sense to include custom data before the rollback

Comments

@civibot
Copy link

civibot bot commented Dec 9, 2019

(Standard links)

@civibot civibot bot added the 5.21 label Dec 9, 2019
seamuslee001
seamuslee001 reviewed Dec 9, 2019
View reviewed changes
CRM/Contribute/BAO/Contribution.php Outdated
@@ -90,6 +90,8 @@ public static function add(&$params, $ids = []) {
if (empty($params)) {
return NULL;
}
$contributionID = CRM_Utils_Array::value('contribution', $ids, CRM_Utils_Array::value('id', $params));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

isn't this duplicating L96 and if so should the comment be moved up above this line and L96 removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep

seamuslee001
seamuslee001 reviewed Dec 9, 2019
View reviewed changes
CRM/Contribute/BAO/Contribution.php
@@ -512,14 +513,6 @@ public static function create(&$params, $ids = []) {
throw $e;
}

$params['contribution_id'] = $contribution->id;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't look to me that $params is picked up from self::add so maybe need this line still?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

or maybe it is but i'm just confuzzled

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I put it back - this is the rc & possibly a live patch so conservative is better

@eileenmcnaughton
dev/core#1460 move post hook to avoid errors with custom fields creat…
80e74f7 
…ed in the hook

Per dev/core#1460 a fatal error can occur if custom data is created in the post hook. In addition I note it makes sense to
include custom data before the rollback
@seamuslee001
Copy link
Contributor

I have tested this and confirmed it works and have generated a unit test #16063 to lock in the fix

@seamuslee001 seamuslee001 merged commit e10329e into civicrm:5.21 Dec 9, 2019
@seamuslee001 seamuslee001 deleted the cont_hook branch December 9, 2019 23:14
@totten
Copy link
Member

totten commented Dec 10, 2019

Shouldn't this be dev/core#1443 instead of dev/core#1460?

@seamuslee001
Copy link
Contributor

@totten yes it should. Eileen and I have probably gotten turned around on the different lab tickets on hooks

@totten totten changed the title dev/core#1460 move post hook to avoid errors with custom fields created in the hook (dev/core#1433) move post hook to avoid errors with custom fields created in the hook Dec 10, 2019
@seamuslee001 seamuslee001 mentioned this pull request Jan 2, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@seamuslee001 seamuslee001 seamuslee001 left review comments

Assignees
No one assigned
Labels
5.21
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eileenmcnaughton @seamuslee001 @totten