[2.7][Filesystem] Changed dumpFile to allow dumping to streams... #14580
Conversation
…. Changed dumpFile to allow dumping to streams.
| $tmpFile = false; | ||
| } | ||
| } | ||
| break; |
There was a problem hiding this comment.
Personally I find this method very confusing because of the way the code is structured. I would suggest to omit the break in the end and put it in a guard clause and try to avoid nesting if statements.
Please note the code style as well :)
There was a problem hiding this comment.
Hi @iltar, sure makes sense, will streamline.
Regarding the code style, are you able to be more specific? I did try to be careful with spacing, variable name and conditions etc. Is it just a case of the things you've already mentioned (simplify, guards, condition nesting)?
Thanks
There was a problem hiding this comment.
I was mainly referring to the style itself. Usually fabbot.io gives you a patch to fix it, but it seems to not work for some reason. Try using this: http://cs.sensiolabs.org/ I think this is exactly what fabbot.io uses.
There was a problem hiding this comment.
@iltar Ah possibly spacing between blocks. Will update that too. Thanks
There was a problem hiding this comment.
@iltar. Gotcha.
…errachy parsing into private methods
|
@fabpot. fabbot.io broken on this PR? |
| * | ||
| * @return string The new temporary filename (with path), or FALSE on failure. | ||
| */ | ||
| public function tempNam($dir, $prefix) |
There was a problem hiding this comment.
This is a new feature, it should be documented as such.
But do we really want/require it?
There was a problem hiding this comment.
Happy to add to symfony-docs if this feature is wanted.
Without a stream aware version of tempnam it is very difficult to write to a stream atomically. The path of least resistance becomes writing to the stream non-atomically using file_put_contents, circumventing the benefit of dumpFile. See #10018.
There was a problem hiding this comment.
Pleas also add a line in the CHANGELOG of the component.
I'm not convinced by this implementation. What about:
do {
$tmpnam = $dir.'/'.$prefix.uniqid(mt_rand(), true);
} while (file_exists($tmpnam));
return $tmpnam;There was a problem hiding this comment.
@nicolas-grekas. Thanks for the feedback.
Change log: Will do.
Optimisation, agreed, speed more important than using OS's method of name generation. Will update.
Ta
…andards. Reverted cahnges to dumpFile. Added to CHANGELOG.md and README.md
|
Closing this and reopening on #14754:
|
...as rename can only work where $oldname and $newname have the same streams wrappers.
Added tempNam, stream wrapper aware version of tempnam(), to do the heavy lifting.