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.

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

Remove the matching on ancient /ocs routes #23820

Merged
merged 1 commit into from
Nov 4, 2020

Conversation

rullzer
Copy link
Member

@rullzer rullzer commented Oct 31, 2020

The appframework has been the default for quite some time now. So time
to take proper advantage of it.

Should shave off matching and loading some routes. Should help with response times for OCS routes.

The appframework has been the default for quite some time now. So time
to take proper advantage of it.

Signed-off-by: Roeland Jago Douma <roeland@famdouma.nl>
@faily-bot
Copy link

faily-bot bot commented Oct 31, 2020

🤖 beep boop beep 🤖

Here are the logs for the failed build:

Status of 34834: failure

mysql5.6-php7.3

Show full log
There was 1 error:

1) Test\Comments\ManagerTest::testDeleteCommentsAtObject
InvalidArgumentException: IDs must be translatable to a number in this implementation.

/drone/src/lib/private/Comments/Manager.php:253
/drone/src/tests/lib/Comments/ManagerTest.php:673

--

There were 2 warnings:

1) Test\Files\ViewTest::testRenameFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

2) Test\Files\ViewTest::testCopyFailDeleteTargetKeepSource
Trying to configure method "writeStream" which cannot be configured because it does not exist, has not been specified, is final, or is static

@MorrisJobke MorrisJobke merged commit 45ef5d1 into master Nov 4, 2020
@MorrisJobke MorrisJobke deleted the td/remove/OCPAPI_check branch November 4, 2020 20:56
@MorrisJobke
Copy link
Member

@kesselb Any idea about those random Psalm failures?

@danxuliu
Copy link
Member

Note that before this pull request OC_App::loadApps() was called before OC::handleLogin, but now it is called after it. This has caused Talk to no longer create room shares in the Talk folder in some cases.

It seems that VerifyMountPointEvent is no longer handled in Talk if a share information is got when the sharee is not logged in. For example, if the share information is got through OCS, like done in the sharing integration tests: https://drone.nextcloud.com/nextcloud/spreed/10104/12/3.

It can also be reproduced in a real instance:

  • Log in as user A
  • Open Talk
  • Create a conversation and add user B
  • Share a file to that conversation
  • Get the ID of the share
  • In a console, get the share information as user B with something like curl -u {userB}:{password} -X GET -H "OCS-APIRequest: true" "http://127.0.0.1:8000/ocs/v2.php/apps/files_sharing/api/v1/shares/{shareId}

If user B now logs in the shared file will be in the root instead of in the Talk folder. However, if now, while user B is logged in, user A shares another file in the conversation and user B reloads the Files app the new shared file will be in the Talk folder of user B.

@PVince81
Copy link
Member

PVince81 commented Dec 9, 2020

The tests in Talk are broken on master, see nextcloud/spreed#4736.

It likely makes more sense to load the apps before handling login, because some apps might do stuff at that time.

@rullzer
Copy link
Member Author

rullzer commented Dec 9, 2020

Let's revert this then and try to fix it better in 22

@PVince81
Copy link
Member

PVince81 commented Dec 9, 2020

it might be enough to move loadApps() to the top to not change the initialization order

I tried automatic revert and it failed, will need to do manually

@PVince81
Copy link
Member

here's a PR that just changes the order: #24628

I guess in the future if we want to fix the order we might need to make it a v3.php and allow enough time for apps to migrate...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants