[Filesystem] Add appendToFile() #20612

chalasr · 2016-11-23T21:06:43Z

Q	A
Branch?	master
Bug fix?	no
New feature?	yes
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	n/a
License	MIT
Doc PR	todo

So we could append content to a file:

(new Filesystem)->appendToFile($file, 'bar');

instead of doing it in two steps:

if (false === $content = @file_get_contents($file)) {
    throw new \Exception();
}

(new Filesystem)->dumpFile($file, $content.'bar');

Doing it opt-in using dumpFile(..., $append = false) would have been enough but not possible for BC.

ro0NL · 2016-11-26T10:22:58Z

We could also go with readFile / writeFile maybe? Where writeFile allows some tweaking to append, prepend or set/dump/overwrite/replace.

chalasr · 2016-11-26T12:04:42Z

@ro0NL We could and it would make sense imho. Though I'm comfortable with what is proposed here because it's something I actually needed a lot of times, I have concrete use cases for it.
Feel free to try your approach, I would be 👍 . Thanks for sharing

nicolas-grekas · 2016-12-12T10:35:39Z

The implementation looks very fragile to me: it's not atomic at all; why should it fail when the file doesn't exist?
I think that this should behave more like fopen+a mode.
But then why should we need a method to do that when fopen+a mode is really fine?

chalasr · 2016-12-12T11:18:53Z

why should it fail when the file doesn't exist?

You're right, it should not fail by default, but I think it should be opt-in for cases where the content must not be appended if the file doesn't already exist.

But then why should we need a method to do that when fopen+a mode is really fine?

To ease error handling as dumpFile() does, so we could catch a specific exception instead of having to mute an eventual warning and to check the return of fopen in user-land code.

Given the whole Filesystem api eases performing such common tasks on the filesystem, I think it's relevant to add this one too, avoiding the need to write this logic yourself and mixing the use of this api with native functions

chalasr · 2016-12-16T09:16:10Z

Removed the exception when the file doesn't exist. Now this behaves like fopen +a but writing to the file and throwing exceptions on failure.

ro0NL · 2016-12-17T20:35:09Z

src/Symfony/Component/Filesystem/Filesystem.php

+
+        $tmpFile = $this->tempnam(dirname($filename), basename($filename));
+
+        if (false === $originalContent = @file_get_contents($filename)) {


What about forwarding to dumpFile instead in this case? So above checks can be removed..

Nice catch, done 👍

ro0NL · 2016-12-18T12:07:38Z

src/Symfony/Component/Filesystem/Filesystem.php

+    public function appendToFile($filename, $content)
+    {
+        if (false !== $originalContent = @file_get_contents($filename)) {
+            $originalContent = '';


$content = $originalContent.$content;

Oops! thanks @ro0NL

ro0NL · 2016-12-18T12:07:49Z

src/Symfony/Component/Filesystem/Filesystem.php

+            $originalContent = '';
+        }
+
+        return $this->dumpFile($filename, $originalContent.$content);


$filename, $content

chalasr · 2016-12-18T12:26:04Z

Travis failure is unrelated

ogizanagi · 2016-12-18T12:30:06Z

src/Symfony/Component/Filesystem/Filesystem.php

+     */
+    public function appendToFile($filename, $content)
+    {
+        if (false !== $originalContent = @file_get_contents($filename)) {


This now loads the current file content into memory (which doesn't look like an atomic append to me). Wouldn't it be better to rely on the FILE_APPEND flag of file_put_contents instead?
You could refactor a bit the logic inside dumpFile in order to extract it in a private method adding the flag or not.

which doesn't look like an atomic append to me

The write is atomic, using file_put_contents for the read operation would not make it more atomic to me, do I miss something?

You could refactor a bit the logic inside dumpFile in order to extract it in a private method adding the flag or not.

It was my first intention, but would you avoid creating a tmpfile for append? Otherwise we still have to use file_get_contents or doing two file_put_contents,

We could do FILE_APPEND|LOCK_EX. Basically creating the tmp file is about forcing an exclusive lock right?

We could do FILE_APPEND|LOCK_EX

Yeah, I was about to propose it :)

Im not an filesystem expert.. but the tmp file in the middle feels like a poor man solution to me.. so perhaps LOCK_EX is the way to go indeed for dumpeFile as well.

using file_put_contents for the read operation would not make it more atomic to me

Never told you to do so ^^'

We could do FILE_APPEND|LOCK_EX

That's basically what I meant.

So what about a public writeFile($filename, $content[, $append = false]) then? Using LOCK_EX by design and let dumpFile forward? That to me would be the simplest implementation.

But a private function in the middle allowing this API (appendToFile and dumpFile) is also fine 👍 i guess.

In fact, using LOCK_EX doesn't make file_put_contents atomic, so in both cases (using file_get_contents/file_put_contents or file_put_contents with flags) the append would not be atomic. Considering this I prefer to let dumpFile() as it is since the behavior is better (maybe not really atomic, but more than using file_put_contents directly).
Refs:
#10018 (comment)
http://stackoverflow.com/questions/4899737/should-lock-ex-on-both-read-write-be-atomic

About appendToFile, I'm still not sure what is better between using file_get_contents or file_put_contents with FILE_APPEND, but I tend to prefer using file_get_contents to avoid having file_put_contents breaks in the middle of the write..

ro0NL · 2016-12-18T13:14:43Z

src/Symfony/Component/Filesystem/Filesystem.php

@@ -624,7 +624,7 @@ public function tempnam($dir, $prefix)
     * @param string $filename The file to be written to
     * @param string $content  The data to write into the file


For using file_put_contents we technically allow string|array|resource.

I agree, updated on the two newly added methods.
Since this targets master, would you mind to open a PR against 2.8 (it doesn't handle streams in 2.7, see 0fc513e) for changing this docblock? I think it's worth it

ogizanagi · 2016-12-18T13:55:15Z

👍

We just need to validate there isn't any downside about using LOCK_EX instead of the previous implementation (what were the reasons behind the original implementation).

Status: Reviewed

xabbuh · 2017-01-08T12:21:01Z

src/Symfony/Component/Filesystem/Filesystem.php

+        $dir = dirname($filename);
+
+        if (!is_dir($dir)) {
+            $this->mkdir($dir);


Don't we have to check here too that the directory is writable after we created it (the directory might have been created by another process)?

Indeed, done. The same could be applied on dumpFile() I think

Yes, I think you are right. Should be done on 2.7 then as bug fix IMO.

xabbuh · 2017-01-08T12:52:37Z

src/Symfony/Component/Filesystem/Filesystem.php

@@ -649,6 +649,31 @@ public function dumpFile($filename, $content)
    }

    /**
+     * Appends content to an existing file.
+     *
+     * @param string $filename The file for which to append content


The file to which to append content

xabbuh · 2017-01-08T12:53:16Z

👍

…d it in dumpFile() (chalasr) This PR was merged into the 2.7 branch. Discussion ---------- [Filesystem] Check that directory is writable after created it in dumpFile() | Q | A | ------------- | --- | Branch? | 2.7 | Bug fix? | yes | New feature? | no | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | #20612 (comment) | License | MIT | Doc PR | n/a In case permissions have been changed meanwhile Commits ------- dbc4148 [Filesystem] Check that the directory is writable after created it in dumpFile()

fabpot · 2017-01-08T16:08:15Z

src/Symfony/Component/Filesystem/Filesystem.php

+     * @param string $filename The file to which to append content
+     * @param string $content  The content to append
+     *
+     * @throws IOException If the file is not writable


If you remove the capital letter above, you should do the same here, right?

Reverted the removal of the capital letter

fabpot · 2017-01-08T19:58:11Z

Can you add a note in the CHANGELOG?

chalasr · 2017-01-08T20:08:06Z

CHANGELOG updated

fabpot · 2017-01-08T20:18:19Z

Thank you @chalasr.

This PR was merged into the 3.3-dev branch. Discussion ---------- [Filesystem] Add appendToFile() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | todo So we could append content to a file: ```php (new Filesystem)->appendToFile($file, 'bar'); ``` instead of doing it in two steps: ```php if (false === $content = @file_get_contents($file)) { throw new \Exception(); } (new Filesystem)->dumpFile($file, $content.'bar'); ``` Doing it opt-in using `dumpFile(..., $append = false)` would have been enough but not possible for BC. Commits ------- 9fb5293 [Filesystem] Add appendToFile()

This PR was merged into the 3.3-dev branch. Discussion ---------- [Filesystem] Add appendToFile() | Q | A | ------------- | --- | Branch? | master | Bug fix? | no | New feature? | yes | BC breaks? | no | Deprecations? | no | Tests pass? | yes | Fixed tickets | n/a | License | MIT | Doc PR | todo So we could append content to a file: ```php (new Filesystem)->appendToFile($file, 'bar'); ``` instead of doing it in two steps: ```php if (false === $content = @file_get_contents($file)) { throw new \Exception(); } (new Filesystem)->dumpFile($file, $content.'bar'); ``` Doing it opt-in using `dumpFile(..., $append = false)` would have been enough but not possible for BC. Commits ------- 1f7b753 [Filesystem] Add appendToFile()

javiereguiluz · 2017-01-08T20:26:09Z

I'm super late to this but I wonder if this method should be called append() instead of appendToFile() ?

ogizanagi · 2017-01-08T20:33:37Z

I'm fine with the appendToFile name. First because of the dumpFile method. Secondly because this method is on the Filesystem class, and you're not appending anything to the filesystem.

But that's only my 2 cents :)

chalasr · 2017-01-08T20:34:49Z

@javiereguiluz I hesitated for append() and I think it could be misleading e.g. append files to a directory. appendToFile() is clear, at least :)

chalasr · 2017-01-08T20:39:48Z

@fabpot I think there is something wrong with your git tool, commits are merged twice, see the master log

ogizanagi · 2017-01-08T20:46:49Z

Commits' contents aren't even the same... 😅
First merged one is missing your CHANGELOG update, like if it the old commit reference was used. I bet on a missing fetch (somehow, but I don't know how the tool works behind the scene) :)

Also, the merge commit date is before the merge itself 😆

carsonbot added Status: Needs Review Filesystem Feature labels Nov 23, 2016

chalasr force-pushed the filesystem/append_to_file branch from 95aec8b to 0e207b0 Compare November 23, 2016 21:27

nicolas-grekas added this to the 3.x milestone Dec 6, 2016

chalasr force-pushed the filesystem/append_to_file branch 2 times, most recently from 5bef77b to 35d33fa Compare December 16, 2016 08:49

chalasr force-pushed the filesystem/append_to_file branch 2 times, most recently from 88a308c to fc87aa0 Compare December 17, 2016 10:17

ro0NL reviewed Dec 17, 2016

View reviewed changes

chalasr force-pushed the filesystem/append_to_file branch from fc87aa0 to 35eb1cb Compare December 18, 2016 11:51

ro0NL reviewed Dec 18, 2016

View reviewed changes

chalasr force-pushed the filesystem/append_to_file branch 2 times, most recently from c185ecd to ec7c144 Compare December 18, 2016 12:12

ogizanagi reviewed Dec 18, 2016

View reviewed changes

ro0NL reviewed Dec 18, 2016

View reviewed changes

chalasr force-pushed the filesystem/append_to_file branch from ec7c144 to 9908df7 Compare December 18, 2016 13:29

carsonbot added Status: Reviewed and removed Status: Needs Review labels Dec 18, 2016

chalasr force-pushed the filesystem/append_to_file branch from 9908df7 to 2e21cf5 Compare December 18, 2016 14:11

ro0NL mentioned this pull request Dec 18, 2016

[Filesystem] Updated phpdoc on allowed types of content #20980

Closed

chalasr force-pushed the filesystem/append_to_file branch 2 times, most recently from 286de84 to 86fe81a Compare December 18, 2016 15:21

chalasr force-pushed the filesystem/append_to_file branch from 5681f39 to 4e737b4 Compare January 6, 2017 18:31

xabbuh reviewed Jan 8, 2017

View reviewed changes

chalasr force-pushed the filesystem/append_to_file branch from 4e737b4 to 9fb4050 Compare January 8, 2017 12:38

xabbuh reviewed Jan 8, 2017

View reviewed changes

chalasr force-pushed the filesystem/append_to_file branch from 9fb4050 to 0e52144 Compare January 8, 2017 12:53

chalasr mentioned this pull request Jan 8, 2017

[Filesystem] Check that directory is writable after created it in dumpFile() #21200

Merged

fabpot reviewed Jan 8, 2017

View reviewed changes

chalasr force-pushed the filesystem/append_to_file branch from 0e52144 to 9fb5293 Compare January 8, 2017 19:22

[Filesystem] Add appendToFile()

1f7b753

chalasr force-pushed the filesystem/append_to_file branch from 9fb5293 to 1f7b753 Compare January 8, 2017 20:07

fabpot merged commit 1f7b753 into symfony:master Jan 8, 2017

xabbuh mentioned this pull request Jan 8, 2017

[Filesystem] document the new appendToFile() method symfony/symfony-docs#7341

Closed

chalasr deleted the filesystem/append_to_file branch January 8, 2017 20:30

nicolas-grekas modified the milestones: 3.x, 3.3 Mar 24, 2017

fabpot mentioned this pull request May 1, 2017

Release v3.3.0-BETA1 #22603

Merged


		$tmpFile = $this->tempnam(dirname($filename), basename($filename));

		if (false === $originalContent = @file_get_contents($filename)) {

		@@ -624,7 +624,7 @@ public function tempnam($dir, $prefix)
		* @param string $filename The file to be written to
		* @param string $content The data to write into the file

[Filesystem] Add appendToFile() #20612

[Filesystem] Add appendToFile() #20612

Conversation

chalasr commented Nov 23, 2016 • edited Loading

ro0NL commented Nov 26, 2016 • edited Loading

chalasr commented Nov 26, 2016

nicolas-grekas commented Dec 12, 2016

chalasr commented Dec 12, 2016

chalasr commented Dec 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr commented Dec 18, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chalasr Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ogizanagi commented Dec 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xabbuh commented Jan 8, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented Jan 8, 2017

chalasr commented Jan 8, 2017

fabpot commented Jan 8, 2017

javiereguiluz commented Jan 8, 2017

ogizanagi commented Jan 8, 2017

chalasr commented Jan 8, 2017

chalasr commented Jan 8, 2017

ogizanagi commented Jan 8, 2017 • edited Loading

chalasr commented Nov 23, 2016 •

edited

Loading

ro0NL commented Nov 26, 2016 •

edited

Loading

chalasr Dec 18, 2016 •

edited

Loading

ogizanagi commented Dec 18, 2016 •

edited

Loading

ogizanagi commented Jan 8, 2017 •

edited

Loading