-
Notifications
You must be signed in to change notification settings - Fork 134
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
Potential fix for #326 #343
Conversation
@@ -190,7 +190,10 @@ public function update($id, $content, $userId, $category = null, $mtime = 0) { | |||
} | |||
|
|||
if ($content !== null) { | |||
$file->putContent($content); | |||
$stream = fopen('php://temp', 'r+'); |
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.
You're never closing $stream
with fclose
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.
The stream is closed by putContent()
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.
Okay, error-prone ownership management.
Does this actually work? |
Just tested, yes it did. Thanks. I use Nextcloud 15.x so it would be nice to push the update to that version too. |
The issue was fixed in Nextcloud Server for Nextcloud 15, 16 and 17. 🎉 I'm closing this PR now, since we don't need a workaround patch in the notes app anymore. Thanks again, @marcelklehr ! |
It appears that the ObjectStore Storage implementation puts the file contents into the php temp stream, which is passed to the S3 uploader, which for whatever reason tries to reopen the underlying file of the stream, which happens to be the php temp stream, which is not reusable by passing its uri and is thus empty.
A quick fix, if the above theory is correct, would be to pass the contents as a stream to putContents(), which forces nextcloud to write the stream to disk in a temp file and thus avoids the failing S3 upload, as the aws uploader now has an actual file to work with instead of the temp stream.
I cannot test this myself, as I lack access to an S3 instance.
If this fix works, an upstream fix in nextcloud would be in order as well.
Fixes #326