Skip to content

refactor: resolve app services through injected containers, never the global server - #3532

Merged
rubenvdlinde merged 7 commits into
developmentfrom
fix/service-locators-and-analysis-memory
Sep 8, 2026
Merged

refactor: resolve app services through injected containers, never the global server#3532
rubenvdlinde merged 7 commits into
developmentfrom
fix/service-locators-and-analysis-memory

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Why

On 2026-09-08 an openregister unit test took 19 GB of RAM and 6 GB of swap, twice, and rebooted the developer VM. PR #3523 fixed the bootstrap and four classes. This finishes the job.

Outside a booted Nextcloud, \OC::$server->get() and \OCP\Server::get() autowire this app's services from scratch, and MagicMapper -> SettingsService -> ValidationOperationsHandler -> ValidateObject is a constructor cycle the container has no detection for. Inside a booted server the same call is merely a hidden dependency that no unit test can control.

What changed

Services and controllers now resolve through what they already own. Where the looked-up thing was a collaborator the class could simply hold, it became a constructor argument: the search trail mapper, the magic mapper's register and schema mappers, the organisation controller's user session. Where the lookup has to stay lazy or dynamic, it goes through an injected Psr\Container\ContainerInterface: the eight background jobs, the link services, the relations controller, the grant resolver.

Tests register on a per-test container mock. A new RegistersContainerServices trait replaces registering mocks on the process-wide fake server, which never reset and leaked mocks between tests. BlobMigrationJobTest no longer needs its capture-and-restore dance or its skip, so its cases actually run.

The nine remaining lookups are marked, not hidden. Each carries a phpcs:ignore naming why it stays:

site why it stays
SharesProvider, ShareLinkService (2 methods each) PSR-11 adapter shims that ARE the container handed to a collaborator, so there is nothing to inject into them
DoriathApplicationRegistrar, DoriathCredentialStore optional sibling app (keepiq), guarded by class_exists and try/catch, absent on most instances
AnonymisationBackendService optional AppAPI PublicFunctions, same guards
CospendProvider last-resort fallback, tried only after the injected container
Db\Schema an entity, not a DI-built class; its guarded lookup was already documented

Also: psalm and phpmd get explicit 2G limits in composer.json, and the NoLegacyServerAccessors sniff test runs again. It was skipped for a PHP_CodeSniffer 3.9 bug and pointed at a copy of the sniff this repo no longer carries, so for months it guarded nothing. PHPCS is at 3.13 now.

Two dead branches removed that static analysis flagged: AgentMapper::findByUuid returns a non-nullable Agent and throws on a miss, so the null check after it could never run; IResponse::getHeader returns an array, so its null coalesce could never fire. Neither changes behaviour.

Verified locally

Check Result
phpunit tests/Unit --no-coverage 19,454 tests, exit 0, peak 454 MB, 105 s
phpcs --standard=phpcs.xml on all 47 touched lib files exit 0
fleet sniff (NoServiceLocator) on lib 0 unannotated lookups

The fleet sniff is the one from ConductionNL/.github branch feat/no-service-locator-sniff, which lands after the per-app refactors so no app goes red on arrival.

Not in this PR

lib/AppInfo, lib/AppHost and lib/Migration keep their lookups: they run before or outside DI, and the sniff excludes them by path.

🤖 Generated with Claude Code

Conduction Release Bot and others added 7 commits September 8, 2026 09:31
…cted container

The eight jobs that reached for \OC::$server at run time now take the app
container in their constructor. Their tests register mocks on a per-test
container mock (new RegistersContainerServices trait) instead of on the
process-wide fake server, which never reset and leaked mocks between tests.
BlobMigrationJobTest no longer needs its capture-and-restore dance or its
skip; its twelve cases run.

Also: psalm and phpmd get explicit 2G memory limits in composer.json, and
the NoLegacyServerAccessors sniff test points at the vendored hydra-gates
sniff and runs again (it was skipped for a PHPCS 3.9 bug and aimed at a
file this repo no longer carries).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…their injected collaborators

SearchTrailController takes the SearchTrailMapper in its constructor and the
mapper gains the clearAllLogs() the admin endpoint always called (the method
never existed, so clearAll threw an \Error for an undefined method).
MagicMapper's cross-table lookups use the register and schema mappers it
already has; OrganisationController reads the session it already has. The
tests stop registering mocks on the global server.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…global server

ApiTokenSettingsController takes IClientService, EndpointService the
AgentMapper, ActivityFilterService and the seven MarkerLookupTrait providers a
LoggerInterface, TenantLifecycleService the IUserManager, DeleteObject the
OrganisationMapper and ConfigurationSettingsHandler the IAppManager.
ImportHandler takes an optional IAppManager (twelve test files construct it
positionally); without one the seed-data app dependency check is skipped with
a debug log instead of reaching for the server.

The provider factories in Application.php pass the logger. Tests that
registered mocks on the global server now inject them, and the seams that
were unreachable before (token probes, provisioning the org admin, the active
organisation on delete, version info) get one test each.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…ver resolve through an injected container

RelationsController, AnalyticsLinkService, DeckLinkService,
BookmarkLinkService, DeckCardService and FileVersioningHandler take a
ContainerInterface and resolve the other apps' classes (Deck, Bookmarks,
Files_Versions, Analytics) and the pluggable leaf link services through it,
wrapped exactly as before (class_exists guard, try/catch, null on absence).
DeckCardService also takes the IUserManager it used to fetch by string.

CaseTokenService and ObjectGrantResolver already accepted an optional
container with a global fallback; the container is required now and the
fallbacks (an anonymous PSR-11 shim and a Server::get of the container) are
gone. The tests inject a container mock; the relations controller gets two
tests for the leaf path that used to be unreachable.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Schema is an entity, not DI-built, and resolveIntegrationRegistryIds() runs
from setConfiguration(), which every mapper calls while hydrating a row.
Threading the registry ids through those thirty call sites is a change of
its own, so the lookup stays behind its isset() guard, with a targeted
phpcs:ignore and a docblock that says so. It is the one remaining site in
lib/ outside AppInfo, AppHost and Migration.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…rvices

Closes the openregister half of the 2026-09-08 memory incident. Every remaining
\OC::$server->get() / \OCP\Server::get() in lib now carries a phpcs:ignore
naming why it stays, so the new fleet sniff can land as an error:

- two PSR-11 adapter shims that ARE the container being handed to a
  collaborator, so there is nothing to inject into them
- three probes for optional sibling apps (keepiq credential services, AppAPI
  PublicFunctions), each guarded by class_exists and try/catch
- one provider fallback, tried only after the injected container
- Db\Schema, an entity rather than a DI-built class, which already documented
  its guarded lookup

Also wires the container and logger arguments the earlier commits' constructors
now require, and drops two dead branches static analysis had flagged: AgentMapper
findByUuid returns a non-nullable Agent and throws, so its null check could never
run, and IResponse::getHeader returns an array, so its null coalesce could never
fire. Neither changes behaviour.

Verified locally: 19,454 unit tests exit 0, peak 454 MB; phpcs exit 0 across all
47 touched lib files; the fleet sniff reports 0 unannotated lookups in lib.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@rubenvdlinde
rubenvdlinde merged commit ed51756 into development Sep 8, 2026
34 of 37 checks passed
@rubenvdlinde
rubenvdlinde deleted the fix/service-locators-and-analysis-memory branch September 8, 2026 08:54
@github-actions

github-actions Bot commented Sep 8, 2026

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/openregister @ 90e1994

Check PHP Vue Security License Tests
lint
phpcs
phpmd
psalm
phpstan
phpmetrics
eslint
stylelint
build
check-specs
test-l10n
test-l10n-parity
format
check-schema-l10n
check-l10n-js
composer ✅ 174/174
npm ✅ 543/543
app:check-code ⏭️
info.xml
REUSE
PHPUnit
Newman
Playwright ⏭️ deferred — runs on the promotion into beta/main, not on a pull request into development
Hydra gates

Quality workflow — 2026-09-08 09:27 UTC

Download the full PDF report from the workflow artifacts.

rubenvdlinde added a commit that referenced this pull request Sep 8, 2026
…oduced (#3534)

The development push run for #3532 went red on psalm and phpmd. Both are mine,
and both were invisible to that PR's own checks because it ran psalm and phpmd
per touched file rather than over the tree.

- psalm: five errors. Four are UnusedBaselineEntry, twenty baselined UnusedParam
  entries the refactor made unnecessary because the parameters are used now.
  Removed. The fifth is OC_App::loadApp() in ImportHandler, a server internal
  psalm cannot see: it was unreachable before, and injecting IAppManager made
  psalm analyse the branch. Suppressed on the method docblock with the reason.
  UnusedBaselineEntry only ever appears on a FULL psalm run, never on a per-file
  one, which is why the PR's checks were green.
- phpmd: SearchTrailMapper crossed the 1000-line ExcessiveClassLength threshold
  at 1015 when it gained the clearAllLogs() the admin endpoint had always called
  and that never existed. Suppressed with that reason, the idiom 74 other classes
  in this repo already use.

Verified: psalm full run exit 0, both phpmd rulesets exit 0, phpcs exit 0 on both
touched files, 19,454 unit tests exit 0 at peak 454 MB.

Co-authored-by: Conduction Release Bot <release-bot@conduction.nl>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant