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 --- .../shirabe/src/installer/installation_manager.rs | 34 ++++++++++++++-------- 1 file changed, 22 insertions(+), 12 deletions(-) (limited to 'crates/shirabe/src/installer/installation_manager.rs') diff --git a/crates/shirabe/src/installer/installation_manager.rs b/crates/shirabe/src/installer/installation_manager.rs index 8d248a76..31bfaa2c 100644 --- a/crates/shirabe/src/installer/installation_manager.rs +++ b/crates/shirabe/src/installer/installation_manager.rs @@ -48,7 +48,9 @@ pub struct InstallationManager { /// For testing only: present iff this manager behaves like /// `Composer\Test\Mock\InstallationManagerMock`, recording operations instead of executing /// them. `None` in production. - mock: Option, + // RefCell so the recording survives `execute(&self)` (shared borrows of the manager + // handle must coexist with re-entrant plugin registration; see `execute`). + mock: Option>, } /// For testing only: recorded operations for the `InstallationManagerMock` behavior. @@ -86,7 +88,9 @@ impl InstallationManager { event_dispatcher: Option>>, ) -> Self { Self { - mock: Some(InstallationManagerMockState::default()), + mock: Some(std::cell::RefCell::new( + InstallationManagerMockState::default(), + )), ..Self::new(loop_, io, event_dispatcher) } } @@ -95,7 +99,7 @@ impl InstallationManager { pub fn __get_trace(&self) -> Vec { self.mock .as_ref() - .map(|m| m.trace.clone()) + .map(|m| m.borrow().trace.clone()) .unwrap_or_default() } @@ -103,7 +107,7 @@ impl InstallationManager { pub fn __get_installed_packages(&self) -> Vec { self.mock .as_ref() - .map(|m| m.installed.clone()) + .map(|m| m.borrow().installed.clone()) .unwrap_or_default() } @@ -111,7 +115,7 @@ impl InstallationManager { pub fn __get_updated_packages(&self) -> Vec<(PackageInterfaceHandle, PackageInterfaceHandle)> { self.mock .as_ref() - .map(|m| m.updated.clone()) + .map(|m| m.borrow().updated.clone()) .unwrap_or_default() } @@ -119,7 +123,7 @@ impl InstallationManager { pub fn __get_uninstalled_packages(&self) -> Vec { self.mock .as_ref() - .map(|m| m.uninstalled.clone()) + .map(|m| m.borrow().uninstalled.clone()) .unwrap_or_default() } @@ -236,8 +240,13 @@ impl InstallationManager { } /// Executes solver operation. + /// + /// `&self` (not `&mut self`, unlike the porting default): callers invoke this through the + /// shared manager handle, and plugin registration inside the batch re-enters the same + /// handle (`PluginManager::get_install_path`), so only shared borrows may be outstanding + /// for the whole call. pub fn execute( - &mut self, + &self, repo: &InstalledRepositoryInterfaceHandle, operations: Vec, dev_mode: bool, @@ -248,7 +257,8 @@ impl InstallationManager { // skipping the download step (ref InstallationManagerMock::execute). The alias operations' // repo mutation is inlined (rather than calling mark_alias_*) so `self.mock` can stay // borrowed across the loop without also borrowing `&self`. - if let Some(mock) = self.mock.as_mut() { + if let Some(mock) = self.mock.as_ref() { + let mut mock = mock.borrow_mut(); let _ = (dev_mode, run_scripts, download_only); let mut repo = repo.borrow_mut(); for operation in operations { @@ -392,7 +402,7 @@ impl InstallationManager { // do a last write so that we write the repository even if nothing changed // as that can trigger an update of some files like InstalledVersions.php if // running a new composer version - repo.borrow_mut().write(dev_mode, self); + repo.borrow_mut().write(dev_mode, self)?; Ok(()) } @@ -658,7 +668,7 @@ impl InstallationManager { } // PHP: ->then(fn() => $repo->write($devMode, $this)) persists the repository after each op. - repo.borrow_mut().write(dev_mode, self); + repo.borrow_mut().write(dev_mode, self)?; let event_name_post = match op_type { "install" => PackageEvents::POST_PACKAGE_INSTALL, @@ -1020,7 +1030,7 @@ pub trait InstallationManagerInterface: std::fmt::Debug { ) -> anyhow::Result; fn ensure_binaries_presence(&mut self, package: PackageInterfaceHandle); fn execute( - &mut self, + &self, repo: &InstalledRepositoryInterfaceHandle, operations: Vec, dev_mode: bool, @@ -1062,7 +1072,7 @@ impl InstallationManagerInterface for InstallationManager { } fn execute( - &mut self, + &self, repo: &InstalledRepositoryInterfaceHandle, operations: Vec, dev_mode: bool, -- cgit v1.3.1