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

FileSystem::rename() fix renaming file/directory if only case changes #155

Merged
merged 3 commits into from Feb 6, 2018

Conversation

@Ciki
Copy link
Contributor

Ciki commented Nov 21, 2017

  • bug fix? yes
  • new feature? no
  • BC break? no

When trying to rename file/directory itself in order to change only case of filename (eg. test.php to Test.php), the process fails as the method first deletes the desired file(name) prior to renaming it .. thus it deletes the original file/dir that was about to be renamed.
This change prevents that case.

When trying to rename file/directory itself in order to change only case of filename (eg. `test.php` to `Test.php`), the process fails as the method first [deletes](https://github.com/nette/utils/blob/master/src/Utils/FileSystem.php#L110) the desired file(name) prior to renaming it .. thus it deletes the original file/dir that was about to be renamed.
@dg

This comment has been minimized.

Copy link
Member

dg commented Nov 21, 2017

Good point. However, checking whether the files are the same should be done differently (I don't know how). For compatibility between NTFS and Linux.

@Ciki

This comment has been minimized.

Copy link
Contributor Author

Ciki commented Nov 21, 2017

@dg you think realpath could do the work properly? I added new commit prior to seeing your comment

@dg

This comment has been minimized.

Copy link
Member

dg commented Nov 21, 2017

Maybe compare stat?

@Ciki

This comment has been minimized.

Copy link
Contributor Author

Ciki commented Nov 22, 2017

I think the realpath should do the work properly but I am no expert on this. Any use case where it might break? I can add the stat comparison but not sure when it can fail and return false thus skipping the removal of dest file (eg. when FALSE is returned for both different source and dest file)

@dg

This comment has been minimized.

Copy link
Member

dg commented Nov 23, 2017

Realpath should do the work, I didn’t test it. It this case there shouldn’t be lower().

…name()
@Ciki

This comment has been minimized.

Copy link
Contributor Author

Ciki commented Nov 23, 2017

@dg updated

@dg

This comment has been minimized.

Copy link
Member

dg commented Nov 24, 2017

Did you test it? I am not sure that realpath really works this way.

@Ciki

This comment has been minimized.

Copy link
Contributor Author

Ciki commented Nov 26, 2017

@dg I did.. on OS preventing same filenames differing in case only, like Windows, realpath correctly transforms eg Test.php and teSt.php to test.php if test.php exists. On the other hand on Linux, where filenames can differ in case, it returns the original and correct names. As far as I can tell and test, it works correctly

@dg

This comment has been minimized.

Copy link
Member

dg commented Nov 28, 2017

Great. But it seems that it breaks test…

@Ciki

This comment has been minimized.

Copy link
Contributor Author

Ciki commented Dec 11, 2017

@dg should I remove the failing test altogether? As the failing rename method cannot be actually tested IMO - according to php.net spec If renaming a directory and newname exists, this function will emit a warning. but this case never occurs

@dg dg merged commit f12927d into nette:master Feb 6, 2018
1 check failed
1 check failed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
dg added a commit that referenced this pull request Feb 6, 2018
@dg

This comment has been minimized.

Copy link
Member

dg commented Feb 6, 2018

Thanks, merged

dg added a commit that referenced this pull request Feb 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.