From b969dba4119547ecf46a00480ad2bcafca4795d8 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 09:33:47 +0000 Subject: [PATCH 1/3] fix(nc35): register OpenRegister PSR-4 prefix without OC_App OpenRegisterAutoloader::ensure() called \OC_App::registerAutoloading(), which Nextcloud 35 removed. The Error was swallowed by the catch-all, so ensure() returned false and the federated-config listener was never registered on 35. Register a PSR-4 loader for OCA\OpenRegister\ over the openregister app's lib/ with spl_autoload_register(), using only public IAppManager::isEnabledForAnyone() and getAppPath(). ensure()'s signature and call site are unchanged; it still never throws. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01M7DxSg7d7wqqZsUppLWf7J --- lib/AppInfo/OpenRegisterAutoloader.php | 145 ++++++++++++++---- .../AppInfo/OpenRegisterAutoloaderTest.php | 76 +++++++++ 2 files changed, 191 insertions(+), 30 deletions(-) diff --git a/lib/AppInfo/OpenRegisterAutoloader.php b/lib/AppInfo/OpenRegisterAutoloader.php index c8a30559e..1f39e87f4 100644 --- a/lib/AppInfo/OpenRegisterAutoloader.php +++ b/lib/AppInfo/OpenRegisterAutoloader.php @@ -27,8 +27,8 @@ * * WHY THIS EXISTS * --------------- - * `Coordinator::registerApps()` walks the SORTED app list, calling - * `OC_App::registerAutoloading()` and then `register()` for one app at a time. + * `Coordinator::registerApps()` walks the SORTED app list, registering each + * app's autoloading and then calling `register()` for one app at a time. * Under the app's OLD id, `nldesign` sorted before `openregister`, so * `Application::register()` ran while the `OCA\OpenRegister\` PSR-4 prefix did * not yet exist — on a completely healthy instance with OpenRegister installed @@ -42,21 +42,35 @@ * normally closed — but app sort order is not a contract to rely on and this * call is idempotent and cheap, so the prelude stays. * + * PUBLIC API ONLY (NEXTCLOUD 35) + * ------------------------------ + * This used to call `\OC_App::registerAutoloading()`. That is private API and + * Nextcloud 35 REMOVED it (moved to the equally private + * `OC\App\AppManager::registerAutoloading()`). The resulting `\Error` landed in + * the catch below, `ensure()` returned false and federated config sharing was + * switched off in silence. For an app shipping its own `vendor/autoload.php` — + * which OpenRegister does — Nextcloud's registration reduces to a PSR-4 prefix + * over `lib/`, so that is exactly what is registered here, with + * `spl_autoload_register()` and the public `IAppManager`. Same approach as + * keepiq#712. + * + * OpenRegister's `vendor/autoload.php` is deliberately NOT required: that would + * pull its whole dependency tree into this process, where versions differing + * from ours would win first-come. Deliberately NOT `IAppManager::loadApp()` + * either: that boots OpenRegister before its own `register()` has run. + * * WHY IT IS A CLASS AND NOT FOUR INLINE LINES * ------------------------------------------- - * Three reasons, all practical. It is independently unit-testable, which four - * lines inside `register()` are not — `Application` cannot be constructed in a - * unit test without a server container. It confines the one unavoidable - * static call to a single method instead of spreading it through bootstrap - * code. And `ensure()` is deliberately an INSTANCE method so the call site in - * `register()` is an ordinary method call: the PHPMD StaticAccess suppression - * below then covers exactly one line in one class, and the rule keeps its + * It is independently unit-testable, which inline lines inside `register()` + * are not — `Application` cannot be constructed in a unit test without a + * server container. And `ensure()` is deliberately an INSTANCE method so the + * call site in `register()` is an ordinary method call: the PHPMD StaticAccess + * suppression below then covers exactly one class, and the rule keeps its * teeth in the bootstrap and everywhere else. * - * @SuppressWarnings(PHPMD.StaticAccess) - `OC_App::registerAutoloading()` is - * the ONLY way to pull another app's PSR-4 prefix into the running process and - * Nextcloud exposes no instance API for it; ADR-040 prescribes this exact - * call. + * @SuppressWarnings(PHPMD.StaticAccess) - `\OCP\Server::get()` is the public + * service locator, and this runs at the composition root where no container + * is available to inject from. * * @spec openspec/specs/federated-config-sharing/spec.md */ @@ -70,25 +84,34 @@ final class OpenRegisterAutoloader { public const OPENREGISTER_APP_ID = 'openregister'; /** - * Register OpenRegister's PSR-4 prefix with the running process. + * The PSR-4 namespace prefix OpenRegister's `lib/` serves. * - * `registerAutoloading()` touches only the autoloader and is idempotent, - * so calling it is safe regardless of which apps registered before this - * one. Every failure is swallowed on purpose: OpenRegister is a SOFT - * dependency, and "absent or disabled" is a supported state in which the - * caller's `class_exists()` guard then answers FALSE truthfully. + * @var string + */ + private const OPENREGISTER_NAMESPACE = 'OCA\\OpenRegister\\'; + + /** + * The registered loader, or null when none is on the SPL chain. * - * The path is resolved into a local BEFORE the static call, deliberately. - * Written as one expression, PHP resolves `OC_App` first and raises its - * Error before `getAppPath()` is ever evaluated — so in any environment - * without the legacy class (a unit test, for one) the prelude would never - * even ask which path it wanted, and a test asserting that it does would - * fail for a reason unrelated to what it meant to check. + * Static on purpose: the SPL chain is process-wide, so one loader per + * process however many instances call `ensure()`. + * + * @var (\Closure(string): void)|null + */ + private static ?\Closure $loader = null; + + /** + * Register OpenRegister's PSR-4 prefix with the running process. + * + * Idempotent: a second call while registered is a no-op. Every failure is + * swallowed on purpose: OpenRegister is a SOFT dependency, and "absent or + * disabled" is a supported state in which the caller's `class_exists()` + * guard then answers FALSE truthfully. * * @param IAppManager|null $appManager Injected for tests; resolved from the * server container when omitted. * - * @return bool True when the prefix was registered, false when OpenRegister + * @return bool True when the prefix is registered, false when OpenRegister * is absent, disabled, or otherwise unreachable. * * @spec openspec/specs/federated-config-sharing/spec.md @@ -96,13 +119,75 @@ final class OpenRegisterAutoloader { public function ensure(?IAppManager $appManager = null): bool { try { $manager = ($appManager ?? Server::get(IAppManager::class)); - $path = $manager->getAppPath(self::OPENREGISTER_APP_ID); - \OC_App::registerAutoloading(self::OPENREGISTER_APP_ID, $path); + + // Checked before the short-circuit: under a long-lived worker the + // static loader outlives the request, and an OpenRegister disabled + // since must not stay wired. + if ($manager->isEnabledForAnyone(self::OPENREGISTER_APP_ID) === false) { + self::unregister(); + return false; + } + + if (self::$loader !== null) { + return true; + } + + $path = rtrim($manager->getAppPath(self::OPENREGISTER_APP_ID), '/'); + if (is_dir($path . '/lib') === false) { + return false; + } + + self::$loader = static function (string $class) use ($path): void { + $file = self::classFile(appPath: $path, class: $class); + if ($file !== null && is_file($file) === true) { + include_once $file; + } + }; + spl_autoload_register(self::$loader); + return true; } catch (Throwable) { // OpenRegister absent, disabled, or the server container is not - // available (unit tests). The caller degrades as designed. + // available (unit tests). The caller degrades as designed. Never + // rethrow: an exception escaping here would abort the caller's + // entire register(). return false; - } + }//end try }//end ensure() + + /** + * Take the loader off the SPL chain (tests, and OpenRegister disabled). + * + * @return void + */ + public static function unregister(): void { + if (self::$loader !== null) { + spl_autoload_unregister(self::$loader); + self::$loader = null; + } + }//end unregister() + + /** + * Map an `OCA\OpenRegister\…` class name to its file under `lib/`. + * + * Returns null for any class that is not OpenRegister's, so this loader + * never answers for (and shadows) names another loader owns. + * + * @param string $appPath Absolute path to the openregister app, no trailing slash. + * @param string $class The fully qualified class name being resolved. + * + * @return string|null The candidate file, or null when not OpenRegister's. + */ + public static function classFile(string $appPath, string $class): ?string { + if (str_starts_with($class, self::OPENREGISTER_NAMESPACE) === false) { + return null; + } + + $relative = substr($class, strlen(self::OPENREGISTER_NAMESPACE)); + if ($relative === '') { + return null; + } + + return $appPath . '/lib/' . str_replace('\\', '/', $relative) . '.php'; + }//end classFile() }//end class diff --git a/tests/Unit/AppInfo/OpenRegisterAutoloaderTest.php b/tests/Unit/AppInfo/OpenRegisterAutoloaderTest.php index a3a99dc52..008b4d999 100644 --- a/tests/Unit/AppInfo/OpenRegisterAutoloaderTest.php +++ b/tests/Unit/AppInfo/OpenRegisterAutoloaderTest.php @@ -33,6 +33,80 @@ */ class OpenRegisterAutoloaderTest extends TestCase { + /** + * Temporary fake openregister app directory. + * + * @var string + */ + private string $appPath; + + protected function setUp(): void { + parent::setUp(); + $this->appPath = sys_get_temp_dir() . '/thematiq-or-' . bin2hex(random_bytes(4)); + mkdir($this->appPath . '/lib/Fake', 0777, true); + file_put_contents( + $this->appPath . '/lib/Fake/Probe.php', + "appPath . '/lib/Fake/Probe.php'); + @rmdir($this->appPath . '/lib/Fake'); + @rmdir($this->appPath . '/lib'); + @rmdir($this->appPath); + parent::tearDown(); + }//end tearDown() + + /** + * Nextcloud 35 removed `OC_App::registerAutoloading()`, which this prelude + * used to call inside its catch-all — so on 35 it silently returned false. + * Only public API and plain PHP may be used. + */ + public function testSourceUsesNoPrivateOcAppApi(): void { + $source = (string) file_get_contents(__DIR__ . '/../../../lib/AppInfo/OpenRegisterAutoloader.php'); + $code = (string) preg_replace('#/\*.*?\*/|//[^\n]*#s', '', $source); + $this->assertStringNotContainsString('OC_App', $code); + }//end testSourceUsesNoPrivateOcAppApi() + + /** + * Enabled OpenRegister: the PSR-4 prefix is registered, idempotently, and + * an OpenRegister class actually becomes loadable. + */ + public function testRegistersPsr4PrefixWhenEnabled(): void { + $appManager = $this->createMock(IAppManager::class); + $appManager->method('isEnabledForAnyone')->with('openregister')->willReturn(true); + $appManager->method('getAppPath')->with('openregister')->willReturn($this->appPath . '/'); + + $this->assertTrue((new OpenRegisterAutoloader())->ensure($appManager)); + $this->assertTrue((new OpenRegisterAutoloader())->ensure($appManager)); + $this->assertTrue(class_exists('OCA\\OpenRegister\\Fake\\Probe')); + }//end testRegistersPsr4PrefixWhenEnabled() + + /** + * Disabled OpenRegister: false, and its path is never asked for. + */ + public function testDisabledOpenRegisterDegradesToFalse(): void { + $appManager = $this->createMock(IAppManager::class); + $appManager->method('isEnabledForAnyone')->willReturn(false); + $appManager->expects($this->never())->method('getAppPath'); + + $this->assertFalse((new OpenRegisterAutoloader())->ensure($appManager)); + }//end testDisabledOpenRegisterDegradesToFalse() + + /** + * The loader only ever answers for OpenRegister's namespace. + */ + public function testClassFileOnlyAnswersForOpenRegister(): void { + $this->assertSame( + '/x/lib/Db/Schema.php', + OpenRegisterAutoloader::classFile(appPath: '/x', class: 'OCA\\OpenRegister\\Db\\Schema') + ); + $this->assertNull(OpenRegisterAutoloader::classFile(appPath: '/x', class: 'OCA\\Thematiq\\Foo')); + $this->assertNull(OpenRegisterAutoloader::classFile(appPath: '/x', class: 'OCA\\OpenRegister\\')); + }//end testClassFileOnlyAnswersForOpenRegister() + /** * The app id must be the real one, not a near-miss. `getAppPath()` throws * for an unknown id, which this class swallows — so a typo here would @@ -50,6 +124,7 @@ public function testAppIdIsOpenregister(): void { */ public function testAbsentOpenRegisterDegradesToFalseWithoutThrowing(): void { $appManager = $this->createMock(IAppManager::class); + $appManager->method('isEnabledForAnyone')->willReturn(true); $appManager->method('getAppPath') ->willThrowException(new RuntimeException('App openregister not found')); @@ -62,6 +137,7 @@ public function testAbsentOpenRegisterDegradesToFalseWithoutThrowing(): void { */ public function testAsksTheAppManagerForOpenregistersPath(): void { $appManager = $this->createMock(IAppManager::class); + $appManager->method('isEnabledForAnyone')->with('openregister')->willReturn(true); $appManager->expects($this->once()) ->method('getAppPath') ->with('openregister') From 4877972e8c372110eeea70c52851cfc872b4ea51 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 09:34:31 +0000 Subject: [PATCH 2/3] chore(nc35): use OCP\ServerVersion instead of deprecated Util::getVersion OCP\Util::getVersion() is deprecated since 31; this app's floor is 32, so OCP\ServerVersion is always available. Resolved lazily from the server container, keeping the existing Throwable fallbacks. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01M7DxSg7d7wqqZsUppLWf7J --- lib/Service/ComplianceReportService.php | 9 ++++----- lib/Service/PlaygroundStateService.php | 11 ++++++----- 2 files changed, 10 insertions(+), 10 deletions(-) diff --git a/lib/Service/ComplianceReportService.php b/lib/Service/ComplianceReportService.php index 4747cedd6..5366a5963 100644 --- a/lib/Service/ComplianceReportService.php +++ b/lib/Service/ComplianceReportService.php @@ -725,10 +725,9 @@ private function buildMetadata(string $tokenSetId, array $tokenSetMeta, string $ /** * Resolve the Nextcloud server version string. * - * Uses the deprecated array-returning \OCP\Util::getVersion() rather than - * the newer \OCP\ServerVersion (NC 31.0.0+) because this app supports NC - * back to 28 (appinfo/info.xml) and a type-hinted ServerVersion - * constructor dependency would break DI autowiring on 28-30. Falls back + * Uses \OCP\ServerVersion (public since NC 31.0.0, below this app's floor + * of 32 in appinfo/info.xml), resolved lazily from the server container, + * instead of the \OCP\Util::getVersion() deprecated since 31. Falls back * to "unknown" when no live Nextcloud server container is present (e.g. a * standalone PHPUnit run without a full NC bootstrap) — that never * happens for the real endpoint/occ command, only in isolated unit tests. @@ -741,7 +740,7 @@ private function buildMetadata(string $tokenSetId, array $tokenSetMeta, string $ */ private function resolveNextcloudVersion(): string { try { - return implode('.', \OCP\Util::getVersion()); + return implode('.', \OCP\Server::get(\OCP\ServerVersion::class)->getVersion()); } catch (\Throwable $e) { return 'unknown'; } diff --git a/lib/Service/PlaygroundStateService.php b/lib/Service/PlaygroundStateService.php index 9aaf15f29..d91a71c14 100644 --- a/lib/Service/PlaygroundStateService.php +++ b/lib/Service/PlaygroundStateService.php @@ -218,10 +218,11 @@ private function getDarkTokens(string $tokenSetId): array { * they take it — and this is the one it opens on, because the version you * are running is the one you are asking about first. * - * Resolved through the deprecated array-returning `\OCP\Util::getVersion()` - * for the same reason `ComplianceReportService` does: a type-hinted - * `\OCP\ServerVersion` constructor dependency is a DI problem on older - * servers, and this value is read once to pick a default. + * Resolved lazily through `\OCP\ServerVersion` (public since NC 31, below + * this app's floor of 32) via the server container rather than a + * constructor dependency, for the same reason `ComplianceReportService` + * does: this value is read once to pick a default. Replaces the + * `\OCP\Util::getVersion()` call deprecated since 31. * * @return int The major version, or 0 when it cannot be read. * @@ -231,7 +232,7 @@ private function getDarkTokens(string $tokenSetId): array { */ protected function getServerMajor(): int { try { - $version = \OCP\Util::getVersion(); + $version = \OCP\Server::get(\OCP\ServerVersion::class)->getVersion(); return (int)($version[0] ?? 0); } catch (Throwable $e) { From 26f945b9d465a77c36be92eb0153c90dc5ee4c15 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 6 Oct 2026 10:32:28 +0000 Subject: [PATCH 3/3] fix(nc35): inject OCP\ServerVersion instead of a global container lookup The hydra phpcs gate rejects \OCP\Server::get() in lib/ (global container lookup), and the extra class reference pushed ComplianceReportService over the phpmd CouplingBetweenObjects limit. ServerVersion is now an optional constructor dependency of ComplianceReportService and PlaygroundStateService (autowired by the container on every supported server, 32+); without it the existing fallbacks ("unknown" / 0) apply, as before in isolated unit tests. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_01M7DxSg7d7wqqZsUppLWf7J --- lib/Service/ComplianceReportService.php | 35 ++++++++++++++++++------- lib/Service/PlaygroundStateService.php | 29 ++++++++++---------- 2 files changed, 40 insertions(+), 24 deletions(-) diff --git a/lib/Service/ComplianceReportService.php b/lib/Service/ComplianceReportService.php index 5366a5963..cddfc6cc2 100644 --- a/lib/Service/ComplianceReportService.php +++ b/lib/Service/ComplianceReportService.php @@ -25,6 +25,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\IConfig; use OCP\IURLGenerator; +use OCP\ServerVersion; /** * Computes the active-configuration WCAG contrast compliance evidence report. @@ -177,6 +178,15 @@ class ComplianceReportService { */ private ITimeFactory $timeFactory; + /** + * The running server's version (public since NC 31, below this app's floor of 32). + * + * Null only when constructed without it (isolated unit tests). + * + * @var ServerVersion|null + */ + private ?ServerVersion $serverVersion; + /** * Constructor. * @@ -189,6 +199,9 @@ class ComplianceReportService { * @param IConfig $config The config service. * @param IURLGenerator $urlGenerator The URL generator. * @param ITimeFactory $timeFactory The injectable clock. + * @param ServerVersion|null $serverVersion The running server's version. + * + * @SuppressWarnings(PHPMD.ExcessiveParameterList) - each collaborator backs one part of the report. */ public function __construct( ContrastService $contrast, @@ -200,6 +213,7 @@ public function __construct( IConfig $config, IURLGenerator $urlGenerator, ITimeFactory $timeFactory, + ?ServerVersion $serverVersion = null, ) { $this->contrast = $contrast; $this->cssParser = $cssParser; @@ -210,6 +224,7 @@ public function __construct( $this->config = $config; $this->urlGenerator = $urlGenerator; $this->timeFactory = $timeFactory; + $this->serverVersion = $serverVersion; }//end __construct() /** @@ -725,22 +740,24 @@ private function buildMetadata(string $tokenSetId, array $tokenSetMeta, string $ /** * Resolve the Nextcloud server version string. * - * Uses \OCP\ServerVersion (public since NC 31.0.0, below this app's floor - * of 32 in appinfo/info.xml), resolved lazily from the server container, - * instead of the \OCP\Util::getVersion() deprecated since 31. Falls back - * to "unknown" when no live Nextcloud server container is present (e.g. a - * standalone PHPUnit run without a full NC bootstrap) — that never - * happens for the real endpoint/occ command, only in isolated unit tests. + * Uses the injected \OCP\ServerVersion (public since NC 31.0.0, below + * this app's floor of 32 in appinfo/info.xml) instead of the + * \OCP\Util::getVersion() deprecated since 31. Falls back to "unknown" + * when it was not injected (e.g. a standalone PHPUnit run without a full + * NC bootstrap) — that never happens for the real endpoint/occ command, + * only in isolated unit tests. * * @return string The Nextcloud version (e.g. "34.0.0.5"), or "unknown". * - * @SuppressWarnings(PHPMD.StaticAccess) - see rationale above. - * * @spec openspec/specs/compliance-evidence/spec.md */ private function resolveNextcloudVersion(): string { + if ($this->serverVersion === null) { + return 'unknown'; + } + try { - return implode('.', \OCP\Server::get(\OCP\ServerVersion::class)->getVersion()); + return implode('.', $this->serverVersion->getVersion()); } catch (\Throwable $e) { return 'unknown'; } diff --git a/lib/Service/PlaygroundStateService.php b/lib/Service/PlaygroundStateService.php index d91a71c14..147c6e038 100644 --- a/lib/Service/PlaygroundStateService.php +++ b/lib/Service/PlaygroundStateService.php @@ -25,6 +25,7 @@ use OCA\Thematiq\Service\RuntimeFile\SetFileReader; use OCP\App\IAppManager; use OCP\IL10N; +use OCP\ServerVersion; use Throwable; /** @@ -92,6 +93,7 @@ class PlaygroundStateService { * @param IL10N $l10n The app's translations. * @param RuntimeFileStore|null $store Where uploaded sets and their dark files live. * @param SetFileReader $files Reads a set file from the release or the store. + * @param ServerVersion|null $serverVersion The running server's version. */ public function __construct( IAppManager $appManager, @@ -101,6 +103,7 @@ public function __construct( IL10N $l10n, private ?RuntimeFileStore $store = null, private SetFileReader $files = new SetFileReader(), + private ?ServerVersion $serverVersion = null, ) { $this->appManager = $appManager; $this->previewValues = $previewValues; @@ -218,29 +221,25 @@ private function getDarkTokens(string $tokenSetId): array { * they take it — and this is the one it opens on, because the version you * are running is the one you are asking about first. * - * Resolved lazily through `\OCP\ServerVersion` (public since NC 31, below - * this app's floor of 32) via the server container rather than a - * constructor dependency, for the same reason `ComplianceReportService` - * does: this value is read once to pick a default. Replaces the - * `\OCP\Util::getVersion()` call deprecated since 31. + * Read from the injected `\OCP\ServerVersion` (public since NC 31, below + * this app's floor of 32), replacing the `\OCP\Util::getVersion()` call + * deprecated since 31. * * @return int The major version, or 0 when it cannot be read. * - * @SuppressWarnings(PHPMD.StaticAccess) - see rationale above. - * * @spec openspec/changes/component-playground/specs/component-playground/spec.md */ protected function getServerMajor(): int { - try { - $version = \OCP\Server::get(\OCP\ServerVersion::class)->getVersion(); + if ($this->serverVersion === null) { + // Only without a full Nextcloud bootstrap, which in practice means + // an isolated unit test. Zero is not a version the switch offers, + // so the instrument falls back to the newest one it knows. + return 0; + } - return (int)($version[0] ?? 0); + try { + return (int)($this->serverVersion->getVersion()[0] ?? 0); } catch (Throwable $e) { - // Only reachable without a full Nextcloud bootstrap, which in - // practice means an isolated unit test. Zero is not a version the - // switch offers, so the instrument falls back to the newest one it - // knows — a working switch either way, and nothing worth a - // dependency on a logger to say. return 0; } }//end getServerMajor()