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 create SimpleFile with empty content #21386

Closed
wants to merge 1 commit into from
Closed

Fix create SimpleFile with empty content #21386

wants to merge 1 commit into from

Conversation

d0niek
Copy link

@d0niek d0niek commented Jun 12, 2020

After that change #19493 where
you have to provide content to method newFile is a bug with empty ''
content.

Php function file_put_contents will returns the numbers of bytes that
written to the file, or FALSE on failure
https://www.php.net/manual/en/function.file-put-contents.php

So for empty '' content it will return 0 which is true for this
statement:

if (!$result) {
    throw new NotPermittedException('Could not create path');
}

From now empty content will not throws an exception.

After that change #19493 where
you have to provide content to method `newFile` is a bug with empty `''`
content.

Php function `file_put_contents` will returns the numbers of bytes that
written to the file, or `FALSE` on failure
https://www.php.net/manual/en/function.file-put-contents.php

So for empty `''` content it will return `0` which is `true` for this
statement:

```
if (!$result) {
    throw new NotPermittedException('Could not create path');
}
```

From now empty content will not throws an exception.
@kesselb
Copy link
Contributor

kesselb commented Jun 12, 2020

Thanks 👍

That looks danger. If the underlying storage is an object store for example the result might be true instead of 0. The storage adapter "local" should be changed to follow the interface and return a boolean always.

public function file_put_contents($path, $data) {
return file_put_contents($this->getSourcePath($path), $data);
}

@kesselb kesselb requested review from icewind1991 and rullzer June 12, 2020 20:58
@kesselb kesselb added 3. to review Waiting for reviews bug labels Jun 12, 2020
@kesselb kesselb added this to the Nextcloud 20 milestone Jun 12, 2020
Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Looks good.
Would you mind also adding a test case?

Copy link
Member

@rullzer rullzer left a comment

Choose a reason for hiding this comment

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

Ah woops. Yes @kesselb is right. Please modify the local storage to behave properly.

@d0niek
Copy link
Author

d0niek commented Jun 15, 2020

The same "problem" is here:

public function file_put_contents($path, $data) {
$handle = $this->fopen($path, "w");
$this->removeCachedFile($path);
$count = fwrite($handle, $data);
fclose($handle);
return $count;
}

Implementation is not Ok with the interface:
/**
* see http://php.net/manual/en/function.file_put_contents.php
*
* @param string $path
* @param string $data
* @return bool
* @since 6.0.0
*/
public function file_put_contents($path, $data);
.
I can fix it but is it Ok? Origin file_put_contents return how much bytes were written and maybe someone wants to compare it with data sent to the method?

What about test case, I can write it too but there aren't any test cases with content so should I write them too?

@MorrisJobke MorrisJobke mentioned this pull request Aug 11, 2020
57 tasks
@MorrisJobke
Copy link
Member

Another fix went into master already: #22237

@MorrisJobke MorrisJobke removed this from the Nextcloud 20 milestone Aug 20, 2020
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