Skip to content
Snippets Groups Projects
Commit ef935da5 authored by totten's avatar totten
Browse files

(#3660) CRM_Extension_ClassLoader - Defend against redundant refreshes

Overview
--------

This is a follow-up to #23824 (c24dd7db) 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 c24dd7db, 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.
parent 253af5eb
No related branches found
No related tags found
No related merge requests found
......@@ -21,6 +21,14 @@ class CRM_Extension_ClassLoader {
*/
const FEATURES = ',psr0,psr4,';
/**
* A list of recently-activated extensions. This list is retained
* even if some ill-advised part of the installer does a `ClassLoader::refresh()`.
*
* @var array
*/
private static $newExtensions = [];
/**
* @var CRM_Extension_Mapper
*/
......@@ -97,6 +105,9 @@ class CRM_Extension_ClassLoader {
}
self::loadExtension($loader, $this->mapper->keyToInfo($key), $this->mapper->keyToBasePath($key));
}
foreach (static::$newExtensions as $record) {
static::loadExtension($loader, $record[0], $record[1]);
}
return $loader;
}
......@@ -130,6 +141,7 @@ class CRM_Extension_ClassLoader {
if (file_exists($file)) {
unlink($file);
}
static::$newExtensions[] = [$info, $path];
if ($this->loader) {
self::loadExtension($this->loader, $info, $path);
}
......
0% Loading or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment