WIP dev/core#748 Clean up transaction handling for CRM_Case_Form_Activity #13672
Conversation
|
(Standard links)
|
| @@ -372,7 +372,9 @@ public function postProcess($params = NULL) { | |||
| } | |||
| } | |||
| else { | |||
| $statusMsg = ts("Selected Activity cannot be deleted."); | |||
There was a problem hiding this comment.
@mattwire so I feel like using the api would be more correct than doing rollback at the form layer.
It seems to me that activity.delete api should also delete entity tags within that action - so that bit of code should be moved into whatever activity.delete api calls (ideally with a test) & the form could call activity.delete rather than all this - which feels brittle
This is a reviewer's commit of civicrm#13672 I pulled out some lines I've checked & agree with to simplify that commit
| @@ -440,15 +444,12 @@ public function postProcess($params = NULL) { | |||
|
|
|||
| // build custom data getFields array | |||
| $customFields = CRM_Core_BAO_CustomField::getFields('Activity', FALSE, FALSE, $this->_activityTypeId); | |||
| $customFields = CRM_Utils_Array::crmArrayMerge($customFields, | |||
| CRM_Utils_Array::crmArrayMerge($customFields, | |||
There was a problem hiding this comment.
from a cleanup pov - this line can completely go can't it?
|
@mattwire I've pulled out a few lines to merge separately to reduce this PR to it's essence. My feeling is the form shouldn't be started a transaction at the start of postProcess - in general that doesn't feel like where transactions should be handled - I feel like if we refactor the 3? actions that might result from this form into their own functions then we could either start & end a transaction within each function or possibly use the api instead - esp the delete & restore ones feel like they should just be api calls whereas the bigger update fn might need a trxn wrapping for now unless we can move some stuff to the api |
5e54450 to
f4b45c3
Compare
|
Closing pending review |
Overview
This is part of a task to solve performance issues/deadlocks caused by smartgroups/ACLs described here: https://lab.civicrm.org/dev/core/issues/748
Transactions should be automatically terminated when PHP execution completes, but the longer we are in a transaction the longer we are locking tables that may be needed by other processes.
Before
When performing create, edit, delete actions on case activities the database transaction is not explicitly committed/rolled back so tables are locked for longer than they need to be.
After
When performing create, edit, delete actions on case activities the database transaction is committed/rolled back as soon as it is no longer needed so tables are locked for the minimum amount of time.
Technical Details
Use
$transaction->commit(),$transaction->rollback().Comments
@seamuslee001 Would appreciate your thoughts on this if you have time?
There is also some unused variable/code cleanup in here.