8000 FileSystem::copy() with $override = false can still override (overwrite) the target file · Issue #12937 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

FileSystem::copy() with $override = false can still override (overwrite) the target file #12937

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.

Sign up for GitHub

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

Jump to bottom
Closed
borekb opened this issue Dec 10, 2014 · 10 comments
Closed

FileSystem::copy() with $override = false can still override (overwrite) the target file #12937

borekb opened this issue Dec 10, 2014 · 10 comments
Labels
Enhancement Filesystem

Comments

@borekb
Copy link
borekb commented Dec 10, 2014

The API of FileSystem::copy() is unintuitive and its docs are probably flawed. It says:

@param bool    $override   Whether to override an existing file or not

But that's not true, even if $override is false the target might be overwritten if it's older than origin. Generally, I would expect

$fs->copy($origin, $target, false);

not to overwrite my $target but it does. Either the docs should make this very clear or the behavior should ideally be changed but that would probably mean a breaking change. Anyway, if #10547 gets a go ahead I think that the time comparison "magic" should be removed or at least hidden behind some appropriately named parameter.
@SenseException
Copy link
Contributor

@borekb You sure mean that part of code in Filesystem

I agree that setting $override to false should mean, that the existing target file cannot be overridden.
On the other hand, maybe there can be 3 options/conditions for overriding:

  • Override
  • Override if source file is older than target
  • Never override

@borekb
Copy link
Author
borekb commented Dec 10, 2014

Yes, that line, and as you've correctly pointed out there are actually three states. As a quick fix, I'd suggest updating the docs to make it very clear that $override = false might still override the target. (Isn't overwrite the right word?)

@jakzal
Copy link
Contributor
jakzal commented Dec 22, 2014

What do you think of changing:

Whether to override an existing file or not

to

Whether to force overwriting of an existing file or not

?

@borekb
Copy link
Author
borekb commented Dec 22, 2014

It's better but I would probably still need to look at the source code to find out what the method actually does. I'm trying to come up with a good method description but it's kind of hard as this method is really a bit overloaded. Something like this maybe?

Copies a file.

The target is always overwritten if origin is newer (safe-guard the call to this method
with exists() if you don't want this behavior). If origin is not newer then the $override
parameter decides the behavior.

@jakzal
Copy link
Contributor
jakzal commented Dec 22, 2014

need to look at the source code to find out what the method actually does

That's good. We shouldn't duplicate in comments what's already in the code.

@borekb
Copy link
Author
borekb commented Dec 23, 2014

It my eyes, a state where this method had no comments and the third parameter was called $x would be better because I would need to go to the source code and find out what it does. Currently, this method seems like an obvious one but in fact isn't.

Ultimately, I think the problem is a design one where the method should not do any time comparisons in the first place but if it does already and it's hard to change it because of possible BCs, then I think doc comments are actually very important and it should be clear from the what the behavior is.

(BTW this is not an academic discussion, it cause us trouble because we misunderstood what the copy() method does, and I think it's quite easy to misunderstand that a boolean actually represents three states here.)

@stof
Copy link
Member
stof commented Jan 18, 2015

Can you send a PR updating the phpdoc ?

@borekb
Copy link
Author
borekb commented Jan 18, 2015

So is the method description suggested in #12937 (comment) OK? If so, I'll open a PR.

@rgomezcasas
Copy link

Hey, I think override must be boolean, overrides or not, but never depending on the last modification date...

@javiereguiluz
Copy link
Member

For what is worth, PHP's copy() function always overrides:

php_copy

@javiereguiluz javiereguiluz mentioned this issue Mar 16, 2016
Closed
nicolas-grekas added a commit that referenced this issue Mar 16, 2016
@nicolas-grekas
minor #18195 Improved the PHPdoc of FileSystem::copy() (javiereguiluz)
56624e6 
This PR was squashed before being merged into the 2.3 branch (closes #18195).

Discussion
----------

Improved the PHPdoc of FileSystem::copy()

| Q             | A
| ------------- | ---
| Branch        | 2.3+
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #12937
| License       | MIT
| Doc PR        | -

Commits
-------

9ad67ca Improved the PHPdoc of FileSystem::copy()
@xabbuh xabbuh closed this as completed Mar 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Enhancement Filesystem
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
7 participants
@javiereguiluz @borekb @jakzal @stof @SenseException @rgomezcasas @xabbuh
0