From 6cb1849473792bd73dbfb6265d363f149f687572 Mon Sep 17 00:00:00 2001 From: nsfisis Date: Tue, 4 Aug 2026 03:03:10 +0900 Subject: fix(plugin): resolve review findings in the plugin activation flow MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit InstallationManager::execute now takes &self (the mock recorder moved into a RefCell) and its callers hold only shared borrows: plugin registration inside a batch re-enters the same manager handle through Composer::getInstallationManager()->getInstallPath(), which panicked on the RefCell re-borrow under the &mut shape — the same re-entrancy the repository side already fixed, unreachable from the ported tests because they call PluginInstaller::install directly like PHPUnit does. The worker-side InstalledVersions mirror now matches the full tail of FilesystemRepository::write: unconditional reload plus the reflection-based selfDir/installedIsLocalDir restore. The previous class_exists(false) guard rested on a lazy-load assumption that does not hold in the worker (its real ClassLoader only knows the Composer checkout's vendor dir, so a later lazy load would read the checkout's installed.php, not the project's); the mirror is now skipped only when the class is not autoloadable at all, i.e. no plugin runtime and hence no observer code. Boot-time seeding stays TODO(plugin). Also from the review: registered_plugins entries are removed only after the deactivate/uninstall loop (PHP unsets last, and a throw must leave the entry observable); extra.class keeps associative-array values and fails loudly on non-strings instead of silently dropping them; the two discarded write() results now propagate (they carry the reload-push failure); register_package's allow-plugins skip message is DEBUG like the addPlugin side; the loader-eviction divergence of REGISTERED_LOADERS and the lossy UTF-8 spots carry searchable markers; the test-only proxy downcast follows the __ naming rule; the R-table dispatch clones the entity out instead of holding the table borrow across the handler; the IO/PartialComposer stubs turn a plugin-side `new NullIO()` into an explicit error instead of an ArgumentCountError; and the empty() emulation covers float 0.0. Co-Authored-By: Claude Fable 5 --- crates/shirabe-php-rpc/php/worker.php | 42 ++++++++++++++++++++++++++--------- 1 file changed, 31 insertions(+), 11 deletions(-) (limited to 'crates/shirabe-php-rpc/php/worker.php') diff --git a/crates/shirabe-php-rpc/php/worker.php b/crates/shirabe-php-rpc/php/worker.php index d8f10e93..50e9ef76 100644 --- a/crates/shirabe-php-rpc/php/worker.php +++ b/crates/shirabe-php-rpc/php/worker.php @@ -365,11 +365,6 @@ final class ShirabeRpcRuntime } } - /** - * Registers the script-class autoloader: classes referenced by composer.json scripts are - * resolved by asking the Rust-side ClassLoader (built by EventDispatcher::makeAutoloader) - * where the class file lives. Handle 0 is the runtime service endpoint on the Rust side. - */ /** Re-prepends the stub autoloader so it precedes any autoloader registered since. */ public static function ensureStubAutoloaderPriority(): void { @@ -380,6 +375,11 @@ final class ShirabeRpcRuntime spl_autoload_register(self::$stubAutoloader, true, true); } + /** + * Registers the script-class autoloader: classes referenced by composer.json scripts are + * resolved by asking the Rust-side ClassLoader (built by EventDispatcher::makeAutoloader) + * where the class file lives. Handle 0 is the runtime service endpoint on the Rust side. + */ public static function enableScriptAutoloader(): void { if (self::$scriptAutoloaderRegistered) { @@ -577,13 +577,33 @@ ShirabeRpcRuntime::$dispatch = [ } return true; }, - // Mirrors FilesystemRepository::write's in-process `InstalledVersions::reload($versions)` - // into this child. The class_exists guard (no autoload) matches the upstream observable - // behavior: when the class was never loaded here, a later lazy load reads the - // freshly-written installed.php anyway. + // Mirrors the tail of FilesystemRepository::write into this child: the unconditional + // `InstalledVersions::reload($versions)` plus the reflection-based selfDir / + // installedIsLocalDir restore. Skipped only when the class is not even autoloadable here + // (the Composer PHP runtime was never loaded): without it no code in this process can + // observe InstalledVersions at all. + // TODO(plugin): seeding the initial state when the plugin runtime boots (the Factory-time + // safelyLoadInstalledVersions of the project's installed.php) is not wired yet; until the + // first write of a run, a plugin observes an unseeded InstalledVersions. '__shirabe_installed_versions_reload' => static function ($args) { - if (class_exists('Composer\\InstalledVersions', false)) { - \Composer\InstalledVersions::reload($args[0]); + [$versions, $repoDir] = $args; + if (!class_exists('Composer\\InstalledVersions')) { + return true; + } + \Composer\InstalledVersions::reload($versions); + try { + $reflProp = new ReflectionProperty(\Composer\InstalledVersions::class, 'selfDir'); + (\PHP_VERSION_ID < 80100) and $reflProp->setAccessible(true); + $reflProp->setValue(null, strtr($repoDir, '\\', '/')); + + $reflProp = new ReflectionProperty(\Composer\InstalledVersions::class, 'installedIsLocalDir'); + (\PHP_VERSION_ID < 80100) and $reflProp->setAccessible(true); + $reflProp->setValue(null, true); + } catch (ReflectionException $e) { + if (preg_match('{Property .*? does not exist}i', $e->getMessage()) !== 1) { + throw $e; + } + // noop, an outdated InstalledVersions class simply lacks the properties } return true; }, -- cgit v1.3.1