Skip to content

switch to Files Node API for zip generation - #17822

Merged
blizzz merged 1 commit into
masterfrom
fix/noid/tip-download-to-node-api
Nov 7, 2019
Merged

switch to Files Node API for zip generation#17822
blizzz merged 1 commit into
masterfrom
fix/noid/tip-download-to-node-api

Conversation

@blizzz

Copy link
Copy Markdown
Member

since it is disencouraged to use the static Filesystem methods

@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable17

Comment threadlib/private/legacy/files.php Outdated
$fileTime = \OC\Files\Filesystem::filemtime($file);
$fh = \OC\Files\Filesystem::fopen($file, 'r');
$streamer->addFileFromStream($fh, basename($file), $fileSize, $fileTime);
$owner = \OC\Files\Filesystem::getOwner($file);

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

this shouldn't use the owner but the current user

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

@icewind1991 what about unauthenticated users, on public shares?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

They have the "current user" set correctly afaik, since it's also needed for the old Filesystem stuff to work (worth double testing though)

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

nope, using $userFolder = \OC::$server->getUserFolder(); returns null in that case. Of cours we can try that first and fall back to the owner? Or does it open any loophole? Previously it also only just read it. IIRC it's best to avoid using root folder directly for performance.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

\OC::$server->getRootFolder()->get(Filesystem::getRoot()) would always have the same behavior as using the old api

Signed-off-by: Arthur Schiwon <blizzz@arthur-schiwon.de>
@blizzz
blizzzforce-pushed the fix/noid/tip-download-to-node-api branch from 4852da5 to f9bfd48CompareNovember 7, 2019 08:45
@blizzz

Copy link
Copy Markdown
MemberAuthor

/backport to stable16

@blizzzblizzz added 4. to release Ready to be released and/or waiting for tests to finish and removed 3. to review Waiting for reviews labels Nov 7, 2019
@blizzz
blizzz merged commit 9fe4b95 into masterNov 7, 2019
@blizzz
blizzz deleted the fix/noid/tip-download-to-node-api branch November 7, 2019 10:03
@backportbot-nextcloud

Copy link
Copy Markdown

backport to stable17 in #17840

@backportbot-nextcloud

Copy link
Copy Markdown

backport to stable16 in #17841

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

Labels

4. to releaseReady to be released and/or waiting for tests to finishbugfeature: filestechnical debt🧱 🤔🚀

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants

@blizzz@rullzer@icewind1991