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

Civi::rebuild() - Consolidate super-functions rebuildMenuAndCaches, flushCache, and cleanupCaches #32065

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

totten
Copy link
Member

@totten totten commented Feb 11, 2025

Overview

When doing a general flush/rebuild/reset, one invokes a few gnarly functions. This combines them. The result is still a bit gnarly, but it improves the situation in that:

  1. The steps have more distinctive names.
  2. You can opt-in/opt-out for specific steps.
  3. You can see how all the steps fit together. It's easier to read/trace.

Before

Civi has many helpers for batch-clearing/batch-rebuilding of caches

  • CRM_Core_Invoke::rebuildMenuAndCaches(bool $triggerRebuild = FALSE, bool $sessionReset = FALSE)
  • CRM_Utils_System::flushCache()
  • CRM_Core_Config::cleanupCaches($sessionReset = FALSE)

All of these are super-functions touching multiple (qualitatively different) structures. The names sound alike. They also call each other. But the lines between them aren't particularly clear. The names put them in a support-gray-zone (sort-of-private; sort-of-public). The functionality is fairly important, so they wind up being used by contrib.

After

All the functions have been merged into one Civi::rebuild($targets), e.g.

// Rebuild everything
Civi::rebuild('*')->execute();

// Rebuild a couple specific things
Civi::rebuild(['menus' => TRUE, 'triggers' => TRUE])->execute();

// Rebuild everything, except for a couple really expensive parts
Civi::rebuild(['*' => TRUE, 'triggers' => FALSE, 'entities' => FALSE])->execute();

The original functions are preserved with stubs to call Civi::rebuild().

(EDIT: Original draft was non-fluent. Added execute() to make style more fluent.)

Comments

  • This doesn't address the API access for this functionality. (API exposure is good for providing HTTP/REST/CLI access. ) Presumably, System.flush API should be expanded to pass-through more of these keys. (However, in terms of making a robust mechanism that can recover the system from weird states... the API has the challenge of being tied-up with all the same data-structures that are being rebuilt. Keeping the guts of it as plain PHP feels safer.)
  • Philosophically, I might prefer that the system leaned more into "flush" or "clear" (delete; leave empty; let someone else hydrate) rather than "rebuild" (proactive; warmup; hydrate). But that's a longer road, and we've held-off on this cleanup forever because it looks longish/scaryish. Maybe it's fine aim for "better" and leave "perfect" for another day...

Copy link

civibot bot commented Feb 11, 2025

🤖 Thank you for contributing to CiviCRM! ❤️ We will need to test and review this PR. 👷

Introduction for new contributors...
  • If this is your first PR, an admin will greenlight automated testing with the command ok to test or add to whitelist.
  • A series of tests will automatically run. You can see the results at the bottom of this page (if there are any problems, it will include a link to see what went wrong).
  • A demo site will be built where anyone can try out a version of CiviCRM that includes your changes.
  • If this process needs to be repeated, an admin will issue the command test this please to rerun tests and build a new demo site.
  • Before this PR can be merged, it needs to be reviewed. Please keep in mind that reviewers are volunteers, and their response time can vary from a few hours to a few weeks depending on their availability and their knowledge of this particular part of CiviCRM.
  • A great way to speed up this process is to "trade reviews" with someone - find an open PR that you feel able to review, and leave a comment like "I'm reviewing this now, could you please review mine?" (include a link to yours). You don't have to wait for a response to get started (and you don't have to stop at one!) the more you review, the faster this process goes for everyone 😄
  • To ensure that you are credited properly in the final release notes, please add yourself to contributor-key.yml
  • For more information about contributing, see CONTRIBUTING.md.
Quick links for reviewers...

➡️ Online demo of this PR 🔗

@civibot civibot bot added the master label Feb 11, 2025
@ufundo
Copy link
Contributor

ufundo commented Feb 12, 2025

This looks great @totten . Will try r-running it on some dev sites now :)

@totten
Copy link
Member Author

totten commented Feb 12, 2025

Thanks @ufundo.

I've pushed a change in the signature (but not in the main behavior).

While re-reading today, it struck me that a lot of Civi::foo() helpers work in a fluent style. Applying the style here would look like:

Civi::rebuild('*')->execute();

That style is more forward-looking (i.e. one can chain-in new methods without breaking things). I don't want to speculate too much about what those methods should be. (I think the string|array $targets has decent mileage and feels relatively KISS.) But just to be more future-friendly and more consistent with other Civi::foo() helpers, this should probably start out in fluent style.

The last commit simply moves the bulk of the code from Civi.php (function rebuild()) to Civi/Core/Rebuilder.php (function execute()).

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.

2 participants