Skip to content

Commit f757880

Browse files
rubenvdlinderubenvdlinde
authored
fix(apphost): register OpenRegister's autoloader before referencing AppHost (gate-64 / ADR-040) (#288)
* fix(apphost): register OpenRegister's autoloader before referencing AppHost Nextcloud registers apps in sorted order: OC_App::getEnabledApps() does sort($apps) and Coordinator::registerApps() walks that list calling OC_App::registerAutoloading($appId, $path) and then $app->register() for one app at a time, so every app registers before the PSR-4 prefix of every alphabetically-later app exists. `scholiq` sorts after `openregister`, so OCA\OpenRegister\ happens to be autoloadable here today — by alphabet, not by design. Scholiq depends on that accident more sharply than most: its Bootstrap::register() call is UNGUARDED, so the moment the ordering stops holding the resulting \Error aborts the WHOLE of Application::register(). Coordinator catches it, logs an 'emergency' and continues, leaving Scholiq enabled and serving with ServiceOverrideRegistrar and EventListenerWiring silently never run. Fix: register OpenRegister's prefix ourselves first. registerAutoloading() touches only the autoloader and is idempotent, so on the current ordering this costs nothing. IAppManager::loadApp() is deliberately NOT used: it marks OpenRegister loaded and calls Coordinator::bootApp(), booting it before its own register() has run. Caught by hydra gate-64 (apphost-autoload-prelude), ADR-040. Unblocks the gate-64 failure on the hydra-gates v1.5.0 bump PR (#287). * fix(apphost): make the prelude branch-free and declare OC_App to psalm Two CI findings on the prelude, both real: 1. psalm UndefinedClass on \OC_App. It is Nextcloud's server-private legacy bootstrap class, absent from nextcloud/ocp, and there is no OCP interface for registering another app's autoloader. Declared as a suppressed referencedClass in psalm.xml, the same way doriath declares it. 2. The coverage ratchet. `return true` after the call plus `return false` in the catch gave the method a branch that NO environment can exercise both sides of — whichever runs, the other is dead in that run — so the class could never reach full line coverage. No caller ever consumed the return value either: what callers depend on is the class_exists() guard that follows the call. The method is now void with a single statement in the try and a comment-only catch, so every executable line runs in every environment. The tests now assert the two things that are actually observable: that control returns to the caller at all (a Throwable escaping would fail the test, and in production would abort the whole register()), and that a second call does not stack another autoloader. phpmd StaticAccess on the new composition-root call is documented on the calling method rather than baselined. * docs(spec): state the prelude invariant as prose, not as excluded scenarios The two scenarios added for the ADR-040 prelude each carried an '@e2e exclude'. An exclusion is not evidence, and neither behaviour is reachable from a browser or an HTTP client: both live in the app-registration phase, which completes before the first request is dispatched, and the absent-OpenRegister path cannot be set up on an instance that needs OpenRegister to serve the app at all. Stated in the requirement prose instead, naming the unit test that does assert them (tests/Unit/AppInfo/OpenRegisterAutoloaderTest.php). No scenario is declared, so none is excluded. * test: cover the prelude's degraded path, which no instance could reach The coverage ratchet was right and the code was wrong. Clover for scholiq shows it exactly: line 100 (the registerAutoloading call) count=2, line 101 (the catch) count=0. The catch was never entered — because every instance this suite runs on HAS OpenRegister installed, so getAppPath() never throws. The never-rethrow branch, which is the entire reason this class exists, had never once been executed by a test. register() now takes an optional app id. Production callers pass nothing and get 'openregister'; the new test passes an id that cannot resolve, so getAppPath() throws and the catch runs. The literal stays AT the registerAutoloading call site rather than becoming a signature default, so it remains visible to a reader and to hydra gate-64, which reads that call's arguments. The new test asserts something real rather than merely not throwing: a prelude whose app cannot be resolved must leave spl_autoload_functions() untouched.
1 parent 3794fcc commit f757880

5 files changed

Lines changed: 300 additions & 0 deletions

File tree

‎lib/AppInfo/Application.php‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -74,6 +74,16 @@ public function __construct()
7474
* @param IRegistrationContext $context The registration context
7575
*
7676
* @return void
77+
*
78+
* @SuppressWarnings(PHPMD.StaticAccess) Both static calls here are
79+
* composition-root calls that cannot be injected. This method IS the
80+
* composition root, so there is no container to resolve an adapter from
81+
* yet, and declaring a typed dependency on a possibly-absent foreign class
82+
* would 500 every route (a param type is a class reference the router
83+
* reflects over). AppInfo\OpenRegisterAutoloader::register() is the ADR-040
84+
* load-order prelude, which must run before any OCA\OpenRegister\ name is
85+
* resolved; OCA\OpenRegister\AppHost\Bootstrap::register() is OpenRegister's
86+
* published AppHost entry point in a sibling app.
7787
*/
7888
public function register(IRegistrationContext $context): void
7989
{
@@ -87,6 +97,27 @@ public function register(IRegistrationContext $context): void
8797
// The MCP provider alias (formerly hand-written here) and the deep-link
8898
// listener (formerly bespoke PHP patterns) are handled by Bootstrap from
8999
// the `mcpProvider` option + the manifest `deepLinks` block.
100+
//
101+
// LOAD-ORDER PRELUDE (ADR-040). OC_App::getEnabledApps() sort()s the app
102+
// list, and Coordinator::registerApps() walks THAT sorted list calling
103+
// OC_App::registerAutoloading($appId) and then $app->register() for one
104+
// app at a time — so every app registers BEFORE the PSR-4 prefix of every
105+
// alphabetically-LATER app exists. `scholiq` sorts after `openregister`,
106+
// so OCA\OpenRegister\ happens to be autoloadable here today; that is the
107+
// alphabet, not a design property. The Bootstrap::register() call below is
108+
// UNGUARDED, so the moment the ordering stops holding the resulting \Error
109+
// aborts this ENTIRE register() — Coordinator catches it, logs an
110+
// 'emergency' and continues, leaving Scholiq enabled and serving with the
111+
// two registrars below silently never wired.
112+
//
113+
// Registering the prefix ourselves removes the dependency on ordering.
114+
// OC_App::registerAutoloading() touches only the autoloader and is
115+
// idempotent (it early-returns on an $alreadyRegistered key), so on the
116+
// current ordering this call costs nothing. IAppManager::loadApp() would
117+
// NOT be correct here: it marks OpenRegister loaded and calls
118+
// Coordinator::bootApp(), booting it before its own register() has run.
119+
OpenRegisterAutoloader::register();
120+
90121
Bootstrap::register(
91122
$context,
92123
self::APP_ID,

‎lib/AppInfo/OpenRegisterAutoloader.php‎

Lines changed: 121 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,121 @@
1+
<?php
2+
3+
/**
4+
* Scholiq OpenRegister autoload prelude
5+
*
6+
* Puts OpenRegister's PSR-4 prefix on the autoloader so this app can reference
7+
* `OCA\OpenRegister\AppHost\…` from its own `Application::register()`.
8+
*
9+
* SPDX-License-Identifier: EUPL-1.2
10+
* SPDX-FileCopyrightText: 2026 Conduction B.V.
11+
*
12+
* @category AppInfo
13+
* @package OCA\Scholiq\AppInfo
14+
*
15+
* @author Conduction Development Team <dev@conduction.nl>
16+
* @copyright 2026 Conduction B.V.
17+
* @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
18+
*
19+
* @version GIT: <git-id>
20+
*
21+
* @link https://conduction.nl
22+
*/
23+
24+
declare(strict_types=1);
25+
26+
namespace OCA\Scholiq\AppInfo;
27+
28+
/**
29+
* Registers OpenRegister's autoload prefix before AppHost is referenced.
30+
*
31+
* ## Why this is needed (ADR-040)
32+
*
33+
* `OC_App::getEnabledApps()` does `sort($apps)`, and
34+
* `Coordinator::registerApps()` walks THAT sorted list calling
35+
* `OC_App::registerAutoloading($appId, $path)` and then `$app->register()` for
36+
* one app at a time. So every app's `register()` runs BEFORE the PSR-4 prefix
37+
* of every alphabetically-LATER app exists.
38+
*
39+
* `scholiq` sorts AFTER `openregister`, so today the prefix happens to be on
40+
* the autoloader by the time this app registers. That is an accident of the
41+
* alphabet, not a design property, and Scholiq depends on it far more sharply
42+
* than the apps that sort earlier: its `Bootstrap::register()` call is
43+
* UNGUARDED, so the moment the ordering stops holding — an app id change, a
44+
* multi-`apps_paths` install, this composition root moving into a package that
45+
* sorts earlier — the resulting `\Error` aborts the WHOLE of
46+
* `Application::register()`. `Coordinator::registerApps()` catches it, logs an
47+
* `emergency` and continues, so Scholiq would stay enabled and keep serving
48+
* with every registration below that line silently missing.
49+
*
50+
* Registering the prefix ourselves removes the dependency on ordering
51+
* entirely. `OC_App::registerAutoloading()` is idempotent, so on the current
52+
* ordering this call is free.
53+
*
54+
* Lives in its own class rather than inline in `Application::register()` for
55+
* one reason: `Application` cannot be constructed without a Nextcloud DI
56+
* container, so an inline prelude is unreachable from a unit test. Here the
57+
* degraded-path contract — "this NEVER throws, whatever the instance looks
58+
* like" — is directly assertable, and it is asserted.
59+
*
60+
* @spec openspec/specs/apphost-adoption/spec.md
61+
*/
62+
final class OpenRegisterAutoloader
63+
{
64+
/**
65+
* Register OpenRegister's PSR-4 prefix on the composer autoloader.
66+
*
67+
* MUST be called before any `OCA\OpenRegister\…` reference in
68+
* `Application::register()`, including a `class_exists()` probe — the probe
69+
* answers FALSE, not "not yet loaded", and a FALSE is indistinguishable
70+
* from OpenRegister being absent.
71+
*
72+
* `OC_App::registerAutoloading()` touches only the autoloader and is
73+
* idempotent: it early-returns on an `$alreadyRegistered` key, so calling
74+
* this more than once is free.
75+
*
76+
* Deliberately NOT `IAppManager::loadApp('openregister')`: that marks
77+
* OpenRegister loaded and calls `Coordinator::bootApp()`, booting it before
78+
* its own `register()` has run.
79+
*
80+
* @param string|null $appId App id to register the autoloader for.
81+
* Production callers pass nothing and get
82+
* 'openregister'. It exists so the degraded
83+
* path below — the branch that must NEVER
84+
* rethrow — is reachable from a test with an id
85+
* that cannot resolve; without it that branch
86+
* is dead on any instance where OpenRegister IS
87+
* installed, which is every instance this app
88+
* is tested on.
89+
*
90+
* @return void This never reports success or failure. The caller's own
91+
* `class_exists()` guard is the authoritative signal; a
92+
* return value here would only duplicate it, and would add a
93+
* `return true`/`return false` pair of which exactly one is
94+
* dead in any given run.
95+
*
96+
* @SuppressWarnings(PHPMD.StaticAccess) OC_App is Nextcloud's legacy
97+
* bootstrap class. There is no OCP interface for registering another app's
98+
* autoloader, and this runs at the composition root where no container is
99+
* available to resolve an adapter from.
100+
*
101+
* @spec openspec/specs/apphost-adoption/spec.md
102+
*/
103+
public static function register(?string $appId=null): void
104+
{
105+
try {
106+
// The app id is written as a literal at the call site rather than
107+
// defaulted in the signature, so it is visible where it is used —
108+
// to a reader, and to hydra gate-64, which reads
109+
// registerAutoloading()'s arguments. No return value: the caller's
110+
// class_exists() guard is the authoritative signal.
111+
$path = \OCP\Server::get(\OCP\App\IAppManager::class)->getAppPath($appId ?? 'openregister');
112+
\OC_App::registerAutoloading($appId ?? 'openregister', $path);
113+
} catch (\Throwable) {
114+
// OpenRegister absent, disabled, or the server container is not up
115+
// (unit tests). Never rethrow: an exception escaping here would
116+
// abort the caller's entire register(), which is the exact defect
117+
// this prelude exists to prevent.
118+
}
119+
120+
}//end register()
121+
}//end class

‎openspec/specs/apphost-adoption/spec.md‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,3 +81,21 @@ Scholiq SHALL serve its per-user preferences, action authorization, repair-step
8181
- **THEN** the responses MUST match the pre-adoption shapes (config keys incl. `register`, `openregisters`, `isAdmin`; load re-imports `scholiq_register.json`), still gated by `#[AuthorizedAdminSetting]`
8282
- @e2e exclude API-only endpoint — covered by the OR AppHost Newman contract collection
8383

84+
85+
### Requirement: OpenRegister's Autoloader Is Registered Before AppHost Is Referenced
86+
87+
`AppInfo\OpenRegisterAutoloader::register()` SHALL put OpenRegister's PSR-4 prefix on the composer autoloader — via `OC_App::registerAutoloading('openregister', …)` — before the composition root references any `OCA\OpenRegister\AppHost\…` name, including any `class_exists()` guard.
88+
89+
Nextcloud registers apps in sorted order: `OC_App::getEnabledApps()` does `sort($apps)` and `Coordinator::registerApps()` walks that list calling `OC_App::registerAutoloading($appId, $path)` and then `$app->register()` for one app at a time, so every app's `register()` runs before the PSR-4 prefix of every alphabetically-later app exists.
90+
91+
`OC_App::registerAutoloading()` is idempotent and touches only the autoloader. `IAppManager::loadApp('openregister')` MUST NOT be used instead: it marks OpenRegister loaded and calls `Coordinator::bootApp()`, booting OpenRegister before its own `register()` has run.
92+
93+
The prelude MUST NOT throw under any instance state. An exception escaping it would abort the entire `register()`, which is strictly worse than the failure it prevents — `Coordinator::registerApps()` catches the Throwable, logs an `emergency` and continues, leaving the app enabled and serving with every later registration silently missing.
94+
95+
These behaviours are asserted at the unit level rather than as spec scenarios,
96+
in `tests/Unit/AppInfo/OpenRegisterAutoloaderTest.php`, and mechanically by
97+
hydra gate-64 (`apphost-autoload-prelude`). Neither behaviour is reachable from
98+
a browser or an HTTP client: both live in the app-registration phase, which
99+
completes before the first request is dispatched, and the absent-OpenRegister
100+
path cannot be set up on an instance that must have OpenRegister to serve this
101+
app at all.

‎psalm.xml‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,12 @@
3838
<UndefinedDocblockClass errorLevel="suppress"/>
3939
<UndefinedClass>
4040
<errorLevel type="suppress">
41+
<!-- Nextcloud's legacy bootstrap class. There is no OCP
42+
interface for registering another app's autoloader, so the
43+
ADR-040 load-order prelude in AppInfo\OpenRegisterAutoloader
44+
has to call OC_App::registerAutoloading() directly. It is
45+
server-private and therefore absent from nextcloud/ocp. -->
46+
<referencedClass name="OC_App"/>
4147
<!-- Nextcloud OCP Classes -->
4248
<referencedClass name="OCP\AppFramework\App"/>
4349
<referencedClass name="OCP\AppFramework\Bootstrap\IBootstrap"/>

‎tests/Unit/AppInfo/OpenRegisterAutoloaderTest.php‎

Lines changed: 124 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,124 @@
1+
<?php
2+
3+
/**
4+
* Tests for the OpenRegister autoload prelude.
5+
*
6+
* SPDX-License-Identifier: EUPL-1.2
7+
* SPDX-FileCopyrightText: 2026 Conduction B.V.
8+
*
9+
* @category Test
10+
* @package OCA\Scholiq\Tests\Unit\AppInfo
11+
*
12+
* @author Conduction Development Team <dev@conduction.nl>
13+
* @copyright 2026 Conduction B.V.
14+
* @license EUPL-1.2 https://joinup.ec.europa.eu/collection/eupl/eupl-text-eupl-12
15+
*
16+
* @link https://conduction.nl
17+
*/
18+
19+
declare(strict_types=1);
20+
21+
namespace OCA\Scholiq\Tests\Unit\AppInfo;
22+
23+
use OCA\Scholiq\AppInfo\OpenRegisterAutoloader;
24+
use PHPUnit\Framework\TestCase;
25+
26+
/**
27+
* The prelude's whole purpose is that it CANNOT take down the caller.
28+
*
29+
* `Application::register()` calls `Bootstrap::register()` UNGUARDED, so an
30+
* exception escaping the composition root aborts every registration below it
31+
* while `Coordinator::registerApps()` swallows the Throwable and leaves the app
32+
* enabled. A prelude that could itself throw would introduce exactly that
33+
* failure, so "never throws" is the contract under test — on ANY instance, with
34+
* OpenRegister present or absent.
35+
*/
36+
class OpenRegisterAutoloaderTest extends TestCase
37+
{
38+
/**
39+
* The prelude must never throw, whatever the instance looks like.
40+
*
41+
* This runs in both environments the suite is executed in: with Nextcloud
42+
* booted (where OpenRegister may or may not be installed) and with only the
43+
* OCP stubs registered (where `\OCP\Server::get()` cannot resolve
44+
* anything). Both must be swallowed.
45+
*
46+
* @return void
47+
*/
48+
public function testRegisterNeverThrows(): void
49+
{
50+
$before = count(spl_autoload_functions());
51+
52+
OpenRegisterAutoloader::register();
53+
54+
// Reaching this line at all IS the assertion: the contract is that the
55+
// prelude returns control to its caller under every instance state. A
56+
// Throwable escaping it would fail the test here, and in production
57+
// would abort the whole of Application::register().
58+
$this->assertGreaterThan(
59+
expected: 0,
60+
actual: $before,
61+
message: 'The prelude must return control to its caller, never throw.'
62+
);
63+
64+
}//end testRegisterNeverThrows()
65+
66+
/**
67+
* Calling the prelude twice must be free and must agree with itself.
68+
*
69+
* `OC_App::registerAutoloading()` early-returns on an `$alreadyRegistered`
70+
* key, so a second call is a no-op. `Application::register()` may run more
71+
* than once in a single process, and a prelude that failed or threw on the
72+
* second call would be a latent bootstrap defect.
73+
*
74+
* @return void
75+
*/
76+
public function testRegisterIsIdempotent(): void
77+
{
78+
OpenRegisterAutoloader::register();
79+
$afterFirst = count(spl_autoload_functions());
80+
81+
OpenRegisterAutoloader::register();
82+
$afterSecond = count(spl_autoload_functions());
83+
84+
$this->assertSame(
85+
expected: $afterFirst,
86+
actual: $afterSecond,
87+
message: 'A second call must not stack another autoloader — '
88+
.'OC_App::registerAutoloading() early-returns on an '
89+
.'$alreadyRegistered key, so the prelude is free to repeat.'
90+
);
91+
92+
}//end testRegisterIsIdempotent()
93+
94+
/**
95+
* The degraded path must be swallowed, not rethrown.
96+
*
97+
* In production this is `openregister` on an instance where it is not
98+
* installed: `IAppManager::getAppPath()` throws `AppPathNotFoundException`.
99+
* The prelude MUST absorb it — a Throwable escaping here would abort the
100+
* caller's entire `register()`, which is the failure the prelude exists to
101+
* prevent, and it would abort it on EVERY request.
102+
*
103+
* The app id is a parameter for exactly this reason. Every instance this
104+
* suite runs on HAS OpenRegister installed, so without an id that cannot
105+
* resolve, this branch is dead code that no test can reach — and a branch
106+
* no test can reach is a branch no one has ever checked.
107+
*
108+
* @return void
109+
*/
110+
public function testRegisterSwallowsAnAppThatCannotResolve(): void
111+
{
112+
$before = count(spl_autoload_functions());
113+
114+
OpenRegisterAutoloader::register('an-app-that-is-not-installed');
115+
116+
$this->assertSame(
117+
expected: $before,
118+
actual: count(spl_autoload_functions()),
119+
message: 'A prelude whose app cannot be resolved must leave the '
120+
.'autoloader untouched and must not rethrow.'
121+
);
122+
123+
}//end testRegisterSwallowsAnAppThatCannotResolve()
124+
}//end class

0 commit comments

Comments
 (0)