fix(rbac): break the MagicMapper ↔ MemberOfResolver construction cycle by WilcoLouwerse · Pull Request #4785 · ConductionNL/openregister

WilcoLouwerse · 2026-10-22T17:15:23Z

Why

Since #4741, occ app:enable openregister dies with Maximum call stack size … Infinite recursion? in the DI container: the MagicMapper factory builds its RBAC handler, which resolves ConditionMatcher, which autowires MemberOfResolver, whose constructor asked for MagicMapper again. Every open openregister PR fails PHPUnit (stable35) and Newman on it, because the job cannot enable the app.

What

MemberOfResolver takes the container instead of MagicMapper and fetches MagicMapper on first read, inside the existing try/catch (a failure still denies). RBAC behaviour is unchanged.
Regression tests in MemberOfResolverTest: construction resolves nothing from the container, the constructor has no MagicMapper parameter, and a container that cannot give the mapper admits nobody.
The tests that construct the resolver are updated, including MatrixRowConditionsBothPathsTest, which arrived on development after this branch.

Tests

MemberOfResolverTest OK (17 tests), ConditionMatcherMemberOfTest OK (8 tests).
MagicRbacMemberOfListTest OK (4), MatrixRowConditionsBothPathsTest OK (4), MatrixRowsFromAppsBothPathsTest OK (3), run in a PHP 8.3 container with pdo_sqlite.
Moving the lazy get out of read()'s try turns the new container-failure test red.
phpstan and phpcs: 0 errors on the changed file.
Not run locally: occ app:enable end to end; this PR's PHPUnit and Newman jobs are that check.

🤖 Generated with Claude Code

openregister#4741 gave ConditionMatcher an autowired MemberOfResolver, whose constructor took MagicMapper. MagicMapper's constructor builds its RBAC handler, which resolves ConditionMatcher from the container, so building MagicMapper re-entered the MagicMapper factory until the stack was exhausted ("Maximum call stack size ... Infinite recursion?"), and `occ app:enable openregister` failed. MemberOfResolver now takes the ContainerInterface and fetches MagicMapper on first read, the same lazy-resolution pattern used for CacheHandler (issue #1564). RBAC behaviour is unchanged; a failed resolution still falls inside the existing try/catch and denies. Tests: constructors updated; regression tests assert that construction never touches the container and that no constructor parameter is a MagicMapper. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-22T17:25:26Z

Quality Report — ConductionNL/openregister @ `7e7c942`

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		✅
composer			✅	✅ 173/173
npm			✅	✅ 648/648
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					❌ 26878/26879 · 25 skipped
Newman					✅ 714/714
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					❌

Quality workflow — 2026-10-22 17:25 UTC

Download the full PDF report from the workflow artifacts.

FlowDryRunService::dryRun (#4753) called ObjectService::find() without a decision, so RecentLensLookupCallSitesTest::testEveryInternalFindHasADecision fails on development. A dry run reads the object to try the flow; nobody opens it, so it runs under WriteCause::asLookup: the audit row stays, the object does not enter the caller's recently-opened list. The test only became visible here because the construction cycle fixed in this PR stopped PHPUnit from running at all. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…emberof-resolver # Conflicts: # lib/Service/Flow/FlowDryRunService.php

github-actions · 2026-10-22T22:57:13Z

Quality Report — ConductionNL/openregister @ `1cc0155`

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		✅
composer			✅	✅ 173/173
npm			❌	✅ 648/648
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					❌ cancelled — no verdict
Newman					✅ 714/714
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					❌

Quality workflow — 2026-10-22 22:57 UTC

Download the full PDF report from the workflow artifacts.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-26T12:26:12Z

Quality Report — ConductionNL/openregister @ `dc3314a`

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		❌
composer			✅	✅ 173/173
npm			❌	✅ 648/648
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					❌ 27153/27160 · 25 skipped
Newman					✅ 714/714
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					❌

Quality workflow — 2026-10-26 12:26 UTC

Download the full PDF report from the workflow artifacts.

MatrixRowConditionsBothPathsTest arrived on development after this branch and built MemberOfResolver with a MagicMapper. The resolver now takes the container and reads the mapper from it on first use, so the harness's container hands the mapper out next to the services it already serves. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-26T13:34:20Z

Quality Report — ConductionNL/openregister @ `fd1d032`

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		❌
composer			✅	✅ 173/173
npm			❌	✅ 648/648
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					❌ 27187/27188 · 25 skipped
Newman					✅ 714/714
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					❌

Quality workflow — 2026-10-26 13:34 UTC

Download the full PDF report from the workflow artifacts.

The lazy container->get(MagicMapper::class) sits inside read()'s try, so a container failure denies like a failing search. A test now reaches that branch; moving the get out of the try turns it red. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

github-actions · 2026-10-26T15:13:41Z

Quality Report — ConductionNL/openregister @ `74da7f5`

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		❌
composer			✅	✅ 173/173
npm			❌	✅ 648/648
app:check-code	⏭️
info.xml	✅
REUSE	✅
lockfile sync		✅
PHPUnit					❌ 27206/27207 · 25 skipped
Newman					✅ 714/714
Playwright					⏭️ deferred: E2E runs locally and on the promotion path only. This pull request targets `development`, so the suite is asked once per promotion into `beta` and `main` rather than once per push per open pull request. Run it locally with npx playwright test, or from the Actions tab on a branch with no open pull request into `development`.
Hydra gates					❌

Quality workflow — 2026-10-26 15:13 UTC

Download the full PDF report from the workflow artifacts.

WilcoLouwerse

Verdict: APPROVE (Strict · full)

Posted as COMMENT: GitHub refuses an approval on your own pull request.

MemberOfResolver takes the container and reads MagicMapper on first use, inside read()'s existing try/catch, so constructing it no longer recurses through MagicMapper → ConditionMatcher → MemberOfResolver. Every construction site passes a container, and a container that cannot give the mapper denies like a failing search. Nothing blocks.

✅ Fixed before posting (2)

Two tests that arrived on development after this branch (MatrixRowConditionsBothPathsTest, borrowed by MatrixRowsFromAppsBothPathsTest) still built the resolver with a mapper: 7 TypeErrors on the previous head. Fixed in 9b120d0.
🟢 No test reached the branch where container->get(MagicMapper::class) itself fails. testAContainerThatCannotGiveTheMapperGivesAnEmptySet does now, and moving the get out of the try turns it red. Fixed in 1a06189.

✅ Verified clean

No other path back to MagicMapper: a constructor-graph walk from every service MagicMapper::initializeHandlers() fetches finds the one known cycle on development and none on this head.
read()'s fallback is fail-closed ([], the caller is a member of nothing).
The suite on the previous head ran 26,914 tests with no error or failure.

CI on this head

Not from this PR, inherited from development: Frontend Check (format) (a spec file), Security (npm) (dompurify, fixed by #4791) and Newman.
PHPUnit was still running when this was posted. On the old head it was canceled at the 45-minute limit while it ran the coverage ratchet's second pass on the merge base, which still has the cycle.

Gates: quality / Hydra Gates was running on this head when this was posted; on development it fails on gates 6, 7 and 49, which #4786 clears.
Reviewed: 5 files, 1a06189.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

… reads #4910 made getSearchIndexStatus() read every table's index names in one indexNamesFor() call; the coverage test still stubbed indexesFor(), so the mock answered nothing and tableCount came back 0. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

WilcoLouwerse mentioned this pull request Oct 22, 2026

fix(objects): object slugs are unique per register and schema and frozen once set #4774

Closed

WilcoLouwerse and others added 2 commits October 23, 2026 00:06

Merge remote-tracking branch 'origin/development' into fix/di-cycle-m…

94ddcab

…emberof-resolver # Conflicts: # lib/Service/Flow/FlowDryRunService.php

WilcoLouwerse mentioned this pull request Oct 23, 2026

As a schema designer I want object slugs to be configurable and unique so that they can be used for stable links #4037

Open

7 tasks

Merge origin/development into fix/di-cycle-memberof-resolver

68851c3

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

WilcoLouwerse commented Oct 26, 2026

View reviewed changes

This was referenced Oct 26, 2026

fix(deps): bump dompurify to 3.4.17 for the critical XSS advisories #4791

Merged

fix(deps): bump dompurify to 3.4.17 for the critical XSS advisories ConductionNL/opencatalogi#1994

Merged

WilcoLouwerse and others added 2 commits October 26, 2026 16:42

Merge origin/development into fix/di-cycle-memberof-resolver

e1cf886

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

WilcoLouwerse merged commit c2f4077 into development Oct 26, 2026
43 of 49 checks passed

WilcoLouwerse deleted the fix/di-cycle-memberof-resolver branch October 26, 2026 16:45

Navigation Menu

Repository navigation

fix(rbac): break the MagicMapper ↔ MemberOfResolver construction cycle - #4785

fix(rbac): break the MagicMapper ↔ MemberOfResolver construction cycle#4785
WilcoLouwerse merged 8 commits into
developmentfrom
fix/di-cycle-memberof-resolver

WilcoLouwerse commented Oct 22, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Oct 22, 2026

Uh oh!

github-actions Bot commented Oct 22, 2026

Uh oh!

github-actions Bot commented Oct 26, 2026

Uh oh!

github-actions Bot commented Oct 26, 2026

Uh oh!

github-actions Bot commented Oct 26, 2026

Uh oh!

WilcoLouwerse left a comment

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

WilcoLouwerse commented Oct 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What

Tests

Uh oh!

github-actions Bot commented Oct 22, 2026

Quality Report — ConductionNL/openregister @ 7e7c942

Uh oh!

github-actions Bot commented Oct 22, 2026

Quality Report — ConductionNL/openregister @ 1cc0155

Uh oh!

github-actions Bot commented Oct 26, 2026

Quality Report — ConductionNL/openregister @ dc3314a

Uh oh!

github-actions Bot commented Oct 26, 2026

Quality Report — ConductionNL/openregister @ fd1d032

Uh oh!

github-actions Bot commented Oct 26, 2026

Quality Report — ConductionNL/openregister @ 74da7f5

Uh oh!

WilcoLouwerse left a comment

Choose a reason for hiding this comment

Verdict: APPROVE (Strict · full)

✅ Fixed before posting (2)

✅ Verified clean

CI on this head

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

WilcoLouwerse commented Oct 22, 2026 •

edited

Loading

Quality Report — ConductionNL/openregister @ `7e7c942`

Quality Report — ConductionNL/openregister @ `1cc0155`

Quality Report — ConductionNL/openregister @ `dc3314a`

Quality Report — ConductionNL/openregister @ `fd1d032`

Quality Report — ConductionNL/openregister @ `74da7f5`