[REF] APIv4 Notes - Ensure child notes are deleted with parent, and hooks are called #21208
Conversation
|
(Standard links)
|
colemanw
force-pushed
the
refNotes
branch
2 times, most recently
from
1945896 to
3aa12ec
Compare
CRM/Core/BAO/Note.php
Outdated
| $note->id = $id; | ||
| $note->find(); | ||
| $note->fetch(); | ||
| if ($note->entity_table == 'civicrm_note') { |
There was a problem hiding this comment.
Honestly I would rip out this status stuff & just fix the 4 places that call this to set the status after it is called. The 'comment' part seems like a really long ago feature idea that never amounted to much
|
Ok @eileenmcnaughton that was more refactoring that I had planned on given the ever-creeping scope of getting APIv4 delete functions working, but you know I can never resist a good cleanup challenge. I've refactored out all core uses of |
|
Thanks @colemanw - I'll take a look. I just want to prod around the function call a little - I was expecting you would switch to calling apiv4 rather than calling deleteRecord directly. They are functionally the same & the v4 is slightly more verbose so my reasoning was more around modelling consistent behaviour in core forms that we would also hope extension forms would use |
|
That deprecation is a bit much for the tests api\v4\Entity\NoteTest::testDeleteWithChildren /home/jenkins/bknix-dfl/build/core-21208-7ccy4/web/sites/all/modules/civicrm/CRM/Core/Error.php:1044 |
|
Oh that's interesting. Looks like APIv4 does call So the reason for the original problem in dev/core#2757 is simply that the Contact BAO doesn't have a method with that name. Ok I'm going to put up another PR for that in a minute. I'll revert the noisy deprecation from this one to make it mergeable. |
…called Deprecates the CRM_Core_BAO_Note::del() function and refactors out all references to it. Related to work on dev/core#2757
|
@eileenmcnaughton want to give this one MOP? |
|
@colemanw I'm happy to leave changing it out of scope for this PR - but did you have a response about what syntax we should be modelling - ie calling the v4 api vs by-passing it? I worry about extension writers copying tendencies hence I prefer we try to use the v4 api as best practice - but you might disagree? |
|
I don't have strong feelings about it, but we should document that Extensions should always use the API, and never treat core BAO code as an example to be emulated 😝 |
|
@colemanw yeah - but in reality? I think they look to core code & copy & paste. Anyway I won't hold this up on that but I do think we should write core code as if it's being copied |
|
@eileenmcnaughton I guess so. I feel ambivalent about calling the API layer from the BAO layer because that feels like going from low to high when the normal flow is high to low. We wouldn't ever e.g. want to call the create api from a BAO::create function because the api then calls the BAO function and we have an infinite loop. With form and page code however I think it's fine. |
|
@colemanw yeah - but we routinely call OTHER create api from withing BAO creates - you are talking about a case where the function is calling itself - which is where we wind up using lower level functions like direct dao or direct sql |
Overview
Note BAO refactoring related to work on dev/core#2757
Companion to #21204
Before
After