Add DAO::writeRecord and DAO::deleteRecord methods

[colemanw]

Overview

Adds generic save and delete functions to the base DAO class where it is inherited by each BAO.

Before

Generic functions for save & delete in the api layers.

After

Generic functions for save & delete now in the DAO layer where it's more centrally useful.

Technical Details

Per discussion on https://lab.civicrm.org/dev/core/issues/1648 this is with an eye toward deprecating and eventually removing all the ad-hoc create, add, and del functions in each BAO, in favor of handling pre/post logic through event listeners.

Comments

As the APIv3 code comment said, "There's an intuitive sense that this behavior should be defined somehow in the BAO/DAO class". Now it is :).
Honestly I think the main hangup all these years was thinking of a good name for these functions that didn't conflict with any existing BAO function name.

[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).

[eileenmcnaughton]

@colemanw so I have been thinking it might make sense for this new function to take 2 params with the second being $checkPermissions = TRUE

I guess as long as it's only called by the api & we get it right at the api layer it's OK - but I thought it would harder to miss as a separate param & defaulting to 'true' would be good practice

[eileenmcnaughton]

OK - looking at it it's pretty clearly called by the API only - & it all makes sense & passes tests - merging

[colemanw]

@eileenmcnaughton I agree that it makes sense for $checkPermissions to be the 2nd param.
Trouble is, it currently doesn't! Some logic would need to be added to both functions to enforce permissions. I'm not against that, just don't want to add a misleading param prematurely.

If we can get these functions consistently handling permissions, then I'd say yes the 2nd param is clearer (and avoids muddling fields with settings in the $record array).

[eileenmcnaughton]

@colemanw it's really a case of moving some logic from the apis to the DAO isn't it?

[colemanw]

More or less. Permission checking in civicrm is kinda all over the place. It would be good to articulate the different types of permission checks and the different layers at which we want them to happen. For example, I don't think these functions should be doing the "gate keeper" api permission checks to see if the user can use a particular api action at all. But there are a few other types of checks (ACLS, related entity permissions...) that probably could be handled at this layer? Feels like too much to put in paragraph form and we need a chart...