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

Fix ObjectStoreStorage#file_put_contents #16397

Closed

Conversation

marcelklehr
Copy link
Member

Currently this method creates creates a stream using php temp stream and passes it to writeStream(). This doesn't work, because the aws uploader cheats and tries to read the original file, which in this case is empty, because php://temp cannot be reused. This is why creating new notes in the notes app for example currently fails and only creates an empty note. My fix uses fopen, which directs all writes to a temp file which can be re-opened by the aws uploader with no issues.

@kesselb kesselb requested review from icewind1991 and rullzer July 14, 2019 19:54
@kesselb kesselb added this to the Nextcloud 17 milestone Jul 14, 2019
@kesselb kesselb added 3. to review Waiting for reviews bug labels Jul 14, 2019
@kesselb
Copy link
Contributor

kesselb commented Jul 14, 2019

Thanks 👍

Mind to sign off your commits: https://github.com/nextcloud/server/pull/16397/checks?check_run_id=168559528

case 'w':
case 'wb':
case 'w+':
case 'wb+':
$tmpFile = \OC::$server->getTempManager()->getTemporaryFile($ext);
$handle = fopen($tmpFile, $mode);
return CallbackWrapper::wrap($handle, null, null, function () use ($path, $tmpFile) {
$this->writeBack($tmpFile, $path);
});

Make sense somehow. A resource created by $this->fopen($path, 'w+'); uses a temporary file the LazyOpenStream can reuse later. Sounds hacky 💩 (not your code - the fact that we write all the data into a temp file and delete it later. there is a way to pass the raw data to s3 uploader but we don't expose this feature from the sdk to the IObjectStore interface).

@marcelklehr
Copy link
Member Author

Yeah, it's not really a clean solution.

@marcelklehr
Copy link
Member Author

I did the change on github, now I can't access the branch in my local git 😞 🤷‍♂️ If you want you can create a new pr.

@MorrisJobke MorrisJobke mentioned this pull request Jul 15, 2019
28 tasks
@marcelklehr marcelklehr force-pushed the marcelklehr-patch-objectstorage-putContent branch from 15448b9 to b3249f9 Compare July 15, 2019 21:37
@marcelklehr marcelklehr deleted the marcelklehr-patch-objectstorage-putContent branch July 15, 2019 21:38
@marcelklehr marcelklehr restored the marcelklehr-patch-objectstorage-putContent branch July 15, 2019 21:38
@marcelklehr marcelklehr deleted the marcelklehr-patch-objectstorage-putContent branch July 15, 2019 21:54
@marcelklehr
Copy link
Member Author

Closed in favor of #16409

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3. to review Waiting for reviews bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants