EntityFinancialAccount - A tale of two BAO classes #25036
Conversation
|
(Standard links)
|
…ityFinancialAccount This BAO class did not follow the standard naming convention which effectively made it invisible to the API. Why was named this way in the first place was never explained, I suspect it was simply a mistake. A BAO class actually did get added before this PR, in 37c608f which further added to the confusion. This consolidates the two classes. Because it was an "invisible" BAO class, the create and del functions were never used by the API. For consistency, I've therefore tagged them as @deprecated which keeps them hidden from the API. We can add business logic back in as needed using hook listeners.
This follows the same pattern as the create action - to only use BAO methods if they are not @deprecated. Note that APIv4 already does this in the DAODeleteAction class.
|
You have to remove those redirects in favour of exceptions to get the tests to pass ( I think you can just do it - it's such an obscure entity that if the forms get fusty about it it probably won't be noticed for years) CRM_Contribute_Form_ContributionTest::testPremiumUpdate |
eee94c9 to
bb33fc6
Compare
| @@ -1395,7 +1395,7 @@ function _civicrm_api3_basic_create_fallback($bao_name, $params) { | |||
| function _civicrm_api3_basic_delete($bao_name, &$params) { | |||
| civicrm_api3_verify_mandatory($params, NULL, ['id']); | |||
| _civicrm_api3_check_edit_permissions($bao_name, ['id' => $params['id']]); | |||
| if (method_exists($bao_name, 'del')) { | |||
| if (method_exists($bao_name, 'del') && !\Civi\Api4\Utils\ReflectionUtils::isMethodDeprecated($bao_name, 'del')) { | |||
There was a problem hiding this comment.
Won't this alter other v3 api delete calls - in possibly unpredictable ways?
There was a problem hiding this comment.
(also - can we use a use ) - at least if you make further updates
There was a problem hiding this comment.
Yes, but I like to think that it will do so in predictable ways. 😁
As @aydun and I have been systematically deprecating BAO::del() functions, we've made them functionally identical to the base method deleteRecord() (in fact gutted them each to one line calling that method), so switching the API from one to the other should have no impact. And APIv4 has already made that switch. Let's see what the tests think about my prediction...
There was a problem hiding this comment.
OK - I'll let the tests decide
There was a problem hiding this comment.
Re a "use" statement. That makes me a little nervous (maybe unjustifiably so I think it's justified because the file itself sets up the classloader and that would happen after a use statement was evaluated) because api/v3/utils.php is not a class and is not in a namespace and gets included from random places that might crash if the require_once "api/v3/utils.php" gets called before our classloader is active.
Overview
This BAO class did not follow the standard naming convention. Why was never explained, I suspect it was simply a mistake. Then last year a new BAO was added that is correctly named, but without dealing with the existing one.
This moves all BAO code into the one with the correct name, and aliases the other.
Technical Details
The nonstandard name effectively made it invisible to the API so the create and del functions were never used by the API. For consistency, I've therefore tagged them as @deprecated which keeps them hidden from the API. We can add business logic back in as needed using hook listeners. @aydun has started work on that in #25026.
Comments
#19927 introduced that extra BAO class. CC @eileenmcnaughton and also the guy who merged it.