Skip to content

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

Merged
dg merged 3 commits into
nette:masterfrom
Ciki:patch-1
Feb 6, 2018
Merged

FileSystem::rename() fix renaming file/directory if only case changes#155
dg merged 3 commits into
nette:masterfrom
Ciki:patch-1

Conversation

@Ciki

@Ciki Ciki commented Nov 21, 2017

Copy link
Copy Markdown
Contributor
  • 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

dg commented Nov 21, 2017

Copy link
Copy Markdown
Member

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

Ciki commented Nov 21, 2017

Copy link
Copy Markdown
Contributor Author

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

@dg

dg commented Nov 21, 2017

Copy link
Copy Markdown
Member

Maybe compare stat?

@Ciki

Ciki commented Nov 22, 2017

Copy link
Copy Markdown
Contributor Author

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

dg commented Nov 23, 2017

Copy link
Copy Markdown
Member

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

@Ciki

Ciki commented Nov 23, 2017

Copy link
Copy Markdown
Contributor Author

@dg updated

@dg

dg commented Nov 24, 2017

Copy link
Copy Markdown
Member

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

@Ciki

Ciki commented Nov 26, 2017

Copy link
Copy Markdown
Contributor Author

@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

dg commented Nov 28, 2017

Copy link
Copy Markdown
Member

Great. But it seems that it breaks test…

@Ciki

Ciki commented Dec 11, 2017

Copy link
Copy Markdown
Contributor Author

@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
@dg

dg commented Feb 6, 2018

Copy link
Copy Markdown
Member

Thanks, merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants