Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[REF] Refactor location-related BAOs to use writeRecord #25944

Merged
merged 2 commits into from
Mar 30, 2023

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Mar 28, 2023

Overview

Updates location BAOs (phone, im, website, email, open id, and address) to use writeRecord, noisily deprecate and stop using add/create functions.

Technical Details

The most challenging was Address which had a lot of extra processing in the add function as well as a nonstandard extra $fixAddress parameter. Extra processing has been moved to hooks and fixAddress to its own function (actually it was already its own function, so never really needed to be a param!).

The Address::add function also provided legacy support for custom data in the format custom_x => 'foo' mixed into the $params whereas all other BAO create/add functions expect a single key custom. I've removed that extra bit and moved it to the Address::legacyCreate function which seemed fitting.

@civibot
Copy link

civibot bot commented Mar 28, 2023

(Standard links)

@aydun
Copy link
Contributor

aydun commented Mar 28, 2023

Address, IM, Phone, Website all work: I added entries to cg_group_extends, created custom groups/fields via UI, then APIv4 get & update all works as expected. On core-25944-8ixv.test-1.civicrm.org:8001

All looks good, other than the failing tests.

@colemanw
Copy link
Member Author

Thanks @aydun did you happen to test Address custom data in the UI e.g. on "Edit Contact" or editing an address inline on the summary screen?

@aydun
Copy link
Contributor

aydun commented Mar 28, 2023

I have now!

The custom fields work for Address on Contact inline edit but styling isn't great:

image

And on full Edit Contact:
image

Also shows on contact summary:
image

The field shows up when editing an Event Location:
image
but ... it is not saved and produces:

    Notice: Only variable references should be returned by reference in Civi\Core\Event\GenericHookEvent->__get() (line 197 of /home/jenkins/bknix-dfl/build/core-25944-8ixv/web/sites/all/modules/civicrm/Civi/Core/Event/GenericHookEvent.php).
    Deprecated function: strtolower(): Passing null to parameter #1 ($string) of type string is deprecated in HTML_Common->_parseAttributes() (line 162 of /home/jenkins/bknix-dfl/build/core-25944-8ixv/web/sites/all/modules/civicrm/packages/HTML/Common.php).
    Deprecated function: strtoupper(): Passing null to parameter #1 ($string) of type string is deprecated in CRM_Utils_PagerAToZ::getDynamicCharacters() (line 100 of /home/jenkins/bknix-dfl/build/core-25944-8ixv/web/sites/all/modules/civicrm/CRM/Utils/PagerAToZ.php).

@colemanw
Copy link
Member Author

Ok thanks @aydun that looks similar to the error tests are giving. I'll look into it.

@aydun
Copy link
Contributor

aydun commented Mar 29, 2023

Still fails for Event locations

@colemanw colemanw force-pushed the writeRecordRef branch 2 times, most recently from ff05303 to ece327b Compare March 29, 2023 18:07
@colemanw
Copy link
Member Author

retest this please

@colemanw colemanw changed the title [REF] Refactor more BAOs to use writeRecord [REF] Refactor location-related BAOs to use writeRecord Mar 29, 2023
@colemanw
Copy link
Member Author

@aydun I believe this is all working now :)
Giving merge-ready & will merge unless you spot any other issues.

@colemanw colemanw added the merge ready PR will be merged after a few days if there are no objections label Mar 29, 2023
@aydun
Copy link
Contributor

aydun commented Mar 30, 2023

@colemanw All looks good. Event locations show and save custom fields now.

FWIW Event location custom fields don't show on the Event Information page. That's fine: someone might want them to, but then someone might not - that can be a problem for another day! This does what it set out to do which is to use writeRecord and enable custom fields on these entities.

Good to merge.

@colemanw colemanw merged commit 5054b50 into civicrm:master Mar 30, 2023
@colemanw colemanw deleted the writeRecordRef branch March 30, 2023 12:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
master merge ready PR will be merged after a few days if there are no objections
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants