CiviCRM Core issueshttps://lab.civicrm.org/dev/core/-/issues2023-08-30T05:03:25Zhttps://lab.civicrm.org/dev/core/-/issues/2115Move financialACLs to a core extension2023-08-30T05:03:25ZeileenMove financialACLs to a core extensionThis work has already started with the code being moved from LineItem api to a pre hook in the hidden ext/financialacls extension.
The definition of done is:
1) we find all places that refer to isACLFinancialTypeStatus & find a way to m...This work has already started with the code being moved from LineItem api to a pre hook in the hidden ext/financialacls extension.
The definition of done is:
1) we find all places that refer to isACLFinancialTypeStatus & find a way to move them over
2) at that point it should be possible to unhide the extension and allow people to enable it only if they want it
3) we would reconcile it with the extension outside of core & consolidate
@monish.deb @JoeMurray @seamuslee this is really just creating a GL for what we have already discussed. Since it's membership-theme-month I'm gonna take a look at the bits of this as relate to membershiphttps://lab.civicrm.org/dev/core/-/issues/2644Proposal - new core extension for scheduled reminders using criteria from sea...2023-08-28T12:20:35ZeileenProposal - new core extension for scheduled reminders using criteria from search-kitThis replaces https://lab.civicrm.org/dev/core/-/issues/2267 as a way to address the same issue - ie our search criteria for scheduled reminders is way too brittle due to the inflexible schema. For example it is not possible to use contr...This replaces https://lab.civicrm.org/dev/core/-/issues/2267 as a way to address the same issue - ie our search criteria for scheduled reminders is way too brittle due to the inflexible schema. For example it is not possible to use contributions > x amount or contributions from campaign y.
Given that we Apiv4 / search kit is the way we are going with search criteria it makes sense to use these as criteria for the reminder search too to replace (in the distant future) the existing fields.
#2267 proposed adding api params & api entity to the scheduled reminder table but I now think it makes more sense as an extension - which I have been experimenting with.
This extension does not have to go into core - it could be something I just maintain to the degree I see fit.
However, the main reason we would consider it as a core extension is that it could eventually replace the existing scheduled reminder form (which would fit our LEXIM plan to get off quick forms and onto everything being apiv4 based). The hope is not to replace the additional functionality in CiviRules for people who want that but to reduce pain points in the scheduled reminder code that is already in core and as a path away from maintaining the relevant quickforms.
Note that if we proceed with this as a core extension I will PR it in once it hits 'alpha' but we would not enable by default and we can have a discussion at that point as to whether it should be hidden or not depending on how mature it is.
I note @totten's response to the question as to whether it should go into core rather seemed to hinge on whether the UI is any good - and since it's not yet written we can't address that at this stage.https://lab.civicrm.org/dev/core/-/issues/1164Editing event participant can affect wrong record if multiple participants ar...2023-08-24T10:07:23ZJKingsnorthEditing event participant can affect wrong record if multiple participants are opened in multiple tabs/windowsThis is an old issue (sorry I couldn't yet find the original issue in JIRA that was raised for this.)
It was recently attempted to get fixed here: https://lab.civicrm.org/dev/core/issues/1164 / https://github.com/civicrm/civicrm-core/pu...This is an old issue (sorry I couldn't yet find the original issue in JIRA that was raised for this.)
It was recently attempted to get fixed here: https://lab.civicrm.org/dev/core/issues/1164 / https://github.com/civicrm/civicrm-core/pull/14244 but a regression caused the patch for event participants to be reverted.
The problem, example of dmaster:
* Events > Find participants > Search
* 'Edit' one participant (Lincoln) in a new tab
* 'Edit' a second participant (Teresa) in a new tab
* Go to Lincoln's edit tab, change to no-show, save
* The change has been applied to the WRONG record (Event registration information for Teresa Terry has been updated.)
This is because the participant ID is being stored in the session.https://lab.civicrm.org/dev/core/-/issues/2635Download latest version of an extension without installing2023-08-19T05:03:24Zmagnolia61Download latest version of an extension without installingOverview
----------------------------------------
It would be good to be able to download the latest version of an extension without installing it for security reasons.
Current behaviour
----------------------------------------
1. an ex...Overview
----------------------------------------
It would be good to be able to download the latest version of an extension without installing it for security reasons.
Current behaviour
----------------------------------------
1. an extension that was previously installed but since disabled or uninstalled remains present in the extension directory
2. the extensions management ui provides buttons to download and install
![image](/uploads/c618c97b6ec39f98d9b884b0211187fb/image.png)
Proposed behaviour
----------------------------------------
1. an extension that was previously installed but since disabled or uninstalled remains present in the extension directory
2. the extensions management ui provides buttons to "download" or to "download and install"
Comments
----------------------------------------
Of course best practice is to remove any extension which is uninstalled, but disabled extensions are also present in the filesystem. Being able to update extensions that are not installed, improves security as the latest stable code can be downloaded.https://lab.civicrm.org/dev/core/-/issues/1536Develop unit tests that catch red warning boxes on common forms2023-08-17T05:03:27ZDaveDDevelop unit tests that catch red warning boxes on common formsMaking myself a rainy day todo.
* Forms are not well covered by unit tests. In general that's not what pure unit tests are supposed to do, but I'm thinking there could be something minimal.
* Since the addition of popup forms, warnings ...Making myself a rainy day todo.
* Forms are not well covered by unit tests. In general that's not what pure unit tests are supposed to do, but I'm thinking there could be something minimal.
* Since the addition of popup forms, warnings and notices don't get seen when testing because they get snippet'd out.
* It's happening often enough where changes introduce some type of "missing XXX" from forms.
So it might be as simple as a test that runs through some common forms and sets up some vars and calls preprocess and buildform and when run as tests that should show notices. It won't catch everything, but might reduce the problem.https://lab.civicrm.org/dev/core/-/issues/2639Money: Plan to migrate from currency separators from locale2023-08-16T05:03:18ZeileenMoney: Plan to migrate from currency separators from localeThere is a goal to switch to sites specifying their money_locale from specifying currency separators
The original discussion on how to do this was in the dev-digest and a PR. @mattwire has since suggested an alternate https://github.com...There is a goal to switch to sites specifying their money_locale from specifying currency separators
The original discussion on how to do this was in the dev-digest and a PR. @mattwire has since suggested an alternate https://github.com/civicrm/civicrm-core/pull/20296#issuecomment-855377189
"Override currency separators" - "By default CiviCRM will format currency in accordance with the locale for the user that is viewing the information"
I've moved the text below from a comment on the github PR to here ....
In terms of adding a setting I went hunting for where it was discussed before & it is here
https://github.com/civicrm/civicrm-core/pull/19753
![image](https://user-images.githubusercontent.com/336308/120940640-63d47800-c772-11eb-8b46-302f4d311ef3.png)
Further down the same PR is the content that was pasted into the dev-digest
![image](https://user-images.githubusercontent.com/336308/120940681-92eae980-c772-11eb-9ff9-588d1e3ca663.png)
Compared to @mattwire's suggestion this is
1) implicitly opt in at the start - ie let early adopters set it & when we are confident be more aggressive
2) a different setting name / description - I think setting money_locale might be less intuitive short term (since it magically disables the other settings) and more intuitive longer term since the other settings should become meaningless & disappear over time.
Revisiting @jaapjansma's comment
![image](https://user-images.githubusercontent.com/336308/120940808-33410e00-c773-11eb-9bae-9910a3ff7583.png)
It DOES seem like site locale is going to be wanted as a setting - although it could be that this is not the point where we add ithttps://lab.civicrm.org/dev/core/-/issues/521PCP Owner notification email sending before payment2023-08-14T05:03:17Zrita_compucorpPCP Owner notification email sending before paymentHi there,
When you are the owner of a fundraising page, you are supposed to receive a notification email after a successful donation has been made to your fundraising page. However when you are using a payment processor that navigates y...Hi there,
When you are the owner of a fundraising page, you are supposed to receive a notification email after a successful donation has been made to your fundraising page. However when you are using a payment processor that navigates you out from civicrm to its own payment page (like sagepay, or paypal standard) the owner notification is sent before the payment has been made.
Steps:
- create a contribution page and enable fundraising pages to be created under it
- make the payment processor to be Sagepay or Paypal standard
- create a fundraising page
- then log out and start donating to the fundraising page
- when you click confirm button on the fundraising page, you will get navigated to the payment processor you set up for the contribution page --> this is the point when the notification email is sent to the fundraiser
Expected result: notification for the owner is sent only after a successful donation to the fundraising pagehttps://lab.civicrm.org/dev/core/-/issues/3361View and Edit links for event participants are inconsistent and in some cases...2023-08-12T00:24:39ZlarsssandergreenView and Edit links for event participants are inconsistent and in some cases do not allow editingWhen using a price set, the View and Edit links for a participants lead to different forms depending on if the registration has an associated contribution. When there is a contribution record, the Edit link does not allow the user to edi...When using a price set, the View and Edit links for a participants lead to different forms depending on if the registration has an associated contribution. When there is a contribution record, the Edit link does not allow the user to edit the price set selections, while it is possible to edit those selection from the View link (with Change Selections). I believe this is a regression.
When there is no contribution record, you cannot edit the price set selections from the View link, but you can from the Edit link. That seems confusing for users and it would be best to make it consistent.
Here are some screenshots:
1) View with a contribution, this makes sense
![image](/uploads/52e7c3a5b96befe0a404f57850cc5bcf/image.png)
2) View without a contribution, seems like it there should be possible to change selections here as well for consistency (or else a user may think they can't edit the price set selections)
![image](/uploads/c93e10bbecf9ae053ea771becb07ac12/image.png)
3) Edit with a contribution, this should be the same as the View with a contribution, i.e. there should be a Change Selections link and the table of selections above it
![image](/uploads/95edcf86ca33d6bcdff302cee619acf8/image.png)
4) Edit without a contribution, this makes sense
![image](/uploads/12e211610125abb94b4f829bca465ebe/image.png)
I believe 3 - Edit with a contribution is a regression. I get his behaviour on dmaster, 5.37 and 5.35.2, but not on 5.24.5, where there is a Change Selections link and the table above.
Changing 2 - View without a contribution may be more complicated. What about adding an Edit Registration link, in the same place where the Change Selections link would otherwise be, that simply takes you to the edit form? As far as I can tell, trying to use Change Selections with a registration that doesn't have a contribution will result in errors. I'm not sure what would be involved in making it possible to use Change Selections in this case, but it seems like adding an edit link would be a simpler solution to keep some consistency between these two cases.https://lab.civicrm.org/dev/core/-/issues/2609Move full text search to an extension?2023-08-11T05:03:16ZeileenMove full text search to an extension?We just hit an issue where someone crashed our db with the full text search. It feels like something that should be deliberately enabled rather than available by default....We just hit an issue where someone crashed our db with the full text search. It feels like something that should be deliberately enabled rather than available by default....https://lab.civicrm.org/dev/core/-/issues/2557Add hook support for Report tabs.2023-08-06T05:03:24ZyashodhaAdd hook support for Report tabs.Add hook support for Report tabs.Add hook support for Report tabs.yashodhayashodhahttps://lab.civicrm.org/dev/core/-/issues/2584Proposal - Separation of Default Language for contacts from the default Language2023-08-05T05:03:23ZVangelisPProposal - Separation of Default Language for contacts from the default Language## Scenario
Right now, the preferred communication language for new contacts is bound to what the site's default language is in or to 'use language in use at the time' is in but as it seems, neither of them fits the scenario below:
A m...## Scenario
Right now, the preferred communication language for new contacts is bound to what the site's default language is in or to 'use language in use at the time' is in but as it seems, neither of them fits the scenario below:
A multinational organization in country A has 2 (or more) enabled languages in their Drupal configuration, thus into each user's profile. They want to have the preferred communication language for each of their contacts set to country A's language while the menus needs to be retained in English by force.
## How to replicate
1. On a Drupal 7 site, allow 2 languages, set the English as default
1. In CiviCRM > Administer > Localisation, set:
* the default language to the non-English
* Inherit CMS language to 'Yes'
* Available Languages: Add the 2 languages that you've added in Drupal
* Default Language for contacts: Any (I tried all options)
* Logout and login again
1. Try to create a new contact
## Possible solution
The suggestion is besides what already is in the selector, to add also a list of available languages to the field 'Default Language for contacts' so that we can enforce a language.
I've already worked on a patch that does the above, I'm not sure if my approach of this idea hides any potentials flaws.
Selector before:
![image](/uploads/f19b2c389213236f81e0df86a5335d33/image.png)
Selector after:
![image](/uploads/75e42b33fe04528f1e2b0d298b03ac30/image.png)
Example: Danish
![Screenshot_from_2021-05-03_09-58-12](/uploads/b12950fd7f96ee0604c839ee1bcbb61e/Screenshot_from_2021-05-03_09-58-12.png)
![Screenshot_from_2021-05-03_09-56-50](/uploads/96302b55f8779875e89b5b565cdd07d0/Screenshot_from_2021-05-03_09-56-50.png)
---
PR is [here - 20214](https://github.com/civicrm/civicrm-core/pull/20214)https://lab.civicrm.org/dev/core/-/issues/2570Option to identify 'deceased' contacts when viewing relationships2023-07-31T05:03:26ZrebeccatregennaOption to identify 'deceased' contacts when viewing relationshipsOverview
----------------------------------------
Marking a contact record as 'is_deceased' doesn't currently appear to have any impact on the contact's relationships, however, it would be extremely useful when viewing relationships on c...Overview
----------------------------------------
Marking a contact record as 'is_deceased' doesn't currently appear to have any impact on the contact's relationships, however, it would be extremely useful when viewing relationships on contact records to know whether one of the contacts involved in the relationship is deceased.
Current behaviour
----------------------------------------
- John is the 'partner of' Fiona
- John is deceased but the relationship (as not manually changed) remained active
- A system user is taking a call from Fiona and looks up her contact record and casually asks how John is, as nothing indicates his passing on the relationship tab thus causing upset and potentially losing a donor
Proposed behaviour
----------------------------------------
Add an icon to the relationship to indicate the contact is deceased.https://lab.civicrm.org/dev/core/-/issues/2551Proposal - add support for defining 'fields' within json blobs2023-07-29T05:03:23ZeileenProposal - add support for defining 'fields' within json blobsThis covers a discussion at https://chat.civicrm.org/civicrm/pl/pt4nizctz7db8xecrq1triy38e
whereby we have a few entities with configuration options that can't be adequately captured in fields. We sort of make this work with payment_pro...This covers a discussion at https://chat.civicrm.org/civicrm/pl/pt4nizctz7db8xecrq1triy38e
whereby we have a few entities with configuration options that can't be adequately captured in fields. We sort of make this work with payment_processor by having fields that we kinda rename via the payment_processor_type table and as long as there are only 4 fields it's fine. However, it's not a great structure.
I'm working on a monolog extension and like payment processor and geocoders the configuration options desired by the service varies - but it's not 100% open ended - ie we don't want to just pass through 'whatever'. The best data model to store this is a json blob- however, we then have no UI knowledge of what would go in there and afform is unable to present this field in a UI-friendly way
We looked at the idea of having something like
```
<field>
<name>contact_type</name>
</field>
<field>
<name>contact_sub_type</name>
</field>
<field>
<name>options</name>
<type>text</type>
<serialize>json</serialize>
<schema-callback>CRM_My_Schema::getFields</schema-callback>
<schema-watch>contact_type,contact_sub_type</schema-watch>
</field>
```
Where the UI layer would need to implement the ability to watch the fields defined as schema-watch and update the UI based on it (presumably only for fields that are ALSO already displayed)
A simple xml definition of subfields might look like https://gist.github.com/totten/84c140310d4adc9b5d3842b5d3364a9e - which could be rendered by a generic function.
@totten @seamuslee - anything else you think is important to transfer over here?https://lab.civicrm.org/dev/core/-/issues/2545Free memberschip sign up (eg. prayerteam) assumes contribution involved2023-07-29T05:03:22Zmagnolia61Free memberschip sign up (eg. prayerteam) assumes contribution involvedOverview
----------------------------------------
Managing memberships in civicrm currently pretty much assumes a contribution is involved. Which is not the case for many forms of memberships. It is even not possible to create a membersh...Overview
----------------------------------------
Managing memberships in civicrm currently pretty much assumes a contribution is involved. Which is not the case for many forms of memberships. It is even not possible to create a membership sign up page without using a contribution page!
Right now for our summercamps it's pretty strange for people to sign up to become a member of the prayerteam to "review their contribution". Although technically they give their contribution in the form of prayer one could argue.
Example use-case
----------------------------------------
1. Create a membership sign up page in CiviCRM for a free membership
2. Look at the wording of the sign-up button
Current behaviour
----------------------------------------
The current button to sign up for a membership says 'review your contribution'.
Proposed behaviour
----------------------------------------
The button text here should be editable, or for signing up for free memberships a different default wording should be used.
Comments
----------------------------------------
Consistent UI would be I guess to be able to create a membership sign up page from the membership menu. Underwater this could be a contribution page with or without fees involved.https://lab.civicrm.org/dev/core/-/issues/2544Move Contact Layout Editor to a core extension2023-07-27T05:03:20ZcolemanwMove Contact Layout Editor to a core extensionAt some point, it would be good to get rid of the hard-coded contact summary screen, and just let the Contact Layout Editor manage the display.
As an intermediary step, it probably makes sense to move Contact Layout Editor into the core...At some point, it would be good to get rid of the hard-coded contact summary screen, and just let the Contact Layout Editor manage the display.
As an intermediary step, it probably makes sense to move Contact Layout Editor into the core tarball (via distmaker import), or the `ext/` directory of the core repo.
Today might not be the day for that, but logging some notes on it anyway:
- Today, the only benefit of moving the extension into core is it would be more visible to end-users.
However, in the future, benefits might include:
- Easier simultaneous development of extension & core with no version conflicts.
- Possible to enable the extension by default.
- Possible to rip out redundant functionality from core.
Challenges
- There isn't an easy upgrade path when an extension is moved into core; you end up with 2 copies of the extension
- Currently, the extension scanner does not give the core `ext/` directory top priority, which means older versions of the extension outside core would take precedence.
Considerations
- As handy as it is, Contact Layout Editor still uses smarty to render the page.
- Maybe the end-game is to make that page an Afform instead. However, that day is a long way off given Afform's current capabilities.https://lab.civicrm.org/dev/core/-/issues/2476Proposal move weird acl stuff to extension2023-07-21T05:03:18ZeileenProposal move weird acl stuff to extensionI've been cleaning up the civicrm_acl table and the code supports 3 types of acls but the UI doesn't and hooks don't leverage them.
My take is that they started out with 2 - contact based & non-smart-group-based and then added a new rep...I've been cleaning up the civicrm_acl table and the code supports 3 types of acls but the UI doesn't and hooks don't leverage them.
My take is that they started out with 2 - contact based & non-smart-group-based and then added a new replacement - role based - without ever removing the first 2
The different types are denoted by the value entity_table in civicrm_acl. I believe this is only ever equal to civicrm_acl_role
I think we could simplify the code by moving the support for the other 2 to a core extension which ideally we only enable on upgrade IF we get results from
```SELECT * FROM civicrm_acl WHERE entity_table != 'civicrm_acl_role'```
@seamuslee @DaveD I'd be interested to see if you come to the same conclusionhttps://lab.civicrm.org/dev/core/-/issues/2402Proposed Updated Phone Types2023-07-20T05:03:21ZcolemanwProposed Updated Phone TypesMotivation/Background
---------
The phone_type option list is somewhat dated and presents options that don't always make sense to modern users. The first two options are "Phone" and "Mobile". Twenty years ago there might have been a mor...Motivation/Background
---------
The phone_type option list is somewhat dated and presents options that don't always make sense to modern users. The first two options are "Phone" and "Mobile". Twenty years ago there might have been a more obvious distinction between the two, but today they are basically synonyms. We should re-label them so that it's clear that the first option means "Landline phone" and the second option means "Phone that can receive SMS". And while we're at it, let's also change their weights since mobile phones are now more common than landlines and should be presented as the first option.
Current Options
--------------
| weight | name | label (EN) |
| ------ | ---------- | ---------- |
| 1 | Phone | Phone |
| 2 | Mobile | Mobile |
| 3 | Fax | Fax |
| 4 | Pager | Pager |
| 5 | Voicemail | Voicemail |
Proposed Options (New Installs)
----------------
_This reverses the order and relabels the first two options, while deleting the last two, as "Pager" and "Voicemail" numbers are rarely used these days (it's an option list so specialized organizations can always add them in)._
| weight | name | label (EN) |
| ------ | ---------- | ----------- |
| 1 | Mobile | Voice & SMS |
| 2 | Phone | Voice Only |
| 3 | Fax | Fax |
Proposed Upgrade
-------------------
DB upgrade would update labels and weights of the first two options. Functionality will not be impacted as the machine names remain the same. The "Pager" and "Voicemail" options will be deleted _only if they are not in use._https://lab.civicrm.org/dev/core/-/issues/2508Improve unit tests by checking if session::setStatus sets errors2023-07-20T05:03:20ZDaveDImprove unit tests by checking if session::setStatus sets errorsThe alert popups that come up in civi happen two ways: One is via javascript-only, which is difficult to test in the regular phpunit tests, and the other is via CRM_Core_Session::setStatus().
One approach is as in https://github.com/civ...The alert popups that come up in civi happen two ways: One is via javascript-only, which is difficult to test in the regular phpunit tests, and the other is via CRM_Core_Session::setStatus().
One approach is as in https://github.com/civicrm/civicrm-core/pull/19939 but it's not workable in practice because it generates too much extra visual noise on screen. However it pointed out some tests which could be improved. See also https://github.com/civicrm/civicrm-core/pull/19939#issuecomment-809683859 for some additional details. The last one here about financialtype I think I saw a variation of on-screen once and was just that the type of popup was wrong (i.e. using error instead of success).
```
not ok 599 - Error: CRM_Contact_Form_Task_EmailCommonTest::testPostProcessWithSignature
not ok 603 - Error: CRM_Contact_Form_Task_UseraddTest::testUserCreateFail
not ok 653 - Error: CRM_Contact_Page_View_UserDashBoardTest::testDashboardContentEmptyContact
not ok 654 - Error: CRM_Contact_Page_View_UserDashBoardTest::testDashboardContentContributionsWithInvoicingEnabled
not ok 655 - Error: CRM_Contact_Page_View_UserDashBoardTest::testDashboardContentContributions
not ok 698 - Failure: CRM_Contribute_BAO_ContributionRecurTest::testAutoRenewalWhenOneMemberIsDeceased
not ok 944 - Error: CRM_Core_BAO_AddressTest::testSharedAddressChaining3
not ok 1115 - Error: CRM_Core_BAO_SettingTest::testGetItemGroup_Override
not ok 1116 - Error: CRM_Core_BAO_SettingTest::testDefaults
not ok 1117 - Error: CRM_Core_BAO_SettingTest::testOnChange
not ok 1118 - Error: CRM_Core_BAO_SettingTest::testSetCivicrmEnvironment
not ok 1119 - Error: CRM_Core_BAO_SettingTest::testPseudoConstants
not ok 507 - Error: api_v3_ContactTest::testMergedGetWithPermanentlyDeletedContact
not ok 1382 - Failure: api_v3_JobTest::testProcessMembershipDeceased
not ok 1418 - Failure: api_v3_LoggingTest::testEnableDisableLogging
not ok 1522 - Failure: api_v3_MembershipStatusTest::testDeceasedMembershipInline
not ok 280 - Error: Civi\Test\ExampleHookTest::testPageOutput
fatal error - ext/oauth-client/tests/phpunit/CRM/OAuth/MailSetupTest.php:18
not ok 2 - Failure: Civi\Financialacls\FinancialTypeTest::testChangeFinancialTypeName
```https://lab.civicrm.org/dev/core/-/issues/2500Run localised unit tests2023-07-18T05:03:23ZBjörn EndresRun localised unit testsOverview
----------------------------------------
Tickets like #2164 highlight again that we have a test gap for localised environments.
Example use-case
----------------------------------------
See #2164
Current behaviour
------------...Overview
----------------------------------------
Tickets like #2164 highlight again that we have a test gap for localised environments.
Example use-case
----------------------------------------
See #2164
Current behaviour
----------------------------------------
Currenlty, afaik, the unit tests are only run in the the native language (en_US).
Proposed behaviour
----------------------------------------
If there is an automated/easy way to do so, we should run the unit test suite in selected (or all?) localisations. Maybe not all the time (as in the PR hooks), but e.g. before a release.
Comments
----------------------------------------
I'm sure somebody has already looked into that, and I'd like to know what the main obstacles were.https://lab.civicrm.org/dev/core/-/issues/2495Proposal - populate all requested smart groups at once2023-07-17T05:03:24ZeileenProposal - populate all requested smart groups at onceThis is tied to https://lab.civicrm.org/dev/core/-/issues/2480 but I wanted to split it off as it is related specifically to the existing load() mechanism and where I think we could tweak existing code slightly to make a function that wo...This is tied to https://lab.civicrm.org/dev/core/-/issues/2480 but I wanted to split it off as it is related specifically to the existing load() mechanism and where I think we could tweak existing code slightly to make a function that would be more efficient to support APIv4
What currently happens
load() is called for each **requested** group in turn.
If you want contacts in 8 different groups is is called 8 times.
(take a moment to realise I'm not talking about populating all groups in this gl - only those groups specifically required for the action)
it follows these steps
1) checks if it has already been loaded within this php process
2) checks if the cache date on the civicrm_group is NULL or longer ago than the smart group cache timeout
3) creates a temp table
4) populates the temp table with the contents of the saved search
5) acquires a mysql lock for the group - if not aquired it returns at this point
6) removes records for those groups from the group_contact_cache table
7) inserts rows from the temp table into the group_contact_cache table
8) drops the temp table
9) updates the group contact cache table.
10) releases the lock
What I think we could do is have a new function
```
public function loadGroups($groupIds) {
}
```
which would start with ids as a list of groups to resolve but
1) filter out any ids already loaded in this php process
2) filter out any groups with expired caches
3) attempt to acquire locks on each group, filter out any groups for which a lock cannot be aquired.
3) create a temp table
4) populate the temp table with the contents of each of the filtered groups (one by one still - we could test UNION later on if we want - out of scope)
5) remove records from the group contact table for all the groups
6) insert rows from the temp table for all the groups
7) drop the temp table
8) update the group contact cache table for all groups
9) release all the locks
While this is potentially less php cycles the thing I'm really trying to get to is less inserts into the civicrm_group_contact cache table as these can clash with each other or with delete actions. There is a risk that the number of rows inserted at once would be too many (although this already exists & it's unclear whether this WOULD actually make it worse) - we might iterate through the temporary table only inserting say 50k rows per query if this turns out to be an issue in testing.
I think @mattwire developed the process whereby we build a temp table first and then insert and I believe it has been a big improvement. I think this would get us further. I would want to focus on testing / implementing it in apiv4 rather than everywhere but api v4 could then figure out what groups it wants and request that they be loaded before joining onto the group_contact_cache table (in practice
LEFT JOIN civicrm_group_contact_cache UNION civicrm_group_contact if not all groups are smart groups).
I've also been pondering the (out of scope!!!) idea of really splitting out the temporary table building from the insert into smart group contact cache - perhaps the table could be durable & the query could use the temporary table itself and we could store the temporary table name & timestamp and somehow fire off a non-browser process to reap it into the group_contact_cache and drop the table.
@pfigel @colemanw @seamuslee @mattwire @totten @bgm @sluc23 @BjoernE