aboutsummaryrefslogtreecommitdiffhomepage
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
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>
-rw-r--r--crates/shirabe-php-rpc/php/stubs/Composer/IO/BaseIO.php10
-rw-r--r--crates/shirabe-php-rpc/php/stubs/Composer/PartialComposer.php10
-rw-r--r--crates/shirabe-php-rpc/php/worker.php42
-rw-r--r--crates/shirabe-php-rpc/src/lib.rs6
-rw-r--r--crates/shirabe/src/command/create_project_command.rs16
-rw-r--r--crates/shirabe/src/command/reinstall_command.rs4
-rw-r--r--crates/shirabe/src/installer.rs4
-rw-r--r--crates/shirabe/src/installer/installation_manager.rs34
-rw-r--r--crates/shirabe/src/plugin/php_plugin_proxy.rs42
-rw-r--r--crates/shirabe/src/plugin/plugin_interface.rs2
-rw-r--r--crates/shirabe/src/plugin/plugin_manager.rs125
-rw-r--r--crates/shirabe/src/repository/filesystem_repository.rs21
-rw-r--r--crates/shirabe/tests/event_dispatcher/event_dispatcher_test.rs2
-rw-r--r--crates/shirabe/tests/plugin/plugin_installer_test.rs4
-rw-r--r--crates/shirabe/tests/repository/filesystem_repository_test.rs2
-rw-r--r--docs/dev/php-rpc.md7
16 files changed, 216 insertions, 115 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,
diff --git a/docs/dev/php-rpc.md b/docs/dev/php-rpc.md
index c43fdfd5..f8f42e90 100644
--- a/docs/dev/php-rpc.md
+++ b/docs/dev/php-rpc.md
@@ -122,9 +122,10 @@ function; an unknown name is an explicit error. Notable internal helpers:
- `__shirabe_composer_require` — the body of `\Composer\Autoload\composerRequire`, sharing
its `$GLOBALS['__composer_autoload_files']` guard (files-autoload entries of plugin
packages).
-- `__shirabe_installed_versions_reload` — mirrors `FilesystemRepository::write`'s in-process
- `InstalledVersions::reload($versions)` into the worker; guarded by
- `class_exists(..., false)` so an unloaded class keeps its upstream lazy-load behavior.
+- `__shirabe_installed_versions_reload` — mirrors the tail of `FilesystemRepository::write`
+ (the unconditional `InstalledVersions::reload($versions)` plus the reflection-based
+ `selfDir`/`installedIsLocalDir` restore) into the worker; skipped only when the class is not
+ even autoloadable there, i.e. no Composer PHP runtime and therefore no observer code.
- `__shirabe_get_property` — for testing only: reads a public property of a P-table entity.
- `__shirabe_oracle_roundtrip` — codec oracle support for tests.