CiviCRM Community Forums (archive)

The intention was that code using api v2 should remain unchanged. However, for a period this wasn't working & the partial migration was done. Once it was working again (because we changed the apiv2 signatures) I don't think it was revisted.

Ideally all code that is using v2 would still call the functions directly & only code that has been checked would use civicrm_api.

<groan>I should have seen that coming!</groan>

Without wishing to appear to be taking this on, how would we scope this?

I understand 3.4.0 is the first release to use API v3? So would a 'diff' between 3.3.x and 3.4.0, then a 'grep' for civicrm_api. would that locate all changes?

And what would the hapless volunteer look for?

We could look for all cases where API v3 is included and then the API v2 calls are made?
Does anyone have a feel for which API v2 calls have side-effects?

Ken

A grep for civicrm_api will spot all the calls that now go through the new api structure.

Erik, the tests are pretty solid on the api itself but the test coverage on all the other functions that call the api is less

Yes, you are right - it would be great to improve it. Ideally find the functions that use civicrm_api, check if they have tests & if not, write them....

Hi,

The problem is real, but complex. how do you know that one api function was wrongly modifying the param X and that result wrongly used in the calling code? (wrongly as in that was a rule not to do it, but because it was passed by reference it could, and did, happen). To make it harder, it isn't systematic, ie the side effect can only happen in one branch of a if structure, or of a much more complex and specific configuration.

Moreover, some of the params value could have  been modified without being used (ie. we fixed a bug that no code tripped on), or in v3, we sometimes modify a param, knowing that it's a local modification because that's by value.

I don't know how to make that audit more systematic ( the naive grepping $params[.*] = probably wont cover everything), but I suspect that working on a better test coverage (mostly of the calling functions) would be a more valuable investment of our time.

Eg. when we find a bug, write a test (that would fail), fix the code and submit both the code and the test. It's probably not a big overhead if you know the test infrastructure (well and have the patience of letting them run, cf usual rant about their speed). I know that's best practice, and I know I'm guilty not doing it (but I'm firmly in the majority here, unfortunately).

The test infrastructure has greatly improved IMO and we have introduced a few tricks to make run tests in a few seconds, but only a few test suites benefit from them now.

Ken, don't take me wrongly, your fixes are super appreciated, was more a general comment and thinking that instead of an audit, writing tests to be sure that what you have fixed it going to be fixed forever (or at least visible if broken again) might be more useful in the long run.

X+

So, instead of a full code audit .. you could pick one instance & write a test for it to make sure it never gets broken again...

Folks,

As I've just released 3.4.0 into production, and I hate the thought of defects, I just couldn't resist looking at the code.

I looked for any situations where 'civicrm_api_include' is called to include the v3 API, yet the latest revision of the branch/3.4 code uses v2 calls ...

./bin/Email2Activity.php:111:            civicrm_api_include('activity');
./CRM/Activity/Import/Parser/Activity.php:39:civicrm_api_include('utils', false, 3);
./CRM/Bridge/OG/CiviCRM.php:94:        civicrm_api_include('uf_group', false, 3);
./CRM/Bridge/OG/Utils.php:85:        civicrm_api_include('uf_group', false, 3);
./CRM/Contribute/Page/UserDashboard.php:83:        civicrm_api_include('utils', false, 3);
./CRM/Mailing/Event/BAO/Subscribe.php:107:            civicrm_api_include('contact', false, 3);  ***** This v3 API isn't used, and the v2 API is included on line 119
./CRM/Profile/Selector/Listings.php:505:            civicrm_api_include('uf_group', false, 3);
./CRM/Utils/SoapServer.php:47:civicrm_api_include('utils', false, 3);      ***** This API doesn't appear to be used
./CRM/Utils/SoapServer.php:48:civicrm_api_include('mailer', false, 3);
./drupal/modules/civicrm_group_roles/civicrm_group_roles.module:114:    civicrm_api_include('uf_group'); ***** see the TODO - 'uf_match_id_get' doesn't exist in v2 or v3

Ken