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

Fire prehooks when uploading directly to storage #16292

Merged
merged 1 commit into from
May 15, 2015

Conversation

PVince81
Copy link
Contributor

Manually fire pre-hooks in direct to storage upload case (webdav PUT of small files).

Fix for missing versions on WebDAV PUT: #16130

CC @icewind1991

TODOs:

  • more extensive manual testing
  • unit tests
    • are hooks firing ?
    • error cases
    • case where a hook sets $run to false

@PVince81 PVince81 added this to the 8.1-current milestone May 12, 2015
@ghost
Copy link

ghost commented May 12, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/12311/
🚀 Test PASSed.🚀
chuck

@PVince81 PVince81 force-pushed the webdav-storage-fireprehooks branch from a7b8819 to 3cae013 Compare May 13, 2015 15:47
@scrutinizer-notifier
Copy link

A new inspection was created.

@PVince81
Copy link
Contributor Author

I've added the unit tests to check if the hooks are firing.

Please review @rullzer @DeepDiver1975 @icewind1991 @schiesbn @MorrisJobke

@PVince81
Copy link
Contributor Author

After this one is merged, a bug will appear related to versions and s2s: #16334
It was hidden because the versions hooks were not triggered on master. (a bug can hide another one!)

@icewind1991
Copy link
Contributor

👍 looks good

@ghost
Copy link

ghost commented May 13, 2015

Refer to this link for build results (access rights to CI server needed):
https://ci.owncloud.org//job/pull-request-analyser-ng-simple/12334/
🚀 Test PASSed.🚀
chuck

@PVince81
Copy link
Contributor Author

needs a second reviewer @rullzer @DeepDiver1975 @icewind1991 @schiesbn @MorrisJobke @nickvergessen

$renameOkay = $storage->moveFromStorage($partStorage, $internalPartPath, $internalPath);
$fileExists = $storage->file_exists($internalPath);
}
if (!$run || $renameOkay === false || $fileExists === false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks weird... the !$run but the comparison for the others..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This only replicates the old behavior.

In the past we only called $view->rename() instead of $storage->moveFromStorage().
In the case of the view, it would internally trigger the pre and post hooks.
If the pre-hook fails, then it would return false. See https://github.com/owncloud/core/blob/master/lib/private/files/view.php#L621 and the else block.
This would cause $renameOkay to be false.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually $run means "all the hooks agreed that the action can be run" or "$run is true when the operation is NOT cancelled by a hook"

Copy link
Contributor

Choose a reason for hiding this comment

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

No it was just a tiny style thing..

You write !$run but the but the other comparisons are written in full...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes... it was out of the habit of not trusting our code to return a solid false in some cases.
I'll leave it as is for now, unless I need to retouch the code for other issues.

@rullzer
Copy link
Contributor

rullzer commented May 15, 2015

upload via webdav now adds a new version :)
Yay for unit tests :)

Just my tiny style comment... but you could also chose to ignore that :P

Looking good 👍

@nickvergessen
Copy link
Contributor

👍

nickvergessen added a commit that referenced this pull request May 15, 2015
Fire prehooks when uploading directly to storage
@nickvergessen nickvergessen merged commit 0991c0c into master May 15, 2015
@nickvergessen nickvergessen deleted the webdav-storage-fireprehooks branch May 15, 2015 13:08
mmattel pushed a commit to mmattel/core that referenced this pull request May 22, 2015
…ehooks

Fire prehooks when uploading directly to storage
@lock lock bot locked as resolved and limited conversation to collaborators Aug 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants