Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Allow some apps to have root URLs in their own routing file #20114

Merged
merged 11 commits into from
Apr 21, 2020
Merged

Allow some apps to have root URLs in their own routing file #20114

merged 11 commits into from
Apr 21, 2020

Conversation

nickvergessen
Copy link
Member

Follow up to prevent #20038 from spreading

@nickvergessen nickvergessen added this to the Nextcloud 19 milestone Mar 23, 2020
@nickvergessen nickvergessen mentioned this pull request Mar 23, 2020
Merged
2 tasks
nickvergessen
nickvergessen commented Mar 23, 2020
View reviewed changes
core/routes.php
['name' => 'requesthandlercontroller#addShare', 'url' => '/ocm/shares', 'verb' => 'POST', 'app' => 'cloud_federation_api'],
['name' => 'requesthandlercontroller#receiveNotification', 'url' => '/ocm/notifications', 'verb' => 'POST', 'app' => 'cloud_federation_api'],
['name' => 'pagecontroller#showCall', 'url' => '/call/{token}', 'verb' => 'GET', 'app' => 'spreed'],
['name' => 'pagecontroller#authenticatePassword', 'url' => '/call/{token}', 'verb' => 'POST', 'app' => 'spreed'],
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Those are now in nextcloud/spreed#3134

All other apps are in this PR

nickvergessen
nickvergessen commented Mar 23, 2020
View reviewed changes
lib/private/AppFramework/Routing/RouteConfig.php
@@ -98,42 +106,7 @@ public function register() {
private function processOCS(array $routes): void {
$ocsRoutes = $routes['ocs'] ?? [];
foreach ($ocsRoutes as $ocsRoute) {
$name = $ocsRoute['name'];
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Best review this file on https://github.com/nextcloud/server/pull/20114/files?w=1

This was referenced Apr 4, 2020
Merged
Merged
@skjnldsv skjnldsv force-pushed the techdebt/noid/allow-some-apps-to-have-root-urls branch from 90dda67 to 12c43b3 Compare April 11, 2020 06:42
@skjnldsv
Copy link
Member

Rebased

@skjnldsv
Copy link
Member

Lots of failures

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Apr 11, 2020
@nickvergessen nickvergessen force-pushed the techdebt/noid/allow-some-apps-to-have-root-urls branch from 12c43b3 to e07f126 Compare April 14, 2020 15:26
@nickvergessen nickvergessen added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Apr 14, 2020
@nickvergessen nickvergessen mentioned this pull request Apr 14, 2020
Closed
@skjnldsv skjnldsv force-pushed the techdebt/noid/allow-some-apps-to-have-root-urls branch from d7058c6 to 8e786e4 Compare April 15, 2020 06:00
@nickvergessen nickvergessen force-pushed the techdebt/noid/allow-some-apps-to-have-root-urls branch from 8e786e4 to 4bae8db Compare April 15, 2020 12:13
This was referenced Apr 15, 2020
Merged
Merged
danxuliu
danxuliu requested changes Apr 16, 2020
View reviewed changes
Copy link
Member

@danxuliu danxuliu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PHP CS is not happy.

Some integration and acceptance tests failures are legit, and all seem to be caused by Unable to generate a URL for the named route \"cloud_federation_api.RequestHandler.addShare\" as such route does not exist.

@nickvergessen
Copy link
Member Author

PHP CS is not happy.

Should be fixed by a last rebase as it's not caused by this PR.

@nickvergessen nickvergessen force-pushed the techdebt/noid/allow-some-apps-to-have-root-urls branch from 4bae8db to a0f75ae Compare April 17, 2020 09:21
@nickvergessen @rullzer
Allow some apps to have root URLs in their own routing file
3f8168b 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
Fix creation of legacy routes
aad16c8 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
PHP CS fixes
f93d55e 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
Extend tests for root url
250467e 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
Fix legacy routes
708b499 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
Fix creating the share controller
e45c87c 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
Fix route definitions of cloud_federation_api
9654a92 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
Reduce noise in test output
031b665 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
Make sure the cloud_federation_api routes are loaded
6332d7c 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
Directly use the class to get the service
685d7b0 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@nickvergessen @rullzer
PHP CS fixes
1b93d5f 
Signed-off-by: Joas Schilling <coding@schilljs.com>
@rullzer rullzer force-pushed the techdebt/noid/allow-some-apps-to-have-root-urls branch from ebba033 to 1b93d5f Compare April 18, 2020 09:21
@nickvergessen
Copy link
Member Author

SHould be good to go now?
Not sure what the acceptance tests complain about. And the failing integration tests are caused by #16035

@rullzer
Copy link
Member

rullzer commented Apr 21, 2020

SHould be good to go now?
Not sure what the acceptance tests complain about. And the failing integration tests are caused by #16035

Lets do this and we fix stuff if it does boom

@rullzer rullzer merged commit 8f650fe into master Apr 21, 2020
@rullzer rullzer deleted the techdebt/noid/allow-some-apps-to-have-root-urls branch April 21, 2020 14:00
@danxuliu
Copy link
Member

Not sure what the acceptance tests complain about

The app-comments can be ignored, they are caused by a regression in the sidebar. The others are legit; for some reason with this pull request the Apps app and the Users app show a blank page (and in 8f650fe^ they work fine).

@rullzer
Copy link
Member

rullzer commented Apr 21, 2020

Not sure what the acceptance tests complain about

The app-comments can be ignored, they are caused by a regression in the sidebar. The others are legit; for some reason with this pull request the Apps app and the Users app show a blank page (and in 8f650fe^ they work fine).

which apps?

@rullzer
Copy link
Member

rullzer commented Apr 21, 2020

aah the apps and user page
mmm indeed.... so I ahve a crude fix ut this seems to break js...

@rullzer rullzer mentioned this pull request Apr 21, 2020
Merged
@NeutroGe NeutroGe mentioned this pull request May 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danxuliu danxuliu danxuliu requested changes

@rullzer rullzer rullzer approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish technical debt
Projects
None yet
Milestone
Nextcloud 19
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nickvergessen @skjnldsv @rullzer @danxuliu @ChristophWurst