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

Remove campaign_id pseudoconstants #19463

Merged

Conversation

colemanw
Copy link
Member

@colemanw colemanw commented Jan 30, 2021

Overview

Improves performance by not loading (potentially huge) list of campaigns into memory.

Before

Entire list is loaded into memory for dynamic UIs like Search Kit, Afform, and the API Explorer.

After

Those UIs use an EntityRef autocomplete, which actually looks a bit nicer.

Technical Details

Pseudoconstants are intended for option lists small enough to be loaded into memory, whereas FKs are for options of any size.
It is possible to have thousands of campaigns, and some sites do, which slows or breaks some UIs (first noticed in the Search Kit bulk update action).

All these fields have both an FK and a pseudoconstant declared, which is redundant; both are not needed, and newer UIs will automatically switch to using an EntityRef based on the FK.

Comments

In theory this is safe to remove, however there may be some code somewhere that relies on the pseudoconstant existing. Stuff to check:

  • Search Builder (the classic one) still loads campaign fields ok? Update: it is broken by this PR, but fixed by Search Builder - Enhance UI with Select2 and EntityRef #19471
  • Any api3 getoptions calls trying to load campaigns? (I couldn't find any using grep)
  • Any calls to DAO::buildOptions('campaign_id')? (don't see any)
  • Any forms using $form->addField('campaign_id')? (not that I can tell)
  • Any attempts to access it with CRM_Core_Pseudoconstant methods? (haven't found any)
  • Reports still load & display campaigns?

@civibot
Copy link

civibot bot commented Jan 30, 2021

(Standard links)

@eileenmcnaughton
Copy link
Contributor

This DOES resolve the speed issue - the jury is out on the safety issue though - @seamuslee001 @mattwire @demeritcowboy

@colemanw
Copy link
Member Author

Sounds like this is worth pushing forward then. A few notes on the safety issue:

If we classify Civi's history 3 "eras", this is what should be considered for each:

  1. Prehistoric era: back when all forms were written in Quickform. All the original code for campaigns, activities, contributions, etc is from this era. None of this code relied on pseudoconstant metadata because it didn't yet exist in the schema. This PR should have no impact on them.

  2. Early modern: pseudoconstant metadata was added to the schema but not yet used for much. Some old quickforms were updated in spots to use $form->addField() which used the metadata. The first earnest use of the metadata was Search Builder, and this PR does indeed break campaign fields in that UI.

  3. Contemporary: UIs like the API Explorer, Search Kit & Afform all use the metadata but are smart enough to understand FK references & therefore do not need a pseudoconstant list as well.

@seamuslee001
Copy link
Contributor

@colemanw test fails relate

@colemanw
Copy link
Member Author

Tests were covering that the campaign pseudoconstant exists. I've taken them out, as we now don't want it to exist.

@seamuslee001
Copy link
Contributor

Jenkins re test this please

@colemanw
Copy link
Member Author

colemanw commented Jan 30, 2021

I made #19471 to fix the Search Builder breakage caused by this PR and it ended up being a very nice improvement to that UI. @demeritcowboy would you be able to test that PR?

@demeritcowboy
Copy link
Contributor

Sure.

@eileenmcnaughton
Copy link
Contributor

I mentioned this in the dev-digest - what say we merge this after the rc is cut?

@colemanw
Copy link
Member Author

colemanw commented Feb 1, 2021

@eileenmcnaughton sounds good. I've checked most places & feel this is safe, but can you just verify campaign fields in Reports & Extended Reports?

@eileenmcnaughton
Copy link
Contributor

@colemanw ok

Pseudoconstants are intended for option lists small enough to be loaded into memory,
whereas FKs are for options of any size.
It is possible to have thousands of campaigns, and some sites do,
which slows or breaks some UIs (first noticed in the Search Kit bulk update action).

All these fields have both an FK and a pseudoconstant declared, which is redundant;
both are not needed, and the pseudoconstant was unnecessarily hogging memory for sites with many campaigns.
@colemanw colemanw force-pushed the removeCampaignPseudoconstant branch from 0909b8d to dbf668e Compare February 4, 2021 19:46
@eileenmcnaughton
Copy link
Contributor

Per above - merging now - I'll take a look at the reports & fix if need be once merged

@eileenmcnaughton eileenmcnaughton merged commit 3358c46 into civicrm:master Feb 4, 2021
@eileenmcnaughton eileenmcnaughton deleted the removeCampaignPseudoconstant branch February 4, 2021 21:15
@eileenmcnaughton
Copy link
Contributor

In our wmf unit tests I hit this bug

#19633

So far extended reports & reports seem OK (I tested the contribution pivot table in extended reports and the contribution detail report in core)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants