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 #16409

Closed
wants to merge 1 commit into from

Conversation

marcelklehr
Copy link
Member

This is the same as #16397 but with a signed off commit.

Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@kesselb kesselb added this to the Nextcloud 17 milestone Jul 15, 2019
@kesselb kesselb added 3. to review Waiting for reviews bug labels Jul 15, 2019
@kesselb kesselb requested review from icewind1991 and rullzer July 15, 2019 21:55
@MorrisJobke MorrisJobke mentioned this pull request Jul 15, 2019
28 tasks
@kesselb
Copy link
Contributor

kesselb commented Jul 15, 2019

Thanks again 👍

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).

@korelstar
Copy link
Member

@MorrisJobke
Please backport this to Nextcloud 15 and 16, too.

@rullzer
Copy link
Member

rullzer commented Jul 17, 2019

It looks sane indeed.
But there are a lot of failing tests.

@marcelklehr marcelklehr deleted the fix/objectstorage-put-contents branch July 17, 2019 13:00
@marcelklehr
Copy link
Member Author

Superseded by #16440 (finally from my own fork, so I can force-push)

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.

4 participants