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

Handle files with is_file instead of file_exists #27440

Merged
merged 3 commits into from
Oct 23, 2021
Merged

Conversation

solracsf
Copy link
Member

@solracsf solracsf commented Jun 9, 2021

Should fix things like fread(): read of 8192 bytes failed with errno=21 Is a directory since is_file() will return false if the given $path points to a directory, where file_exists() will return true if the given $path points to a valid file or directory.

Since we're expecting files only, this should make sense.

@MichaIng
Copy link
Member

MichaIng commented Jun 9, 2021

Both resolve symlinks, and return false, if the target does not exist, btw:

EDIT: test -e vs test -f in shells, basically.

@solracsf
Copy link
Member Author

solracsf commented Jun 9, 2021

Here is another explanation (just in case...):
https://blog.magepsycho.com/is_file-vs-file_exists/

@solracsf solracsf requested a review from schiessle June 11, 2021 09:29
@szaimen szaimen added this to the Nextcloud 23 milestone Jun 18, 2021
@MichaIng
Copy link
Member

Probably trivial enough, but @acsfer a rebase would be great, so CI checks can run through.

Should fix things like `fread(): read of 8192 bytes failed with errno=21 Is a directory`
@solracsf
Copy link
Member Author

@MichaIng rebased 👍

@MichaIng
Copy link
Member

Tests need to be updated:

1) Test\Files\Storage\Wrapper\EncryptionTest::testGetHeader with data set #0 ('/foo/bar.txt', false, '/foo/bar.txt')
Expectation failed for method name is "file_exists" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

2) Test\Files\Storage\Wrapper\EncryptionTest::testGetHeader with data set #1 ('/foo/bar.txt.part', false, '/foo/bar.txt')
Expectation failed for method name is "file_exists" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

3) Test\Files\Storage\Wrapper\EncryptionTest::testGetHeader with data set #2 ('/foo/bar.txt.ocTransferId7437493.part', false, '/foo/bar.txt')
Expectation failed for method name is "file_exists" when invoked 1 time(s).
Method was expected to be called 1 times, actually called 0 times.

4) Test\Files\Storage\Wrapper\EncryptionTest::testGetHeader with data set #3 ('/foo/bar.txt.part', true, '/foo/bar.txt')
Expectation failed for method name is "readFirstBlock" when invoked 1 time(s)
Parameter 0 for invocation OC\Files\Storage\Wrapper\Encryption::readFirstBlock('/foo/bar.txt.part') does not match expected value.
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'/foo/bar.txt'
+'/foo/bar.txt.part'

@MichaIng
Copy link
Member

There is a second occurrence, but not sure if it is relevant here, let's see what the drone says now: https://github.com/nextcloud/server/blob/is-file-handle/tests/lib/Files/Storage/Wrapper/EncryptionTest.php#L655

@skjnldsv skjnldsv mentioned this pull request Oct 13, 2021
@skjnldsv
Copy link
Member

There were 4 failures:
148 |  
149 | 1) Test\Files\Storage\Wrapper\EncryptionTest::testGetHeaderAddLegacyModule with data set #0 (array('AES-128'), true, true, array('AES-128', 'OC_DEFAULT_MODULE'))
150 | Expectation failed for method name is "file_exists" when invoked 1 time(s).
151 | Method was expected to be called 1 times, actually called 0 times.
152 |  
153 | 2) Test\Files\Storage\Wrapper\EncryptionTest::testGetHeaderAddLegacyModule with data set #1 (array(), true, false, array())
154 | Expectation failed for method name is "file_exists" when invoked 1 time(s).
155 | Method was expected to be called 1 times, actually called 0 times.
156 |  
157 | 3) Test\Files\Storage\Wrapper\EncryptionTest::testGetHeaderAddLegacyModule with data set #2 (array(), true, true, array('OC_DEFAULT_MODULE'))
158 | Failed asserting that actual size 0 matches expected size 1.
159 |  
160 | /drone/src/tests/lib/Files/Storage/Wrapper/EncryptionTest.php:694
161 |  
162 | 4) Test\Files\Storage\Wrapper\EncryptionTest::testGetHeaderAddLegacyModule with data set #3 (array(), false, true, array())
163 | Expectation failed for method name is "file_exists" when invoked 1 time(s).
164 | Method was expected to be called 1 times, actually called 0 times.
165 |  

@skjnldsv skjnldsv added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Oct 22, 2021
@skjnldsv skjnldsv modified the milestones: Nextcloud 23, Nextcloud 24 Oct 22, 2021
@skjnldsv skjnldsv added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels Oct 22, 2021
@skjnldsv skjnldsv modified the milestones: Nextcloud 24, Nextcloud 23 Oct 22, 2021
@skjnldsv skjnldsv merged commit f4e4a85 into master Oct 23, 2021
@skjnldsv skjnldsv deleted the is-file-handle branch October 23, 2021 09:18
@skjnldsv
Copy link
Member

/backport to stable22

@skjnldsv
Copy link
Member

/backport to stable21

@skjnldsv
Copy link
Member

/backport to stable20

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. to release Ready to be released and/or waiting for tests to finish bug feature: encryption (server-side)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants