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.

Sign up for GitHub

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

Jump to bottom

fix using FSEEK_END with SeekableHttpStream to get file size #33718

Merged
merged 1 commit into from
Sep 15, 2022
Merged

fix using FSEEK_END with SeekableHttpStream to get file size #33718

merged 1 commit into from
Sep 15, 2022

Conversation

icewind1991
Copy link
Member

Delays the reconnect on fseek to allow fseek(0, FSEEK_END) to return successfully.

This is used by some libraries to get the filesize from stream.

Adds tests to ensure that this method can be used for all storage backends.

@icewind1991 icewind1991 added the 3. to review Waiting for reviews label Aug 26, 2022
@icewind1991 icewind1991 added this to the Nextcloud 25 milestone Aug 26, 2022
@icewind1991 icewind1991 mentioned this pull request Aug 26, 2022
Merged
PVince81
PVince81 approved these changes Aug 29, 2022
View reviewed changes
Copy link
Member

@PVince81 PVince81 left a comment

Choose a reason for hiding this comment

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

👍

@szaimen
Copy link
Contributor

szaimen commented Aug 29, 2022

Seems like some tests fail?

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Aug 29, 2022
@icewind1991 icewind1991 force-pushed the seekable-http-fseek-end branch from 66a5067 to a5ad9a1 Compare August 29, 2022 14:54
@icewind1991
Copy link
Member Author

Improved the typings around current a bit

@come-nc
Copy link
Contributor

come-nc commented Aug 29, 2022

You need to run composer run cs:fix

come-nc
come-nc approved these changes Aug 29, 2022
View reviewed changes
Copy link
Contributor

@come-nc come-nc left a comment

Choose a reason for hiding this comment

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

Apart from codesniffer and psalm fixes this now looks good.

@icewind1991 icewind1991 force-pushed the seekable-http-fseek-end branch from a5ad9a1 to 841e361 Compare August 29, 2022 17:11
@blizzz blizzz mentioned this pull request Aug 30, 2022
Merged
@icewind1991 icewind1991 force-pushed the seekable-http-fseek-end branch from 841e361 to b65b837 Compare August 30, 2022 18:10
@PVince81
Copy link
Member

object store azure fails with:

1) Test\Files\ObjectStore\AzureTest::testFseekSize
fseek(): stream does not support seeking

/drone/src/tests/lib/Files/ObjectStore/ObjectStoreTest.php:156

This was referenced Sep 6, 2022
Merged
Merged
@skjnldsv skjnldsv mentioned this pull request Sep 15, 2022
Merged
@icewind1991 icewind1991 force-pushed the seekable-http-fseek-end branch from b65b837 to 255bcd0 Compare September 15, 2022 15:16
@icewind1991
Copy link
Member Author

Disabled the test in azure for now and will look into fixing azure seek support in a separate PR

@PVince81 PVince81 added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Sep 15, 2022
@icewind1991
fix using FSEEK_END with SeekableHttpStream to get file size
d8961ed 
Signed-off-by: Robin Appelman <robin@icewind.nl>
@icewind1991 icewind1991 force-pushed the seekable-http-fseek-end branch from 255bcd0 to d8961ed Compare September 15, 2022 15:23
@PVince81 PVince81 merged commit 0023a10 into master Sep 15, 2022
@PVince81 PVince81 deleted the seekable-http-fseek-end branch September 15, 2022 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc approved these changes

@PVince81 PVince81 PVince81 approved these changes

@artonge artonge Awaiting requested review from artonge

@juliusknorr juliusknorr Awaiting requested review from juliusknorr

@CarlSchwan CarlSchwan Awaiting requested review from CarlSchwan

Assignees
No one assigned
Labels
4. to release Ready to be released and/or waiting for tests to finish
Projects
None yet
Milestone
Nextcloud 25
Development

Successfully merging this pull request may close these issues.
4 participants
@icewind1991 @szaimen @come-nc @PVince81