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

CRM-21218 Improve component statics flushing #11022

Merged
merged 1 commit into from
Nov 28, 2017

Conversation

eileenmcnaughton
Copy link
Contributor

@eileenmcnaughton eileenmcnaughton commented Sep 23, 2017

Without this I'm seeing it try to rebuild when it should just be flushing.

Overview

This fixes test failures on the extended report extension which enables components in it's setup. It is hitting problems with the newly enabled components still being excluded in some static caches. These show up on our nightly test matrix

Before

Test failures in ExtendedReports due to Civi::$statics['CRM_Core_Component]['info'] not holding info for all components

After

Tests work. I don't believe changes should affect elsewhere, although I flush the caches slightly more thoroughly is is on an rare action.

Technical Details

The 'onChange' callback for settings are called BEFORE they are saved. Currently CRM_Core_Components::flushEnabledComponents is called & it causes the components to be reloaded. However, the reload should happen AFTER the setting is saved. This changes that function to simply flush the static & not to reload, allowing a better reload later.

There are a few places where that class static is used & I opted for flushing the whole class. It feels safer to clear all cached vars relating to the thing we are changing, and I don't believe this is an action where any performance cost is important.

Comments

@totten can you review this.


Without this I'm seeing it try to rebuild when it should just be flushing.
@totten
Copy link
Member

totten commented Nov 21, 2017

(CiviCRM Review Template WORD-1.0)

  • (r-jira) Pass
  • (r-test) Pass
  • (r-code) Pass: Stylistically, I like this form of "flushing" where you just delete a cache record and then let it repopulate on-demand. It's more, eh, normal. :)
  • (r-doc) Pass: No impact on UI or API
  • (r-maint) Pass
  • (r-run) Pass(*): IMHO, this is the main review question. There are at least three scenarios that come to mind:
    • Running the test suite: 👍
    • Updating the setting via UI: 👍 (eg toggle CiviCampaign/CiviGrant; then navigate into them and create/browse some records)
    • Updating the setting via CLI: 👍 (eg similar use-case as above, but with commands like echo '{"enable_components":["CiviEvent","CiviContribute","CiviMember","CiviMail","CiviReport","CiviPledge","CiviCase","CiviCampaign","CiviGrant"]}' | drush cvapi setting.create --in=json)
    • However, I don't feel this a particularly creative list. There may be some scenario I'm not thinking of. Discussed more in r-tech.
  • (r-user) Pass: No impact on user interface
  • (r-tech) Undecided:
    • Based on grepping universe:
      • There is only one other extension which runs flushEnabledComponents() -- civihr/hrcase/CRM/HRCase/Upgrader.php (@davialexandre).
      • There are a couple of other extensions which write to the setting enable_components -- biz.jmaconsulting.bugp/bugp.php (@JoeMurray) and civihr/hrui/hrui.php (@davialexandre)
    • It'd be ideal one of those authors (or one of their colleagues) weigh in -- they might be able to assess some other angle that escapes me.

@eileenmcnaughton
Copy link
Contributor Author

So to be clear - although it's possible that this function might be called from somewhere else, and ignoring the question of whether we should always support extension writers who do 'the wrong thing' - ie. don't use an established interface and don't add unit tests to support this - the issue is that we have a function that flushes caches and it should do that. The caches will be rebuilt when they are used and this will be correct. Currently they are rebuilt incorrectly because they are being rebuilt at the wrong time.

@totten
Copy link
Member

totten commented Nov 28, 2017

This gets 👍 on the general coding merits, and it didn't seem to raise any eyebrows from the folks who could potentially be impacted, so let's do it.

@totten totten merged commit 8dc6364 into civicrm:master Nov 28, 2017
@eileenmcnaughton eileenmcnaughton deleted the components branch November 28, 2017 02:01
sluc23 pushed a commit to ixiam/civicrm-core that referenced this pull request Jan 10, 2018
CRM-21218 Improve component statics flushing
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.

3 participants