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

Directory separator and its duplicates #103

Conversation

BrianHenryIE
Copy link
Contributor

@BrianHenryIE BrianHenryIE commented Dec 14, 2020

Fix for #90 , #43.

The meat in this is trim(preg_replace('/[\/\\\\]+/', DIRECTORY_SEPARATOR, $path), DIRECTORY_SEPARATOR) which is run on every variable in Mover::moveFile() that is to be used in a file path. The provides Windows <-> Unix compatibility and by always trimming the slashes, makes it easier to reason inside the code. $workingDir is the only variable with a leading slash, since it is the absolute path and all others are relative.

No tests written, but existing tests are fine. Marking it as draft for a few weeks while I test it in production!

Two integration tests added.

@BrianHenryIE
Copy link
Contributor Author

BrianHenryIE commented Dec 14, 2020

I think this relies on #91 : Requires #91 for #90 to work, but this PR does not build on #91.

The test for #90 will not pass until #91 is merged.

Previous inheritence required PSR-4 to override anyway.
And rename variables for better understandinf
The integration tests are creating a temptestdir which is deleted after each test is run, but when they fail remains.
@BrianHenryIE
Copy link
Contributor Author

BrianHenryIE commented Dec 14, 2020

It would be great if someone on Windows could test this.

git clone https://github.com/coenjacobs/mozart.git
git remote add brianhenryie git://github.com/brianhenryie/mozart.git
git fetch brianhenryie
git merge brianhenryie/move-files-once-per-autoloader
git merge brianhenryie/DIRECTORY_SEPARATOR-and-its-duplicates
composer install
composer run-script test

I just realised the tests will fail for the same reason we needed DIRECTORY_SEPARATOR!

Source of `$workingDir` is `getcwd()`.
Automated tests pass on MacOS.
Manual test passes on Windows: coenjacobs#90 (comment)
@BrianHenryIE
Copy link
Contributor Author

This looks good with master merged into it and tested on MacOS and on Windows 10 w/ PHP 7.4.

It was on a pretty fresh VM on Parallels so needed:

Interestingly, "DIRECTORY_SEPARATOR is not necessarily needed, PHP always converts / to the appropriate character in its file functions."

To run the tests, I needed.

php vendor\phpunit\phpunit\phpunit

It's failing with PHP 8 in GitHub Actions.

@BrianHenryIE BrianHenryIE marked this pull request as ready for review February 7, 2021 21:10
@BrianHenryIE BrianHenryIE deleted the DIRECTORY_SEPARATOR-and-its-duplicates branch May 3, 2021 02:21
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.

1 participant