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/lib/Service/ComplianceReportService.php b/lib/Service/ComplianceReportService.php index 4747cedd6..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,23 +740,24 @@ 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 - * 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\Util::getVersion()); + return implode('.', $this->serverVersion->getVersion()); } catch (\Throwable $e) { return 'unknown'; } diff --git a/lib/Service/PlaygroundStateService.php b/lib/Service/PlaygroundStateService.php index 9aaf15f29..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,28 +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 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. + * 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\Util::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() 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')