SpecScanner - Fix installation error ("Class XyzSpecProvider does not exist") #24874
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…during installation During the process of installing an extension, there are various flushes/rebuilds. Each of these flushes can lead to a (temporary) rebuild of the container. In 5.54.0, a caching bug prevents these flushes from rebuilding the container promptly. In 5.54.1 and 5.55.beta, the caching bug goes away -- and now it tries to rebuild the container (multiple times). However, during the an early attempt, it fails: there is a mismatch between `getActiveModuleFiles()` (which is used by `CRM_Api4_Services`/`LegacySpecScanner` to discover classes like `ImportSpecProvider`) and the active class-loader (which is not available yet -- but it's supposed to load the source for `ImportSpecProvider`). This means that `CRM_Api4_Services`/`LegacySpecScanner` may raise a class-not-found exception. This patch basically makes `LegacySpecScanner` behave more like `ClassScanner` during the early parts of bootstrap -- if a class is not loadable yet, then ignore it. This should still work out in the end -- because there are multiple rebuilds, and the final disposition is based on the final build.
The `SearchKitSubscriber` is now registered as an auto-service. This behaves better when dealing with various enable/disable flows.
|
(Standard links)
|
|
This seems ok to me |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Overview
(This is inspired by @eileenmcnaughton's #24807. It does not preclude #24807, but it should fix more symptoms.)
This addresses a regression affecting
civiimportandsearch_kit(and probablycivigeometry, but I haven't tested that one). The problem arises as follows:SpecProvider(such asciviimportorsearch_kit).civicrm/admin/extensions), install the extension.Before (5.54.0)
It appears to work.
Internally, the installation flushes various caches in various ways. It does not flush/rebuild the container (in local/PHP memory). This can be confusing when doing automated tests, but (for production) it ultimately works out anyway. (It deletes the container-cache-file, so the future page-views will rebuild, and the final content will be correct.)
Before (5.54.1, 5.55.beta)
It appears to fail.
Internally, the installation flushes various caches in various ways. This includes flushing the container (in local/PHP memory as well as the file-based cache). This is better for automated tests, but (for production) one of the interim flushes occurs at a poor moment -- when
LegacySpecScannersees an inconsistent world and crashes with:After
It appears to work.
Internally, the installation flushes various caches, including the container (in local/PHP memory as well as the file-based cache). This is better for automated tests. For production, it basically works -- it ignores the inconsistent interim state (seen in 5.54.1), and it ultimately works because (as in 5.54.0) the final content of the container is correct.