APIv4 - Use correct BAO delete function (fixes dev/core#2757)

[colemanw]

Overview

Fixes the contact delete bug noted in dev/core#2757 and gives us a way to noisy-deprecate BAO::del() functions without tripping up APIv4 tests.

[civibot]

(Standard links)

• If this is your first pull-request for CiviCRM, please browse CONTRIBUTING.md for information about the development and testing processes.
• If you are reviewing this pull-request, you may wish to consult the test sites and the Review Standards (long template, short template).

[colemanw]

@eileenmcnaughton I'd say based on the previous test run on this PR that there is existing test coverage. I messed up the isMethodDeprecated function and it tripped a couple tests. Have now fixed that function and added a test for it!

[eileenmcnaughton]

I think the test fail relates @colemanw

api.v4.Entity.ConformanceTest.testConformance with data set "Contact" (from api_v4_Entity_ConformanceTest__testConformance)

Failing for the past 1 build (Since #43291 )

Took 0.47 sec.

Stacktrace

api\v4\Entity\ConformanceTest::testConformance with data set "Contact" ('Contact')
Entity "Contact" was not deleted
Failed asserting that 1 matches expected 0.
/home/jenkins/bknix-dfl/build/core-21232-7cxac/web/sites/all/modules/civicrm/tests/phpunit/api/v4/Entity/ConformanceTest.php:464

/home/jenkins/bknix-dfl/build/core-21232-7cxac/web/sites/all/modules/civicrm/tests/phpunit/api/v4/Entity/ConformanceTest.php:158

/home/jenkins/bknix-dfl/extern/phpunit8/phpunit8.phar:671

[colemanw]

Got it passing @eileenmcnaughton !

[eileenmcnaughton]

@colemanw I tested this and it still fails - #21137 - the reason is that apiv3 (but not v4 with this PR) deletes a contact in accordance with the sites trash policy unless the api caller forces it to hard-delete. I think that is actually the right approach - ie the default should be delete to trash.

Separately (& probably out of scope) I think a change could be made to the deletecontact to better handled related memberships when deleting to trash - but that's a bit more wonky since the UI forces soft delete first so the process is already done

[colemanw]

Whew that's a tough one @eileenmcnaughton.

We have a few different entities with an is_deleted column and they all behave slightly differently. In my mind, the UI is one matter, but as far as the API is concerned, "Delete" means "Delete." As in, "It's gone because we told the API to delete it." That's currently what the SyntaxConformanceTest expects.

But to address your concern, what do you think about these options:

1. Adding new API actions like "Trash" and "Undelete".
2. Adding a new param to the delete action like "useTrash".
3. Just telling people to use the update action and set is_trash: true to put a contact in the trash.

The 3rd option is of course my favorite because it doesn't involve any work on my part, but I understand you might feel differently :)

[totten]

The conflict makes sense to me if I imagine two different kinds of development tasks:

• Developing application UIs. We're writing code to act on behalf of a user (eg implementing the "X"/"Delete" buttons). For these, it's usu preferred to toggle is_deleted if the entity supports it - but we'd be satisfied with a straight-delete.
• Developing data-integrations. We're conscious about data lifecycle. For these, it's usu preferred to DELETE (regardless of whether is_deleted is a thing).

With #3, my concern would be this: What happens when an entity changes - gaining or losing support for is_deleted or similar trash-can mechanism? If the future data-model is revised to allow is_deleted, then (guesstimate) half the callers will become quietly wrong. (They'll wrongly send DELETE when the proper behavior would be to send is_deleted=1.) There is a subtle conflict between the conventions "API should do exactly what I say" and "API should be backward/forward compatible". It seems to me one should offer three behaviors:

• Open-minded removal (If the current version supports trashing this entity, do that. If not, do the full delete.)
  • This is likely the best mode when developing application UIs.
• Affirmative delete (Send SQL DELETE. Do not touch is_deleted.)
  • This is likely the best mode when developing data-integrations.
• Affirmative trash (Send UPDATE is_deleted. Do not send DELETE. If this isn't supported, raise an error.)

Here's a variation on#2 which provides those three behaviors:

• There is one API action, delete, for any entity.
• The action accepts an option useTrash. It can specify one of three behaviors (open-minded removal, affirmative delete, affirmative trash -- eg useTrash=required|optional|bypass or useTrash=TRUE|FALSE|optional).
• In the APIv4 Explorer, whenever a dev chooses action delete, the Explorer actively recommends passinguseTrash=optional -- like it currently recommends limit=25. (It's a similar tension - you're not required to set a limit, but most callers should be intentional about the limit. Omitting the option is likely to create confusion.).
• If useTrash is not specified, then choose whichever behavior provides continuity (pre-5.42-behavior). I'm guessing that means default to useTrash=FALSE/useTrash=bypass.

[highfalutin]

This is the newest aspect of an ongoing, bigger question. It has probably been discussed publicly somewhere at some point, but I'm not aware of it. The question is, what should the API do, in a broad sense? Should the API perform all the same business logic that gets performed when actions are taken in the UI? Or should it be a lean, clean CRUD machine -- essentially a 1:1 SQL wrapper with no business logic?

In CiviCRM, "save" often doesn't just mean save -- it often means "process the input depending on various factors, save the resulting entity, and possibly create/update/delete several linked entities". And, more relevant to the discussion here, "delete/deleted" means various things in various places, as @colemanw mentions. This business logic -- a pretty thick (and sometimes convoluted) layer between input and database -- is a big part of what makes Civi smart (and it can also make devels tear their hair out when they need to make a customization).

In working with API3 and now API4, I've gotten the impression that the answer to "what should the API do?" has changed. Version 3 seemed to include business logic in its "CRUD" operations, at least sometimes, and version 4 seems to be avoiding that as much as possible. API4 conforms much more tightly to the shape of the MySQL database layer, using a structure borrowed directly from SQL where feasible. As I understand it, this is an attempt "to reduce ambiguity and improve flexibility and consistency" as stated in the docs.

Again, sorry if I'm re-treading old ground in this wordy post. But it seems worthwhile to ask what our guiding principle is. (@eileenmcnaughton, @colemanw or others, can you link to that if it exists?) If it's to make the API as transparent and internally consistent as possible, then "delete" should always mean, literally, "delete". A separate "move to trash" action could be provided.

That's probably my preferred solution (Coleman's option 1 above). And maybe we should consider that it's the UI that's ambiguous! On sites where "trash/undelete" is enabled, we could change the UI: instead of a "Delete Contact" button, call it "Move to Trash".

[highfalutin]

To be clear, while there are some things I like about API4 being a thinner layer over the database, it would also be great if it provided access to the complex logic that's present in the application. What I'm advocating is for those to be clearly separate actions. I think we should avoid overloading a small set of actions with a bunch of flags that switch their behavior.

[highfalutin]

Aaaand, when spoke out against flag-switches just now, I hadn't read @totten's idea -- which actually sounds like a justifiable use of a flag 🎏

[colemanw]

Ok, I've combined the suggestions from @eileenmcnaughton and @totten and myself into this:

This does represent a change to APIv4, as the default behavior is now $useTrash = TRUE. Here's what led me to that point:

• The previous behavior of permanent deletion with no business logic (skipping the BAO altogether) was very incorrect. No matter what, a change is needed.
• @totten's trinary flag seems nice but the BAO::deleteContact function doesn't support it and I didn't want to spend the day fighting with that function.
• Defaulting $useTrash to FALSE means an uphill battle with the unit tests.
• The kicker is that BAO::deleteContact will fail if you ask it to skip trash and the user does not have 'delete contacts' permission. I don't think setting a default that will cause failure in some cases is a good idea.

[colemanw]

Retest this please

[artfulrobot]

Prompted by @eileenmcnaughton's helpful dev digest...

@totten I didn't understand

The action accepts an option useTrash. It can specify one of three behaviors (open-minded removal, affirmative delete, affirmative trash -- eg useTrash=required|optional|bypass or useTrash=TRUE|FALSE|optional).

Are those listed in respective order? I'm assuming not.

And does "optional" mean "use trash if supported by the entity"?

@colemanw useTrash = TRUE (or is it use_trash): what happens when the entity does not have trash functionality?

@highfalutin +1 for your idea re the business logic. I think an API first aproach is really helpful, and that UIs should use those APIs. I too like that API4 feels "lower level"/more SQL like. I'd like the basic CRUD to be pretty low level and other actions to expose higher level business stuff, but I guess it will always be a balance/bit blurred.

[colemanw]

useTrash = TRUE (or is it use_trash): what happens when the entity does not have trash functionality?

Params are camelCase, fields are snake_case, and useTrash is a param not a field :)
Entities without trash functionality will not have the useTrash param and if you try to set it the api will throw an exception.
I suppose we could be more forgiving by adding a setUseTrash() function to the base class that does nothing but throw an exception if you pass anything but false. There's already a precident for that in APIv4 - you can do this ->setVersion(4) and it ignores you because version is already 4, but if you do ->setVersion(5) it will throw an exception.

[colemanw]

On reflection, I feel like adding a dummy setUseTrash function would be false-advertising, since IDEs would show a setter for a param that doesn't exist. We could do something clever in the __call() function but that feels like black magic.

The standard for APIv4 to-date is that it's very strict about enforcing params, and I'm hesitant to start relaxing that due to problems that don't yet exist.

My vote would be to merge this as-is.

[colemanw]

@eileenmcnaughton I still feel this is merge-ready, and the status-quo behavior that it fixes is very very wrong.

[eileenmcnaughton]

@colemanw - I finally re-visited this with a bit of headspace and I agree with the conclusion you came to. This is consistent with the v3 api & UI and like the v3 api & BAO there is an option to force delete.

I don't think the discussion above is blocking.

I did notice a couple of things

1. I think a docs update is needed
2. without declaring the method in the Delete action it is not discoverable. I think maybe if the property were in the Delete class not the trait some magic might happen - but absent that I guess it needs a docblock update.

I think this is mergeable - will re-run tests (test this please) and let you re-check the above but giving merge-ready

[colemanw]

@eileenmcnaughton I've rebased this and also added the missing annotation so the param is discoverable in the IDE.

[eileenmcnaughton]

@colemanw OK - & it's been a couple more days so anyone who wanted to scream could have - merging

[eileenmcnaughton]

@colemanw I do think we want something in docs still

[colemanw]

@eileenmcnaughton I've added it to the changelog

[eileenmcnaughton]

@colemanw ok - but in this case I was more imaging in the api docs since it's a behaviour people probably need to understand. Ah well win some

[highfalutin]

@eileenmcnaughton let me know where to add it and I'll write something

[eileenmcnaughton]

@highfalutin maybe under the delete action? https://docs.civicrm.org/dev/en/latest/api/v4/actions/ - but I'd be interested to see where you think you would look to find it too

[highfalutin]

@eileenmcnaughton that's where I'd look, along with the explorer and my IDE.

https://lab.civicrm.org/documentation/docs/dev/-/merge_requests/977

[eileenmcnaughton]

thanks @highfalutin !!!