Skip to content

fix(routes): two entries shared a route name, so two routes never registered - #2085

Merged
rubenvdlinde merged 1 commit into
developmentfrom
routes/unique-names
Sep 19, 2026
Merged

rubenvdlinde merged 1 commit into
developmentfrom
routes/unique-names

Conversation

@rubenvdlinde

Copy link
Copy Markdown
Contributor

Two declared routes were never registered

Nextcloud names a route strtolower($appName.'.'.$controller.'.'.$action.$postfix)
and RouteCollection::add() overwrites an entry that already carries that name.
The URL and the verb are not part of the name. So two entries pointing at the
same controller action with no postfix are one route, and the last one
declared is the one that survives. Nothing warns.

Measured with Nextcloud's own RouteParser::parseDefaultRoutes() over this
file, not with a regex:

before: declared=261 registered=259 lost=2
after:  declared=261 registered=261 lost=0
entry what happened
GET /api/lti/{deployment}/login overwritten by the POST beside it
GET / overwritten by the SPA catch-all GET /{path}

LTI 1.3 third-party login initiation is specified for both verbs and the
Platform picks. A Platform that picked GET got a 405 from an endpoint this app
advertises.

The ui#dashboard pair carried a comment describing the overwrite as intended
last-wins behaviour. That is what a lost route looks like once someone has
explained it to themselves.

The postfix goes on the entry that is not referenced

This is the one way the fix can do harm, so it was checked before pushing.
integriq.ui.dashboard is resolved by name in two places:

  • appinfo/info.xml navigation
  • lib/Flow/SynchronizationLogActions.php:94, as
    linkToRoute('integriq.ui.dashboard', ['path' => ''])

Today that name resolves to the catch-all, which takes path as a path
segment. So the catch-all keeps the bare name and the / entry takes
postfix => 'Index'. Renaming the other way round would have turned that
caller's path into a query string. lti#login is referenced nowhere, so the
GET half takes postfix => 'Get' and the currently reachable POST keeps its
name.

No URL and no verb changes in this PR.

The test, and what reddens it

tests/Unit/AppInfo/RouteNameUniquenessTest asserts on the registration key of
each entry, not on the file parsing or the array being non-empty. Two tests:
no two entries may share a key, and the two lost routes are named by URL and
verb so a later edit cannot keep the count and lose them again.

Mutation-checked against the old routes.php, and both assertions reddened,
not a setup line:

1) testEveryDeclaredRouteRegistersUnderItsOwnName
Two 'routes' entries register as 'lti#login', so Nextcloud keeps only the last one.
  kept:        POST /api/lti/{deployment}/login
  OVERWRITTEN: GET /api/lti/{deployment}/login
Failed asserting that an array does not have the key 'lti#login'.
  RouteNameUniquenessTest.php:92

2) testTheTwoRoutesThatWereLostAreRoutedAgain
the GET half of LTI login initiation must keep its own route name
Failed asserting that null is identical to 'GET /api/lti/{deployment}/login'.
  RouteNameUniquenessTest.php:133

composer check:routes already passed on the broken file and still reports
PASS, all 261 routes point at existing controller methods. It reads the
declared array and never asks what registers, which is why it could not see
this.

Verified

  • php -l on both files
  • composer check:routes: PASS, 261 routes
  • vendor/bin/phpunit --filter RouteNameUniquenessTest: 2 tests, 267 assertions, green
  • phpcs scopes lib only in this repo, and neither changed file is under it

Inherited, not fixed here

appinfo/routes.php carries pre-existing phpcs findings, including a 157
character line at lti#agsScore. They are on lines this change did not touch
and belong to the debt sweep.

🤖 Generated with Claude Code

…istered

Nextcloud names a route after its controller, its action and its postfix, and
RouteCollection::add() overwrites an entry of the same name. The URL and the
verb are not part of it. GET /api/lti/{deployment}/login was overwritten by the
POST beside it, and GET / by the SPA catch-all.

Measured with Nextcloud's own RouteParser: declared=261 registered=259 before,
261 after. The postfix goes on the entries nothing references by name, so
info.xml navigation and Flow\SynchronizationLogActions keep resolving
integriq.ui.dashboard to the catch-all they resolve today. No URL or verb
changes.

RouteNameUniquenessTest asserts on each entry's registration key and is
mutation-checked against the old routes file.
@github-actions

Copy link
Copy Markdown
Contributor

Quality Report — ConductionNL/integriq @ 5ed343e

Check PHP Vue Security License Tests
lint ✅
phpcs ❌
phpmd ❌
psalm ❌
phpstan ❌
phpmetrics ✅
eslint ✅
stylelint ✅
build ✅
build ✅
check-specs ✅
test-l10n ❌
format ❌
check-schema-l10n ❌
check-l10n-js ✅
composer ✅ ✅ 141/141
npm ✅ ✅ 545/545
app:check-code ⏭️
info.xml ✅
REUSE ❌
lockfile sync ✅
PHPUnit ✅
Newman ✅
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 on any branch from the Actions tab, or locally with npx playwright test.
Hydra gates ❌
Hydra gates (axe) ✅

Quality workflow — 2026-09-19 08:55 UTC

Download the full PDF report from the workflow artifacts.

@rubenvdlinde
rubenvdlinde merged commit e8a2dea into development Sep 19, 2026
38 of 48 checks passed
@rubenvdlinde
rubenvdlinde deleted the routes/unique-names branch September 19, 2026 08:58
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