First attempt to check against core routes before loading all app routes #24295
Conversation
juliusknorr
requested review from
rullzer,
MorrisJobke,
ChristophWurst,
kesselb and
skjnldsv
core/routes.php
Outdated
| ['name' => 'Svg#getSvgFromApp', 'url' => '/svg/{app}/{fileName}', 'verb' => 'GET'], | ||
| ['name' => 'Css#getCss', 'url' => '/css/{appName}/{fileName}', 'verb' => 'GET'], | ||
| ['name' => 'Js#getJs', 'url' => '/js/{appName}/{fileName}', 'verb' => 'GET'], | ||
| ['name' => 'Svg#getSvgFromCore', 'url' => '/core/svg/core/{folder}/{fileName}', 'verb' => 'GET'], |
There was a problem hiding this comment.
This would not be needed, but those should be safe to change and it feels a bit cleaner to have them properly prefixed, so that one knows where they are located.
There was a problem hiding this comment.
Most likely the same for the others. Maybe we register the old URLs as well for a grace period and notify the maintainers of those apps about the change?
There was a problem hiding this comment.
Let's actually skip this change for now, since this was mainly my starting point, but I figured that it was suitable to just check core routes first if we don't match the a known route prefix like /apps/.
juliusknorr
force-pushed
the
perf/noid/router
branch
from
8ce1c69 to
ced2560
Compare
juliusknorr
force-pushed
the
perf/noid/router
branch
from
ced2560 to
ea69a14
Compare
|
Let us do this for 22 early in the cycle so we catch potential issues. |
Most of the time we will find a matching route in either the loaded ones matched on the URL or in the core ones. Therfore we can attempt to try those first and only load all app routes if no match was found by the Symfony URLMatcher. Signed-off-by: Julius Härtl <[email protected]>
|
failing test is unrelated and fixed in master |
|
this causes a regression in the Talk app using the autocomplete API: https://github.com/nextcloud/spreed/issues/5887 hard to tell why |
| if (!\OC::$server->getConfig()->getSystemValueBool('maintenance') && !Util::needUpgrade()) { | ||
| \OC_App::loadApps(); | ||
| } | ||
| $this->loadRoutes('core'); |
There was a problem hiding this comment.
Just loading the core routes here seems instead of all routes causes the guests app to be loaded too late so the AppConfigOverwrite is not yet registered when the DI is assembling the arguments for the autocomplete controller 🙈
There was a problem hiding this comment.
Seems this is due to https://github.com/nextcloud/guests/blob/eb7eb7c1b31e751bbbcc7671e5e1997b82297d45/appinfo/routes.php#L22-L26 where I'm not sure why the late setup would be needed here. A quick test for moving that to app.php seems to fix the issue
There was a problem hiding this comment.
how to proceed, then ? I don't have enough core knowledge to be able to evaluate whether your proposed change is safe to fix nextcloud/guests#654
|
revert PR here #27715 for a short term quick fix |
Most of the time we will find a matching route in either the loaded ones
matched on the URL or in the core ones. Therfore we can attempt to try
those first and only load all app routes if no match was found by the
Symfony URLMatcher.
This improves the loading times of all routes that do not match the checks to load only required routes
/apps/,/ocsapp/apps/,/settings/or/core/, mostly the core routes that do not start with core like unified search, svg, css, login to mention some of them.https://blackfire.io/profiles/compare/4c47d56e-5311-4acc-883a-176b8f7c374f/graph
https://blackfire.io/profiles/compare/6df20296-dc75-43a6-8629-22d530805fb7/graph