-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Make sure urlparams are correctly injected in global routes #14626
Make sure urlparams are correctly injected in global routes #14626
Conversation
Signed-off-by: Joas Schilling <coding@schilljs.com>
Signed-off-by: Joas Schilling <coding@schilljs.com>
@@ -78,6 +78,17 @@ | |||
['name' => 'contactsMenu#findOne', 'url' => '/contactsmenu/findOne', 'verb' => 'POST'], | |||
['name' => 'WalledGarden#get', 'url' => '/204', 'verb' => 'GET'], | |||
['name' => 'Search#search', 'url' => '/core/search', 'verb' => 'GET'], | |||
|
|||
// Legacy routes that need to be globally available while they are handled by an app | |||
['name' => 'viewcontroller#showFile', 'url' => '/f/{fileid}', 'verb' => 'GET', 'app' => 'files'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should also be possible, to move those to the respective apps, when at the same time we allow defining a root' => '/cloud'
similar to OCS urls.
Just not sure how this breaks existing route generations (because all lowercase instead of CamelCase controller names, which is why the hacks in lib/private/AppFramework/Routing/RouteConfig.php
were necessary)
if (strpos($controllerName, '\\Controller\\') !== false) { | ||
// This is from a global registered app route that is not enabled. | ||
[/*OC(A)*/, $app, /* Controller/Name*/] = explode('\\', $controllerName, 3); | ||
throw new HintException('App ' . strtolower($app) . ' is not enabled'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is inaccurate for cloud_federation_api
as it shows cloudfederationapi
instead.
Worth adding another hack? however it's pretty useless anyway.
🤖 beep boop beep 🤖 Here are the logs for the failed build: Status of 16911: failureTESTS=integration-federation_features
Show full log
TESTS=acceptance, TESTS-ACCEPTANCE=app-files
Show full log
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and works 👍 Also the code looks good 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense, code looks good!
faily-bot was still complaining about failures yet this PR got merged?! |
Yes because there are some failures on master and we are currently looking into how to fix them. They slipped through. |
Fix #14621