Afform - Fix saving joined entities (email, address, phone, etc) #20264
Conversation
|
(Standard links)
|
colemanw
force-pushed
the
afformJoinSaveFix
branch
from
2572d28
to
37b2ebd
Compare
| $api4($joinEntityName, 'replace', [ | ||
| civicrm_api4($joinEntityName, 'replace', [ | ||
| // Disable permission checks because the main entity has already been vetted | ||
| 'checkPermissions' => FALSE, |
There was a problem hiding this comment.
So this seems to be the guts of the pr - what limits it to only address etc entities and not others?
There was a problem hiding this comment.
This will only fire for af-join entitites which are like "sub-entities" of a Contact record (aside: in theory it could be possible to have a sub-entity of something else but so far it's just Contacts). And it only fires after the Contact has been saved, which would have thrown an exception if authentication failed. So at this point we already know that saving the Contact is allowed.
There was a problem hiding this comment.
@colemanw ok - but how do we ensure that when we extend the entities we don't accidentally expose more inappropriate sub entities - how are they defined? Presumably pro-actively?
There was a problem hiding this comment.
Also what if you are trying to update a joined case say from an activity but you don't have case permissions how would this work?
There was a problem hiding this comment.
Yes they have to be explicitly defined in code, e.g. afjoinAddressDefault.aff.json. And then they have to be explicitly added to the form by the form author.
There was a problem hiding this comment.
@seamuslee001 af-joins really are sub-entities. Other than email, phone, address, website, im, and multi-record custom fields, I don't expect there to be others to follow this pattern.
Activities and Cases are both full entities in their own right. We wouldn't use the af-join pattern with them. We'd probably use an EntityRef to link them, although we'd have to do something about bridging them through the ActivityContact table. But that's a separate issue to this.
There was a problem hiding this comment.
@colemanw I think that deals with my concerns on a technical level but I'm still nervous on a docs level - ie the concept of sub-entities & implications should be documented. I think the urgency of having a home for afform documentation is increasing (@MikeyMJCO @totten @seamuslee001 )
There was a problem hiding this comment.
I've added https://lab.civicrm.org/documentation/docs/dev/-/merge_requests/913 to start to establish a docs location. It is very minimal but hopefully we could put something about how sub-entities are declared in there
colemanw
force-pushed
the
afformJoinSaveFix
branch
from
37b2ebd
to
6c1e958
Compare
|
Test fail unrelated, now that we have a docs PR I think the docs conversation can continue there, as this fixes a bug I'm going to merge it |
Overview
Fixes saving emails, phones, websites, addresses, etc when the form is submitted by non-admin users.
https://lab.civicrm.org/dev/core/-/issues/2592
Before
Form fails to save email, etc. when submitted by anonymous users.
After
Works
Technical Details
The
getSecureApi4()only really works for the main entity. Once that's validated we can disable permission checks to save related entities.