CRM-21391 Convert Case to use core Task class #11759
Conversation
|
This one seems pretty straight forward & an agreed improvement, merging |
| if (!self::$_tasks) { | ||
| self::$_tasks = array( | ||
| 1 => array( | ||
| self::TASK_DELETE => array( |
There was a problem hiding this comment.
@mattwire @eileenmcnaughton @colemanw (https://docs.civicrm.org/dev/en/latest/standards/review/#r-tech) - Doesn't this break anything that listens to hook_civicrm_searchTasks by virtue of changing the numbers?
Don't get me wrong... the old values are magic-numbers that don't make sense. They seem like a design-flaw. Lots of extensions use the idiom $tasks[] = array(...), which is more robust.
But... the example in the docs specifically uses the technique:
https://docs.civicrm.org/dev/en/latest/hooks/hook_civicrm_searchTasks/#example
And there are multiple examples in universe where hook_civicrm_searchTasks references numerical IDs/existing constants (biz.jmaconsulting.bugp/bugp.php, biz.jmaconsulting.printgrantpdfs/printgrantpdfs.php, civicrm-drupal-6.x/civitest.module.sample, uk.co.compucorp.civicrm.giftaid/civigiftaid.php). Anecdotally, I remember needing to do similar things in Civi 2.x (basically, as long as the hook has existed).
(Note: This isn't a pointed critique of #11759 -- it generally applies to all the offspring of #11240.)
There was a problem hiding this comment.
@totten Yes there is a chance it could break some extensions. However, the numeric indexes have been a bit inconsistent throughout versions and shouldn't really be relied on - once all the "Task" PRs are merged we'll have a consistent set of constants that extension authors can use, meaning that their extensions won't break in the future! If we can get them all merged before the 5.0.0 release we can have something in the release notes making extensions authors aware that they may need to tweak the searchTask constants if using hardcoded values in their extensions.
There was a problem hiding this comment.
As an aside of the ones linked
https://github.com/JMAConsulting/biz.jmaconsulting.bugp/blob/master/bugp.php only adds, as does https://github.com/JMAConsulting/biz.jmaconsulting.printgrantpdfs/blob/master/printgrantpdfs.php as does https://github.com/compucorp/uk.co.compucorp.civicrm.giftaid/blob/master/civigiftaid.php
the test one should go
|
|
||
| if (!empty($this->_formValues['case_deleted'])) { | ||
| unset($tasks[1]); | ||
| unset($tasks[CRM_Case_Task::DELETE]); |
There was a problem hiding this comment.
@mattwire @eileenmcnaughton change here cause regression as it should be TASK_DELETE
Steps to replicate:
- Go to Find Cases
- Try to search deleted cases
Error: Undefined class constant 'DELETE' in CRM_Case_Form_Search->buildQuickForm() (line 180 of /Users/monish/src/civicrm/CRM/Case/Form/Search.php).
There was a problem hiding this comment.
@monishdeb is that tracked somewhere?
(ie. a) good point thanks for letting us know but b) is fixing this under control ?)
Overview
Refactor case task form to use base class
Before
After
Technical Details
details in #11240
Comments
This is a commit from #11240
In terms of review check that the code was a positive improvement and tested a couple of actions as well as checking the same number of actions were present