refactor(base): modernize and tidy `OC::handleRequest()` by joshtrichards · Pull Request #60293 · nextcloud/server

joshtrichards · 2026-05-11T06:03:54Z

Summary

Refactors the monolithic OC::handleRequest() in lib/base.php for readability and maintainability. Streamlines/documents the control flow and decomposes it a bit. No functional changes intended.

Key changes:

Extracts app-loading logic into named helpers: loadAuthenticationApps(), loadAppsForPreAuthenticationPhase(), loadAppsForAuthenticatedRequests(), loadAppsForRouting()
Extracts themingAssetRequest() helper to replace a long inline &&-chain
Splits checkMaintenanceMode() into a pure renderer (renderMaintenancePage()) with the gate condition now inline in handleRequest() — making the control flow explicit
Renames printUpgradePage() → renderUpgradePage() for naming consistency
Removes unreachable !self::$CLI guard (handleRequest() is only ever called from index.php)
Replaces substr($path, -3) !== '.js' with str_ends_with()
Resolves $userSession once and reuses it; removes redundant re-fetches
Adds/improves inline comments and a full docblock on handleLogin(), which has important side effects to know about

Notes

Not sure if this is best left for v35 versus v34 so I left off the milestone. This isn't a feature and should just be an internal refactor so it'd be nice to have it in so we're iterating any backports on the newer code, but I have no strong opinion either way as long as it ends up in master. ;)
OC::handleRequest() has no existing standalone / direct unit test coverage. There is, however, a significant amount of indirect coverage via existing integration tests and most of the code paths - particularly the sensitive ones (like maintenance, theming, authentication, etc.) - are covered in that way. The current architecture (bare exit(), header(), Server::get() calls, superglobal reads, etc) makes unit testing impractical without a larger refactor.

TODO

Test test test
Drop old /core/ajax/update.php request path bypass in this pass?

Checklist

Code is properly formatted
Sign-off message is added to all commits
Tests (unit, integration, api and/or acceptance) are included
Screenshots before/after for front-end changes
Documentation (manuals or wiki) has been updated or is not required
Backports requested where applicable (ex: critical bugfixes)
Labels added where applicable (ex: bug/enhancement, 3. to review, feature component)
Milestone added for target branch/version (ex: 32.x for stable32)

AI (if applicable)

The content of this PR was partly or fully generated using AI

Signed-off-by: Josh <josh.t.richards@gmail.com>

The real contract is mostly side effects and possibly exceptions, which is important to have documented. Signed-off-by: Josh <josh.t.richards@gmail.com>

Signed-off-by: Josh <josh.t.richards@gmail.com>

…route Signed-off-by: Josh <josh.t.richards@gmail.com>

Long standing behavior. Just adds test coverage for it. Signed-off-by: Josh <josh.t.richards@gmail.com>

come-nc

A few remarks but the changes looks good.
Could you squash the commits into something more sensible?

come-nc · 2026-05-26T08:39:36Z

+		$upgrade = Util::needUpgrade();
+
+		// Show "upgrade" page if Nextcloud needs to be upgraded and not in maintenance mode (i.e. already in progress).
+		if ($upgrade && !$maintenance && !$bypassMaintenance) {


Suggested change

if ($upgrade && !$maintenance && !$bypassMaintenance) {

if ($upgrade && !$bypassMaintenance) {

The maintenance check is redundant, we exited earlier if we are in maintenance without bypass.
We may want to rename $bypassMaintenance as it also bypasses upgrade check. Could be $isJavascriptResource or something? Or $bypassMaintenanceAndUpgrade.

come-nc · 2026-05-26T08:40:03Z

+			// NOTE: This is shown to the first web visitor to land after a code update...
+			// ...and will continue to be shown to subsequent visitors until the upgrade is
+			// triggered.


Suggested change

// NOTE: This is shown to the first web visitor to land after a code update...

// ...and will continue to be shown to subsequent visitors until the upgrade is

// triggered.

// NOTE: This is shown to the first web visitor to land after a code update

// and will continue to be shown to subsequent visitors until the upgrade is

// triggered.

This would actually be clearer:

Suggested change

// NOTE: This is shown to the first web visitor to land after a code update...

// ...and will continue to be shown to subsequent visitors until the upgrade is

// triggered.

// NOTE: This is shown to web visitors after a code update until the upgrade is triggered.

come-nc · 2026-05-26T08:45:55Z

+			$router->match($requestPath);
+			return;
+		} catch (ResourceNotFoundException $e) {
+			// ...


Should be replaced by an explanation of why we continue when this exception happens.

come-nc · 2026-05-26T08:48:48Z

 		if ($requestPath === '') {
-			// Someone is logged in
-			$userSession = Server::get(IUserSession::class);
+			// Redirect to the default app if visitor is logged in
 			if ($userSession->isLoggedIn()) {
 				header('X-User-Id: ' . $userSession->getUser()?->getUID());
 				header('Location: ' . Server::get(IURLGenerator::class)->linkToDefaultPageUrl());
 			} else {
-				// Not handled and not logged in
+				// Redirect to the login page if visitor is not logged in
 				header('Location: ' . Server::get(IURLGenerator::class)->linkToRouteAbsolute('core.login.showLoginForm'));
 			}
 			return;
 		}


Can’t that be done earlier?
Looks like a loss of time and resources to first run the router and load all apps just to issue a redirection for default route.

come-nc · 2026-05-26T08:51:44Z

 		}
 	}

+	private static function themingAssetRequest(string $requestPath): bool {


Suggested change

private static function themingAssetRequest(string $requestPath): bool {

private static function isThemingAssetRequest(string $requestPath): bool {

joshtrichards added 4 commits May 11, 2026 02:03

refactor(base): modernize and tidy handleRequest()

4a57e07

Signed-off-by: Josh <josh.t.richards@gmail.com>

chore: fixup typo

1423b8c

Signed-off-by: Josh <josh.t.richards@gmail.com>

chore: fixup base typo

a5282c4

Signed-off-by: Josh <josh.t.richards@gmail.com>

chore: handle login bypass same as maintenance bypass

17dea66

Signed-off-by: Josh <josh.t.richards@gmail.com>

CarlSchwan reviewed May 11, 2026

View reviewed changes

Comment thread lib/base.php Outdated

joshtrichards added 2 commits May 11, 2026 09:34

chore: fixup typos and further streamline upgrade path

3edb729

Signed-off-by: Josh <josh.t.richards@gmail.com>

chore(base): lint fixup + add note about auth app loading

e86a321

Signed-off-by: Josh <josh.t.richards@gmail.com>

joshtrichards added the 2. developing Work in progress label May 11, 2026

joshtrichards added 2 commits May 14, 2026 09:47

docs(base): document real handleLogin contract/behavior

5e3a042

The real contract is mostly side effects and possibly exceptions, which is important to have documented. Signed-off-by: Josh <josh.t.richards@gmail.com>

chore(base.php): ensure bypass gets correct apps + routing gets all apps

96614f9

Signed-off-by: Josh <josh.t.richards@gmail.com>

joshtrichards added the ♻️ refactor Refactor code (not a bug fix, not a feature just refactoring) label May 14, 2026

joshtrichards mentioned this pull request May 14, 2026

refactor(ocs): improve readability of v1 front controller #60376

Draft

8 tasks

chore(base.php): fixup for lint/cs

768b22a

Signed-off-by: Josh <josh.t.richards@gmail.com>

joshtrichards added 3. to review Waiting for reviews feature: files and removed 2. developing Work in progress labels May 15, 2026

joshtrichards marked this pull request as ready for review May 15, 2026 14:41

joshtrichards requested a review from a team as a code owner May 15, 2026 14:41

joshtrichards requested review from ArtificialOwl, CarlSchwan, leftybournes and salmart-dev and removed request for a team May 15, 2026 14:41

joshtrichards changed the title ~~refactor(base): modernize and tidy handleRequest()~~ refactor(base): modernize and tidy OC::handleRequest() May 15, 2026

chore(base): make renderMaintenancePage pvt and drop old ajax update …

7977062

…route Signed-off-by: Josh <josh.t.richards@gmail.com>

joshtrichards added this to the Nextcloud 35 milestone May 19, 2026

joshtrichards requested a review from come-nc May 19, 2026 13:49

test(integration): add coverage for .js bypass during maintenance

a5dad89

Long standing behavior. Just adds test coverage for it. Signed-off-by: Josh <josh.t.richards@gmail.com>

come-nc reviewed May 26, 2026

View reviewed changes

joshtrichards added the technical debt 🧱 🤔🚀 label May 27, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor(base): modernize and tidy `OC::handleRequest()`#60293

refactor(base): modernize and tidy `OC::handleRequest()`#60293
joshtrichards wants to merge 11 commits into
masterfrom
jtr/refactor-base-handleRequest

joshtrichards commented May 11, 2026 •

edited

Loading

Uh oh!

Uh oh!

come-nc left a comment

Uh oh!

come-nc May 26, 2026

Uh oh!

come-nc May 26, 2026

Uh oh!

come-nc May 26, 2026

Uh oh!

come-nc May 26, 2026

Uh oh!

come-nc May 26, 2026

Uh oh!

come-nc May 26, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	if ($upgrade && !$maintenance && !$bypassMaintenance) {
	if ($upgrade && !$bypassMaintenance) {

	private static function themingAssetRequest(string $requestPath): bool {
	private static function isThemingAssetRequest(string $requestPath): bool {

Uh oh!

Conversation

joshtrichards commented May 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Notes

TODO

Checklist

AI (if applicable)

Uh oh!

Uh oh!

come-nc left a comment

Choose a reason for hiding this comment

Uh oh!

come-nc May 26, 2026

Choose a reason for hiding this comment

Uh oh!

come-nc May 26, 2026

Choose a reason for hiding this comment

Uh oh!

come-nc May 26, 2026

Choose a reason for hiding this comment

Uh oh!

come-nc May 26, 2026

Choose a reason for hiding this comment

Uh oh!

come-nc May 26, 2026

Choose a reason for hiding this comment

Uh oh!

come-nc May 26, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

joshtrichards commented May 11, 2026 •

edited

Loading