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

(dev/core#3660) CRM_Extension_ClassLoader - Defend against redundant refreshes #23900

Merged
merged 1 commit into from
Jun 29, 2022

Conversation

totten
Copy link
Member

@totten totten commented Jun 29, 2022

Overview

This is a follow-up to #23824 (c24dd7d) which addresses a regressive edge-case.

Steps to Reproduce

  • Write an extension like wmf-civicrm which (a) calls System.flush (rebuildMenuAndCaches())
    during hook_install -- and then (b) loads some class from the same extension.
    function foo_civicrm_install() {
      civicrm_api3('System', 'flush', []);
      CRM_Foo_Helper::doStuff();
    }
  • Try to install the extension.

Before

Crashes on loading the class CRM_Foo_Helper

After

Loads the class CRM_Foo_Helper.

Comments

(1) To see what's happening, consider CRM_Extension_Manager_Module::onPreInstall(). This registers the new classloader and then fires hook_install which eventually fires rebuildMenuAndCaches(). With c24dd7d, this resets the classloader again. But the extension isn't fully installed yet - so it forgets about the new extension.

(2) Is it safe to have some (temporarily) sticky items? Ish. You might say: "Ah, but what if we need to remove an extension? Won't this static variable retain stale things?" Doesn't matter. In PHP, classloading is a one-way-street. (You cannot unload.) So you'll still have old classes in memory, regardless of whether the ClassLoader has some old metadata about where to find classes.

(3) I'm on the fence about whether this patch is a good idea. Calling System.flush explicitly in this context seems like an invitation to trouble. OTOH, it worked before #23824, so it can be called a regression.

…refreshes

Overview
--------

This is a follow-up to civicrm#23824 (c24dd7d) which addresses a
regressive edge-case.

Steps to Reproduce
------------------

* Write an extension like `wmf-civicrm` which (a) calls `System.flush` (`rebuildMenuAndCaches()`)
  during `hook_install` -- and then (b) loads some class from the same extension.
    ```php
    function foo_civicrm_install() {
      civicrm_api3('System', 'flush', []);
      CRM_Foo_Helper::doStuff();
    }
    ```
* Try to install the extension.

Before
------

Crashes on loading the class `CRM_Foo_Helper`

After
-----

Loads the class `CRM_Foo_Helper`.

Comments
--------

(1) To see what's happening, consider `CRM_Extension_Manager_Module::onPreInstall()`.
This registers the new classloader and then fires `hook_install` which eventually
fires `rebuildMenuAndCaches()`. With c24dd7d, this resets the classloader again.
But the extension isn't fully installed yet - so it forgets about the new extension.

(2) Is it safe to have some (temporarily) sticky items?  Ish.  You might
say: "Ah, but what if we need to remove an extension?  Won't this static
variable retain stale things?" Doesn't matter.  In PHP, classloading is a
one-way-street.  (You cannot unload.) So you'll still have old classes in
memory, regardless of whether the `ClassLoader` has some old metadata
about where to find classes.

(3) I'm on the fence about whether this patch is a good idea.  Calling
`System.flush` explicitly in this context seems like an invitation to
trouble.  OTOH, it worked before civicrm#23824, so it can be called a regression.
@civibot
Copy link

civibot bot commented Jun 29, 2022

(Standard links)

@civibot civibot bot added the master label Jun 29, 2022
@eileenmcnaughton
Copy link
Contributor

I've tested this on our build & it fixes the issue for us

@totten totten changed the base branch from master to 5.51 June 29, 2022 02:17
@civibot civibot bot added 5.51 and removed master labels Jun 29, 2022
@totten totten marked this pull request as ready for review June 29, 2022 02:18
@eileenmcnaughton
Copy link
Contributor

This resolved our build-issue - merging

@eileenmcnaughton eileenmcnaughton merged commit a4e1ab2 into civicrm:5.51 Jun 29, 2022
@totten totten deleted the 5.51-sticky-classloader branch June 29, 2022 05:45
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