aboutsummaryrefslogtreecommitdiffhomepage
path: root/crates/shirabe/src/installer
diff options
context:
space:
mode:
authornsfisis <nsfisis@gmail.com>2026-08-04 03:03:10 +0900
committernsfisis <nsfisis@gmail.com>2026-08-04 05:43:20 +0900
commit6cb1849473792bd73dbfb6265d363f149f687572 (patch)
tree78f02030ac91929f449072d504673e9e6a21e2a2 /crates/shirabe/src/installer
parenta02fc7d728a9973a3275a0f47604081c4439b424 (diff)
downloadphp-shirabe-6cb1849473792bd73dbfb6265d363f149f687572.tar.gz
php-shirabe-6cb1849473792bd73dbfb6265d363f149f687572.tar.zst
php-shirabe-6cb1849473792bd73dbfb6265d363f149f687572.zip
fix(plugin): resolve review findings in the plugin activation flow
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 <noreply@anthropic.com>
Diffstat (limited to 'crates/shirabe/src/installer')
-rw-r--r--crates/shirabe/src/installer/installation_manager.rs34
1 files changed, 22 insertions, 12 deletions
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<InstallationManagerMockState>,
+ // RefCell so the recording survives `execute(&self)` (shared borrows of the manager
+ // handle must coexist with re-entrant plugin registration; see `execute`).
+ mock: Option<std::cell::RefCell<InstallationManagerMockState>>,
}
/// For testing only: recorded operations for the `InstallationManagerMock` behavior.
@@ -86,7 +88,9 @@ impl InstallationManager {
event_dispatcher: Option<std::rc::Rc<std::cell::RefCell<EventDispatcher>>>,
) -> 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<String> {
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<PackageInterfaceHandle> {
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<PackageInterfaceHandle> {
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<AnyOperation>,
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<bool>;
fn ensure_binaries_presence(&mut self, package: PackageInterfaceHandle);
fn execute(
- &mut self,
+ &self,
repo: &InstalledRepositoryInterfaceHandle,
operations: Vec<AnyOperation>,
dev_mode: bool,
@@ -1062,7 +1072,7 @@ impl InstallationManagerInterface for InstallationManager {
}
fn execute(
- &mut self,
+ &self,
repo: &InstalledRepositoryInterfaceHandle,
operations: Vec<AnyOperation>,
dev_mode: bool,