diff options
| author | nsfisis <nsfisis@gmail.com> | 2026-08-04 03:03:10 +0900 |
|---|---|---|
| committer | nsfisis <nsfisis@gmail.com> | 2026-08-04 05:43:20 +0900 |
| commit | 6cb1849473792bd73dbfb6265d363f149f687572 (patch) | |
| tree | 78f02030ac91929f449072d504673e9e6a21e2a2 /crates | |
| parent | a02fc7d728a9973a3275a0f47604081c4439b424 (diff) | |
| download | php-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')
| -rw-r--r-- | crates/shirabe-php-rpc/php/stubs/Composer/IO/BaseIO.php | 10 | ||||
| -rw-r--r-- | crates/shirabe-php-rpc/php/stubs/Composer/PartialComposer.php | 10 | ||||
| -rw-r--r-- | crates/shirabe-php-rpc/php/worker.php | 42 | ||||
| -rw-r--r-- | crates/shirabe-php-rpc/src/lib.rs | 6 | ||||
| -rw-r--r-- | crates/shirabe/src/command/create_project_command.rs | 16 | ||||
| -rw-r--r-- | crates/shirabe/src/command/reinstall_command.rs | 4 | ||||
| -rw-r--r-- | crates/shirabe/src/installer.rs | 4 | ||||
| -rw-r--r-- | crates/shirabe/src/installer/installation_manager.rs | 34 | ||||
| -rw-r--r-- | crates/shirabe/src/plugin/php_plugin_proxy.rs | 42 | ||||
| -rw-r--r-- | crates/shirabe/src/plugin/plugin_interface.rs | 2 | ||||
| -rw-r--r-- | crates/shirabe/src/plugin/plugin_manager.rs | 125 | ||||
| -rw-r--r-- | crates/shirabe/src/repository/filesystem_repository.rs | 21 | ||||
| -rw-r--r-- | crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs | 2 | ||||
| -rw-r--r-- | crates/shirabe/tests/plugin/plugin_installer_test.rs | 4 | ||||
| -rw-r--r-- | crates/shirabe/tests/repository/filesystem_repository_test.rs | 2 |
15 files changed, 212 insertions, 112 deletions
diff --git a/crates/shirabe-php-rpc/php/stubs/Composer/IO/BaseIO.php b/crates/shirabe-php-rpc/php/stubs/Composer/IO/BaseIO.php index 3a268c22..51cdbe08 100644 --- a/crates/shirabe-php-rpc/php/stubs/Composer/IO/BaseIO.php +++ b/crates/shirabe-php-rpc/php/stubs/Composer/IO/BaseIO.php @@ -16,8 +16,16 @@ abstract class BaseIO implements IOInterface, \ShirabeRustStub /** @var int */ protected $__epoch; - public function __construct(int $rhandle, int $epoch) + public function __construct(int $rhandle = 0, int $epoch = 0) { + if (func_num_args() < 2) { + // Constructing the class from plugin code (a common idiom for e.g. `new BufferIO()`) + // is an open question of the plugin design; only proxy instantiation passes a + // Rust handle. Fail with a diagnosable message instead of an ArgumentCountError. + throw new \RuntimeException( + 'Shirabe does not support constructing ' . static::class . ' inside the plugin process yet' + ); + } $this->__rhandle = $rhandle; $this->__epoch = $epoch; } diff --git a/crates/shirabe-php-rpc/php/stubs/Composer/PartialComposer.php b/crates/shirabe-php-rpc/php/stubs/Composer/PartialComposer.php index 7f085178..2641b267 100644 --- a/crates/shirabe-php-rpc/php/stubs/Composer/PartialComposer.php +++ b/crates/shirabe-php-rpc/php/stubs/Composer/PartialComposer.php @@ -21,8 +21,16 @@ class PartialComposer implements \ShirabeRustStub /** @var int */ protected $__epoch; - public function __construct(int $rhandle, int $epoch) + public function __construct(int $rhandle = 0, int $epoch = 0) { + if (func_num_args() < 2) { + // Constructing the class from plugin code (a common idiom for e.g. `new BufferIO()`) + // is an open question of the plugin design; only proxy instantiation passes a + // Rust handle. Fail with a diagnosable message instead of an ArgumentCountError. + throw new \RuntimeException( + 'Shirabe does not support constructing ' . static::class . ' inside the plugin process yet' + ); + } $this->__rhandle = $rhandle; $this->__epoch = $epoch; } 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; }, diff --git a/crates/shirabe-php-rpc/src/lib.rs b/crates/shirabe-php-rpc/src/lib.rs index cfee5e4c..9c5dae55 100644 --- a/crates/shirabe-php-rpc/src/lib.rs +++ b/crates/shirabe-php-rpc/src/lib.rs @@ -508,9 +508,9 @@ fn rpc_call( send_frame(&reply)?; } Frame::ReleaseRustHandle { .. } => { - // TODO(plugin): there is no persistent R table yet (script-event handles are - // scoped to a single dispatched call), so stub destructor notifications carry no - // state to clean up. + // TODO(plugin): R-table garbage collection is deferred — the shirabe crate + // keeps its entries alive for the worker's lifetime, and per-call script-event + // handles carry no state either, so the notification is dropped here. continue; } Frame::EpochBump { .. } => { diff --git a/crates/shirabe/src/command/create_project_command.rs b/crates/shirabe/src/command/create_project_command.rs index a907bc09..1e289b08 100644 --- a/crates/shirabe/src/command/create_project_command.rs +++ b/crates/shirabe/src/command/create_project_command.rs @@ -1008,20 +1008,26 @@ impl CreateProjectCommand { let project_installer = ProjectInstaller::new(&directory, dm.clone(), fs); let installation_manager = composer.get_installation_manager().clone(); - let mut im = installation_manager.borrow_mut(); - im.set_output_progress(!no_progress); - im.add_installer(Box::new(project_installer)); + { + let mut im = installation_manager.borrow_mut(); + im.set_output_progress(!no_progress); + im.add_installer(Box::new(project_installer)); + } let installed_repo = crate::repository::InstalledRepositoryInterfaceHandle::new( InstalledArrayRepository::new()?, ); - im.execute( + // A shared borrow: plugin registration inside execute re-enters this manager handle + // through the Composer graph. + installation_manager.borrow().execute( &installed_repo, vec![InstallOperation::new(package.clone()).into()], true, true, false, )?; - im.notify_installs(io.clone()); + installation_manager + .borrow_mut() + .notify_installs(io.clone()); // collect suggestions self.suggested_packages_reporter diff --git a/crates/shirabe/src/command/reinstall_command.rs b/crates/shirabe/src/command/reinstall_command.rs index 0d3567a2..05543870 100644 --- a/crates/shirabe/src/command/reinstall_command.rs +++ b/crates/shirabe/src/command/reinstall_command.rs @@ -247,14 +247,14 @@ impl Command for ReinstallCommand { let repo = crate::repository::InstalledRepositoryInterfaceHandle::from_repository_handle( &local_repo, ); - installation_manager.borrow_mut().execute( + installation_manager.borrow().execute( &repo, uninstall_operations, dev_mode, true, false, )?; - installation_manager.borrow_mut().execute( + installation_manager.borrow().execute( &repo, install_operations.clone(), dev_mode, diff --git a/crates/shirabe/src/installer.rs b/crates/shirabe/src/installer.rs index f68ab35f..bda4e2f6 100644 --- a/crates/shirabe/src/installer.rs +++ b/crates/shirabe/src/installer.rs @@ -1257,7 +1257,9 @@ impl Installer { if self.execute_operations { local_repo.set_dev_package_names(self.locker.borrow_mut().get_dev_package_names()?); - self.installation_manager.borrow_mut().execute( + // A shared borrow: plugin registration inside execute re-enters this manager + // handle through the Composer graph. + self.installation_manager.borrow().execute( &crate::repository::InstalledRepositoryInterfaceHandle::from_repository_handle( &local_repo, ), 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, diff --git a/crates/shirabe/src/plugin/php_plugin_proxy.rs b/crates/shirabe/src/plugin/php_plugin_proxy.rs index 87d67ea4..09867079 100644 --- a/crates/shirabe/src/plugin/php_plugin_proxy.rs +++ b/crates/shirabe/src/plugin/php_plugin_proxy.rs @@ -15,7 +15,7 @@ use shirabe_php_rpc::{ }; /// A Rust-side entity a PHP proxy stub points back to. -#[derive(Debug)] +#[derive(Debug, Clone)] enum RustEntity { Composer(ComposerHandle), Io(std::rc::Rc<std::cell::RefCell<dyn IOInterface>>), @@ -25,6 +25,10 @@ thread_local! { /// The R table. Entries are strong references kept for the worker's lifetime. /// TODO(plugin): GC (dropping entries on ReleaseRustHandle) is not implemented yet; /// until then entities registered here are intentionally never released. + /// TODO(plugin): thread-local while the worker and its stub intern table are + /// process-global; the session lock serializes calls, and every dispatch currently runs on + /// the thread that registered the handle, but a handle minted on one thread is invisible + /// to another. static R_TABLE: std::cell::RefCell<IndexMap<u64, RustEntity>> = std::cell::RefCell::new(IndexMap::new()); } @@ -96,6 +100,12 @@ pub(crate) fn io_stub_class( /// Looks a class up in every registered Rust-side `ClassLoader`, in registration order — the /// Rust mirror of what the PHP `spl_autoload_register` stack would do in-process. +/// +/// TODO(plugin): `ClassLoader::register` keeps one loader per vendor-dir (matching upstream's +/// `$registeredLoaders`), but the real spl stack keeps every registered loader; because the +/// `spl_autoload_register` shim is a no-op, registering a second plugin loader under the same +/// vendor-dir evicts the first one here, and a class of the earlier plugin that was never +/// loaded can become unresolvable (PHP would still find it). pub(crate) fn find_file_in_registered_loaders(class: &str) -> Option<String> { for (_vendor_dir, mut loader) in ClassLoader::get_registered_loaders() { if let Some(file) = loader.find_file(class) { @@ -122,6 +132,7 @@ impl RustMethodDispatcher for PluginRpcDispatcher { if rhandle == 0 { if method_name == "__shirabe_find_file" { let class = match args.first() { + // TODO(phase-e): lossy UTF-8; class names are bytes in PHP. Some(PluginValue::String(bytes)) => String::from_utf8_lossy(bytes).into_owned(), other => { return Err(runtime_throw(format!( @@ -139,20 +150,20 @@ impl RustMethodDispatcher for PluginRpcDispatcher { ))); } - R_TABLE.with(|table| { - let table = table.borrow(); - match table.get(&rhandle) { - Some(RustEntity::Io(io)) => dispatch_io_method(io, method_name, &args), - Some(RustEntity::Composer(_)) => { - // TODO(plugin): the Composer object graph (getConfig, getRepositoryManager, - // ...) becomes reachable over RPC later. - Err(runtime_throw(format!( - "the Composer method `{method_name}` is not available over RPC yet" - ))) - } - None => Err(runtime_throw(format!("unknown Rust handle {rhandle}"))), + // The entity is cloned out so no table borrow is held while the handler runs (a + // handler that re-enters register_*_entity would otherwise panic on the RefCell). + let entity = R_TABLE.with(|table| table.borrow().get(&rhandle).cloned()); + match entity { + Some(RustEntity::Io(io)) => dispatch_io_method(&io, method_name, &args), + Some(RustEntity::Composer(_)) => { + // TODO(plugin): the Composer object graph (getConfig, getRepositoryManager, + // ...) becomes reachable over RPC later. + Err(runtime_throw(format!( + "the Composer method `{method_name}` is not available over RPC yet" + ))) } - }) + None => Err(runtime_throw(format!("unknown Rust handle {rhandle}"))), + } } } @@ -193,6 +204,7 @@ fn decode_write_args( method_name: &str, args: &[PluginValue], ) -> Result<(Vec<String>, bool, i64), PhpThrow> { + // TODO(phase-e): lossy UTF-8; IO messages are bytes in PHP. let messages = match args.first() { Some(PluginValue::String(bytes)) => vec![String::from_utf8_lossy(bytes).into_owned()], Some(PluginValue::List(items)) => { @@ -347,7 +359,7 @@ impl PluginInterface for PhpPluginProxy { self.class.clone() } - fn as_php_plugin_proxy(&self) -> Option<&PhpPluginProxy> { + fn __as_php_plugin_proxy(&self) -> Option<&PhpPluginProxy> { Some(self) } } diff --git a/crates/shirabe/src/plugin/plugin_interface.rs b/crates/shirabe/src/plugin/plugin_interface.rs index 54fa6009..989329ea 100644 --- a/crates/shirabe/src/plugin/plugin_interface.rs +++ b/crates/shirabe/src/plugin/plugin_interface.rs @@ -44,7 +44,7 @@ pub trait PluginInterface: std::fmt::Debug { /// For testing only: recovers the PHP-backed proxy so tests can read plugin properties the /// way PHPUnit asserts `$plugins[0]->version`. - fn as_php_plugin_proxy(&self) -> Option<&crate::plugin::PhpPluginProxy> { + fn __as_php_plugin_proxy(&self) -> Option<&crate::plugin::PhpPluginProxy> { None } } diff --git a/crates/shirabe/src/plugin/plugin_manager.rs b/crates/shirabe/src/plugin/plugin_manager.rs index 6760bf20..65c4b03f 100644 --- a/crates/shirabe/src/plugin/plugin_manager.rs +++ b/crates/shirabe/src/plugin/plugin_manager.rs @@ -272,15 +272,19 @@ impl PluginManager { .map(|v| v.as_bool() == Some(true)) .unwrap_or(false); if !self.is_plugin_allowed(&package.get_name(), is_global_plugin, plugin_optional, true)? { - self.io.write_error(&format!( - "Skipped loading \"{}\" {}as it is not in config.allow-plugins", - package.get_name(), - if is_global_plugin || self.running_in_global_dir { - "(installed globally) " - } else { - "" - } - )); + self.io.write_error3( + &format!( + "Skipped loading \"{}\" {}as it is not in config.allow-plugins", + package.get_name(), + if is_global_plugin || self.running_in_global_dir { + "(installed globally) " + } else { + "" + } + ), + true, + crate::io::DEBUG, + ); return Ok(()); } @@ -298,6 +302,7 @@ impl PluginManager { Some(PhpMixed::Null) => true, Some(PhpMixed::Bool(false)) => true, Some(PhpMixed::Int(0)) => true, + Some(PhpMixed::Float(f)) if *f == 0.0 => true, Some(PhpMixed::String(s)) if s.is_empty() || s == "0" => true, Some(PhpMixed::Array(a)) => a.is_empty(), Some(PhpMixed::List(l)) => l.is_empty(), @@ -309,17 +314,19 @@ impl PluginManager { code: 0, }.into()); } - let classes: Vec<String> = if let Some(arr) = class_value.and_then(|v| v.as_list()) { - arr.iter() - .filter_map(|v| v.as_string().map(|s| s.to_string())) - .collect() - } else { - vec![ - class_value - .and_then(|v| v.as_string()) - .unwrap_or("") - .to_string(), - ] + // PHP: is_array($extra['class']) ? $extra['class'] : [$extra['class']] — an associative + // array iterates its values too, and a non-string entry reaches class_exists() where it + // raises a TypeError (an Error, not caught by the plugin installer's rollback). + let expect_class_name = |value: &PhpMixed| -> String { + value.as_string().map(|s| s.to_string()).unwrap_or_else(|| { + panic!("extra.class entries must be strings (PHP raises a TypeError): {value:?}") + }) + }; + let classes: Vec<String> = match class_value { + Some(PhpMixed::List(items)) => items.iter().map(expect_class_name).collect(), + Some(PhpMixed::Array(map)) => map.values().map(expect_class_name).collect(), + Some(other) => vec![expect_class_name(other)], + None => unreachable!("empty() above rejected a missing extra.class"), }; let composer = self.composer_full(); @@ -421,6 +428,8 @@ impl PluginManager { let path = class_loader.find_file(&class).unwrap_or_else(|| { panic!("plugin class `{class}` is already defined but has no autoloadable file") }); + // TODO(phase-e): file_get_contents is lossy UTF-8; the eval'd plugin source + // should be carried as bytes. let code = file_get_contents(&path) .unwrap_or_else(|| panic!("unable to read the plugin class file `{path}`")); let class_counter = CLASS_COUNTER.load(std::sync::atomic::Ordering::Relaxed); @@ -578,24 +587,34 @@ impl PluginManager { return Ok(()); } - let plugins = self + // PHP unsets registeredPlugins only after the loop; a deactivate() throw must leave the + // entry observable, so removal happens last here too. Plugins are cloned out per index + // (shared handles); installer entries are only referenced while calling removeInstaller. + let name = package.get_name(); + let count = self .registered_plugins - .shift_remove(&package.get_name()) - .unwrap_or_default(); - for plugin in plugins { + .get(&name) + .map(|entries| entries.len()) + .unwrap_or(0); + for index in 0..count { + let plugin = match &self.registered_plugins.get(&name).unwrap()[index] { + PluginOrInstaller::Plugin(p) => Some(p.clone()), + PluginOrInstaller::Installer(_) => None, + }; match plugin { - PluginOrInstaller::Installer(inst) => { - self.composer_full() - .borrow() - .get_installation_manager() - .borrow_mut() - .remove_installer(&*inst); - } - PluginOrInstaller::Plugin(p) => { - self.remove_plugin(&p)?; + Some(p) => self.remove_plugin(&p)?, + None => { + let composer = self.composer_full(); + let installation_manager = composer.borrow().get_installation_manager(); + if let PluginOrInstaller::Installer(inst) = + &self.registered_plugins.get(&name).unwrap()[index] + { + installation_manager.borrow_mut().remove_installer(&**inst); + } } } } + self.registered_plugins.shift_remove(&name); Ok(()) } @@ -605,25 +624,35 @@ impl PluginManager { return Ok(()); } - let plugins = self + // PHP unsets registeredPlugins only after the loop, as in deactivate_package. + let name = package.get_name(); + let count = self .registered_plugins - .shift_remove(&package.get_name()) - .unwrap_or_default(); - for plugin in plugins { + .get(&name) + .map(|entries| entries.len()) + .unwrap_or(0); + for index in 0..count { + let plugin = match &self.registered_plugins.get(&name).unwrap()[index] { + PluginOrInstaller::Plugin(p) => Some(p.clone()), + PluginOrInstaller::Installer(_) => None, + }; match plugin { - PluginOrInstaller::Installer(inst) => { - self.composer_full() - .borrow() - .get_installation_manager() - .borrow_mut() - .remove_installer(&*inst); - } - PluginOrInstaller::Plugin(p) => { + Some(p) => { self.remove_plugin(&p)?; self.uninstall_plugin(&p)?; } + None => { + let composer = self.composer_full(); + let installation_manager = composer.borrow().get_installation_manager(); + if let PluginOrInstaller::Installer(inst) = + &self.registered_plugins.get(&name).unwrap()[index] + { + installation_manager.borrow_mut().remove_installer(&**inst); + } + } } } + self.registered_plugins.shift_remove(&name); Ok(()) } @@ -895,11 +924,13 @@ impl PluginManager { global: bool, ) -> Option<String> { if !global { + // Shared borrow: this runs re-entrantly while InstallationManager::execute holds a + // shared borrow of the same manager handle. return self .composer_full() .borrow() .get_installation_manager() - .borrow_mut() + .borrow() .get_install_path(package); } @@ -909,7 +940,7 @@ impl PluginManager { .unwrap() .borrow_partial() .get_installation_manager() - .borrow_mut() + .borrow() .get_install_path(package) } diff --git a/crates/shirabe/src/repository/filesystem_repository.rs b/crates/shirabe/src/repository/filesystem_repository.rs index b2a2530e..1949803b 100644 --- a/crates/shirabe/src/repository/filesystem_repository.rs +++ b/crates/shirabe/src/repository/filesystem_repository.rs @@ -343,18 +343,21 @@ impl FilesystemRepository { ); // make sure the in memory state is up to date with on disk - // The upstream in-process reload is split in two here: the Rust-side mirror below, - // and a push into the PHP worker where the real observers (plugins) live. The push - // is skipped when no worker is running — with no child there is nothing that could - // observe the stale state, and the worker glue additionally ignores it while the - // class is not loaded there (a later lazy load reads the freshly written - // installed.php, matching upstream observations in every case). + // The upstream in-process reload/selfDir/installedIsLocalDir tail is mirrored + // twice: into the Rust-side statics below, and into the PHP worker where the real + // observers (plugins) live. The push is skipped when no worker is running — with no + // child there is nothing that could observe the state; the glue skips it only when + // the class is not even autoloadable there (no Composer PHP runtime = no observer + // code either). if shirabe_php_rpc::worker_is_running() { crate::event_dispatcher::unwrap_php_result(shirabe_php_rpc::call_function( "__shirabe_installed_versions_reload", - vec![shirabe_php_rpc::PluginValue::from_php_mixed( - &PhpMixed::Array(versions.clone()), - )], + vec![ + shirabe_php_rpc::PluginValue::from_php_mixed(&PhpMixed::Array( + versions.clone(), + )), + shirabe_php_rpc::PluginValue::string(repo_dir.clone()), + ], ))?; } InstalledVersions::reload(versions); diff --git a/crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs b/crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs index 9cd51a7b..5a6c93e5 100644 --- a/crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs +++ b/crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs @@ -431,7 +431,7 @@ mockall::mock! { ) -> anyhow::Result<bool>; fn ensure_binaries_presence(&mut self, package: PackageInterfaceHandle); fn execute( - &mut self, + &self, repo: &shirabe::repository::InstalledRepositoryInterfaceHandle, operations: Vec<AnyOperation>, dev_mode: bool, diff --git a/crates/shirabe/tests/plugin/plugin_installer_test.rs b/crates/shirabe/tests/plugin/plugin_installer_test.rs index e3f89851..5dfb6e06 100644 --- a/crates/shirabe/tests/plugin/plugin_installer_test.rs +++ b/crates/shirabe/tests/plugin/plugin_installer_test.rs @@ -173,7 +173,7 @@ impl InstallationManagerInterface for MockInstallationManager { fn ensure_binaries_presence(&mut self, _package: PackageInterfaceHandle) {} fn execute( - &mut self, + &self, _repo: &InstalledRepositoryInterfaceHandle, _operations: Vec<AnyOperation>, _dev_mode: bool, @@ -372,7 +372,7 @@ fn plugin_property( ) -> String { let plugin = plugin.borrow(); let proxy = plugin - .as_php_plugin_proxy() + .__as_php_plugin_proxy() .expect("registered plugins are PHP-backed proxies"); match proxy.__get_property(name).unwrap() { PhpMixed::String(s) => s, diff --git a/crates/shirabe/tests/repository/filesystem_repository_test.rs b/crates/shirabe/tests/repository/filesystem_repository_test.rs index 323aff9f..bcdb766e 100644 --- a/crates/shirabe/tests/repository/filesystem_repository_test.rs +++ b/crates/shirabe/tests/repository/filesystem_repository_test.rs @@ -103,7 +103,7 @@ mockall::mock! { ) -> anyhow::Result<bool>; fn ensure_binaries_presence(&mut self, package: PackageInterfaceHandle); fn execute( - &mut self, + &self, repo: &shirabe::repository::InstalledRepositoryInterfaceHandle, operations: Vec<AnyOperation>, dev_mode: bool, |
