Skip to content

Trash bin: fix exception triggered if no correct fileId is provided.#18728

Merged
rullzer merged 1 commit intonextcloud:masterfrom
simonspa:trashbin_exception
Jan 8, 2020
Merged

Trash bin: fix exception triggered if no correct fileId is provided.#18728
rullzer merged 1 commit intonextcloud:masterfrom
simonspa:trashbin_exception

Conversation

@simonspa
Copy link
Contributor

@simonspa simonspa commented Jan 7, 2020

Somehow in some installations the trash bin preview controller is called with incorrect $fileId. By assigning a default value and catching the default, a BAD_REQUEST can be returned instead. This behavior is taken from the "normal" files preview controller.

This fixes #17115

@simonspa simonspa mentioned this pull request Jan 7, 2020
Closed
@simonspa simonspa force-pushed the trashbin_exception branch from a1070f2 to f4fe0ed Compare January 7, 2020 22:38
@kesselb kesselb added this to the Nextcloud 18 milestone Jan 7, 2020
@simonspa simonspa force-pushed the trashbin_exception branch from f4fe0ed to d9913ed Compare January 7, 2020 22:41
@kesselb kesselb added 3. to review Waiting for reviews bug labels Jan 7, 2020
@kesselb
Copy link
Contributor

kesselb commented Jan 7, 2020

Thanks 👍

Could you try to rebase your patch with the latest changes from nextcloud/server? https://github.com/simonspa/server/tree/trashbin_exception is some commits behind hence the CI is failing. I'm usually do it like this: https://thoughtbot.com/blog/keeping-a-github-fork-updated

@simonspa simonspa force-pushed the trashbin_exception branch from d9913ed to b3ad27f Compare January 7, 2020 22:47
@simonspa
Copy link
Contributor Author

simonspa commented Jan 8, 2020

@kesselb I somehow forgot to update my fork before adding the fix. Anyway, rebased now. Somehow some of the CI tests still fail. Do I need to take care of something else?

rullzer
rullzer approved these changes Jan 8, 2020
View reviewed changes
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Nice catch!

ChristophWurst
ChristophWurst approved these changes Jan 8, 2020
View reviewed changes
Copy link
Member

@ChristophWurst ChristophWurst left a comment

Choose a reason for hiding this comment

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

Alternatively we could have made it nullable, but fine by me 👍

Thanks :)

@ChristophWurst ChristophWurst added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Jan 8, 2020
@rullzer rullzer merged commit ada25ab into nextcloud:master Jan 8, 2020
@welcome
Copy link

welcome bot commented Jan 8, 2020

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22
Most developers hang out on IRC. So join #nextcloud-dev on Freenode for a chat!

@rullzer rullzer mentioned this pull request Jan 8, 2020
Merged
1 task
@simonspa
Copy link
Contributor Author

simonspa commented Jan 8, 2020

Maybe this could also be backported for 17.0.3?

@rullzer
Copy link
Member

rullzer commented Jan 8, 2020

/backport to stable17

@backportbot-nextcloud backportbot-nextcloud bot mentioned this pull request Jan 8, 2020
Merged
@backportbot-nextcloud
Copy link

backportbot-nextcloud bot commented Jan 8, 2020

backport to stable17 in #18734

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

Reviewers

@rullzer rullzer rullzer approved these changes

@ChristophWurst ChristophWurst ChristophWurst approved these changes

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@kesselb kesselb Awaiting requested review from kesselb

Assignees

No one assigned

Labels

4. to release Ready to be released and/or waiting for tests to finish bug

Projects

None yet

Milestone

Nextcloud 18.0.0

Development

Successfully merging this pull request may close these issues.

TypeError: Argument 1 passed to OCA\Files_Trashbin\Controller\PreviewController::getPreview() must be of the type int, null given

4 participants

@simonspa @kesselb @rullzer @ChristophWurst