-
Notifications
You must be signed in to change notification settings - Fork 2.1k
change order of registering api and capabilities to fix file version previews #9269
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
Conversation
|
without this the versions preview will use urls like http://localhost/core/index.php/ocs/preview?file=%2FNeue+Textdatei.txt&version=1403887662, with this they look like http://localhost/core/index.php/apps/files_versions/preview?file=%2FNeue+Textdatei.txt&version=1403890359 So there is definitely something wrong here. Adding to Milestone ownCloud 7 CE. |
|
Related issue: #8375 |
|
this looks good 👍 |
|
I cannot reproduce the issue with current master. |
|
@butonic wouldn't it make more sense to fix this globally by resetting the collection to root after ocs registration |
|
@DeepDiver1975 I totally agree. But I chose the quick fix for this because I have not yet had a look at the API route registering code and wanted to make sure that it makes sense to restore the collection before the API register call returns. |
|
@DeepDiver1975 any further comments on this ? For me the version previews already worked, so not sure how to reproduce the original issue. |
|
@DeepDiver1975 @icewind1991 I now reset the route collection to 'root' because I cannot get the current route collection name from the route class. It smells horribly because the class behaves differently depending on state (the current route collection) and we have no way of asking from the outside which one that is. I'd say we either need a public |
|
My vote goes to |
|
A new inspection was created. |
|
@DeepDiver1975 @icewind1991 @bartv2 This should fix weird routing errors. Let's see what jenkins says |
|
🚀 Test Passed. 🚀 |
|
Code looks good 👍 |
| @@ -72,6 +73,7 @@ public static function register($method, $url, $action, $app, | |||
| ->requirements($requirements) | |||
| ->action('OC_API', 'call'); | |||
| self::$actions[$name] = array(); | |||
| OC::$server->getRouter()->useCollection($oldCollection); | |||
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.
because of $collectionName being initialized with null, null could be passed into useCollection() - I doubt this is giving us the right behavior.
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.
Actually, before the route.php is included the collection is set to the collection for the current app: https://github.com/owncloud/core/blob/fix_versions_preview/lib/private/route/router.php#L128
Registering api routes in routes.php is simply wrong. Well, not wrong, but it does not work reliably. They should go into api-routes.php or something. This PR at least resets the collection to what it was before setting an API route.
|
👍 |
change order of registering api and capabilities to fix file version previews
|
I'll take care about backporting |
|
backport to stable7: |
When registering an api call the collection gets set to
My guess is it does not get reverted so when the next create rout call is executed is goes into the wrong collection. Fixes
@PVince81 @icewind1991 @schiesbn a quick review please.