Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

[Filesystem] Add appendToFile()#20612

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
fabpot merged 1 commit intosymfony:masterfromchalasr:filesystem/append_to_file
Jan 8, 2017

Conversation

@chalasr
Copy link
Member

@chalasrchalasr commentedNov 23, 2016•
edited
Loading

QA
Branch?master
Bug fix?no
New feature?yes
BC breaks?no
Deprecations?no
Tests pass?yes
Fixed ticketsn/a
LicenseMIT
Doc PRtodo

So we could append content to a file:

(newFilesystem)->appendToFile($file,'bar');

instead of doing it in two steps:

if (false ===$content = @file_get_contents($file)) {thrownew \Exception();}(newFilesystem)->dumpFile($file,$content.'bar');

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

ogizanagi, SofHad, tjaari, and ro0NL reacted with thumbs up emoji
@ro0NL
Copy link
Contributor

ro0NL commentedNov 26, 2016•
edited
Loading

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

@chalasr
Copy link
MemberAuthor

@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-grekasnicolas-grekas added this to the3.x milestoneDec 6, 2016
@nicolas-grekas
Copy link
Member

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?

jvasseur reacted with thumbs up emoji

@chalasr
Copy link
MemberAuthor

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 asdumpFile() does, so we could catch a specific exception instead of having to mute an eventual warning and to check the return offopen in user-land code.

Given the wholeFilesystem 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

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch 2 times, most recently from5bef77b to35d33faCompareDecember 16, 2016 08:49
@chalasr
Copy link
MemberAuthor

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

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch 2 times, most recently from88a308c tofc87aa0CompareDecember 17, 2016 10:17
}

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Nice catch, done 👍

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch fromfc87aa0 to35eb1cbCompareDecember 18, 2016 11:51
*/
publicfunctionappendToFile($filename,$content)
{
if (false !==$originalContent = @file_get_contents($filename)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

$content = $originalContent.$content;

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Oops! thanks@ro0NL

if (false !==$originalContent = @file_get_contents($filename)) {
$originalContent ='';
}

Copy link
Contributor

Choose a reason for hiding this comment

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

$filename, $content

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch 2 times, most recently fromc185ecd toec7c144CompareDecember 18, 2016 12:12
@chalasr
Copy link
MemberAuthor

Travis failure is unrelated

* @throws IOException If the file is not writable
*/
publicfunctionappendToFile($filename,$content)
{
Copy link
Contributor

Choose a reason for hiding this comment

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

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 theFILE_APPEND flag offile_put_contents instead?
You could refactor a bit the logic insidedumpFile in order to extract it in a private method adding the flag or not.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

which doesn't look like an atomic append to me

The write is atomic, usingfile_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 usefile_get_contents or doing twofile_put_contents,

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

We could do FILE_APPEND|LOCK_EX

Yeah, I was about to propose it :)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

So what about a publicwriteFile($filename, $content[, $append = false]) then? UsingLOCK_EX by design and letdumpFile forward? That to me would be the simplest implementation.

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Updated

ro0NL reacted with thumbs up emoji
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

AboutappendToFile, I'm still not sure what is better between usingfile_get_contents orfile_put_contents with FILE_APPEND, but I tend to prefer usingfile_get_contents to avoid havingfile_put_contents breaks in the middle of the write..

ogizanagi reacted with thumbs up emoji
* Atomically dumps content into a file.
*
* @param string $filename The file to be written to
* @param string $content The data to write into the file
Copy link
Contributor

Choose a reason for hiding this comment

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

For usingfile_put_contents we technically allowstring|array|resource.

Copy link
MemberAuthor

@chalasrchalasrDec 18, 2016•
edited
Loading

Choose a reason for hiding this comment

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

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, see0fc513e) for changing this docblock? I think it's worth it

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree.

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch fromec7c144 to9908df7CompareDecember 18, 2016 13:29
@ogizanagi
Copy link
Contributor

ogizanagi commentedDec 18, 2016•
edited
Loading

👍

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

Status: Reviewed

ro0NL and chalasr reacted with thumbs up emoji

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch from5681f39 to4e737b4CompareJanuary 6, 2017 18:31
{
$dir =dirname($filename);

if (!is_dir($dir)) {
Copy link
Member

Choose a reason for hiding this comment

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

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)?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch from4e737b4 to9fb4050CompareJanuary 8, 2017 12:38

/**
* Appends content to an existing file.
*
Copy link
Member

Choose a reason for hiding this comment

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

The file to which to append content

@xabbuh
Copy link
Member

👍

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch from9fb4050 to0e52144CompareJanuary 8, 2017 12:53
fabpot added a commit that referenced this pull requestJan 8, 2017
…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/aIn case permissions have been changed meanwhileCommits-------dbc4148 [Filesystem] Check that the directory is writable after created it in dumpFile()
*
* @param string $filename The file to which to append content
* @param string $content The content to append
*
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Reverted the removal of the capital letter

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch from0e52144 to9fb5293CompareJanuary 8, 2017 19:22
@fabpot
Copy link
Member

Can you add a note in the CHANGELOG?

@chalasrchalasrforce-pushed thefilesystem/append_to_file branch from9fb5293 to1f7b753CompareJanuary 8, 2017 20:07
@chalasr
Copy link
MemberAuthor

CHANGELOG updated

@fabpot
Copy link
Member

Thank you@chalasr.

@fabpotfabpot merged commit1f7b753 intosymfony:masterJan 8, 2017
fabpot added a commit that referenced this pull requestJan 8, 2017
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        | todoSo we could append content to a file:```php(new Filesystem)->appendToFile($file, 'bar');```instead of doing it in two steps:```phpif (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()
fabpot added a commit that referenced this pull requestJan 8, 2017
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        | todoSo we could append content to a file:```php(new Filesystem)->appendToFile($file, 'bar');```instead of doing it in two steps:```phpif (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
Copy link
Member

I'm super late to this but I wonder if this method should be calledappend() instead ofappendToFile() ?

@chalasrchalasr deleted the filesystem/append_to_file branchJanuary 8, 2017 20:30
@ogizanagi
Copy link
Contributor

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

But that's only my 2 cents :)

chalasr reacted with thumbs up emoji

@chalasr
Copy link
MemberAuthor

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

@chalasr
Copy link
MemberAuthor

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

@ogizanagi
Copy link
Contributor

ogizanagi commentedJan 8, 2017•
edited
Loading

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 😆

@nicolas-grekasnicolas-grekas modified the milestones:3.x,3.3Mar 24, 2017
@fabpotfabpot mentioned this pull requestMay 1, 2017
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@fabpotfabpotfabpot left review comments

@xabbuhxabbuhxabbuh left review comments

+2 more reviewers

@ro0NLro0NLro0NL left review comments

@ogizanagiogizanagiogizanagi left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

3.3

Development

Successfully merging this pull request may close these issues.

8 participants

@chalasr@ro0NL@nicolas-grekas@ogizanagi@xabbuh@fabpot@javiereguiluz@carsonbot
[8]ページ先頭
©2009-2025 Movatter.jp