Fix invalid paths on Windows #100
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
szaleq
commented
Apr 25, 2024
Comment on lines
+126
to
+128
| } elseif (0 !== strpos($absolutePath, getcwd()) && 1 === preg_match('/.*[\/\\\\]([^\/\\\\]*[\/\\\\][^\/\\\\]*)[\/\\\\][^\/\\\\]*/', $vendorDirectory, $output_array)) { | ||
| $this->relativePath = $output_array[1]; | ||
| } elseif (1 === preg_match('/.*\/([^\/]*\/[^\/]*)\/composer.json/', $composerJsonFileAbsolute, $output_array)) { | ||
| // Not every package gets installed to a folder matching its name (crewlabs/unsplash). | ||
| } elseif (1 === preg_match('/.*[\/\\\\]([^\/\\\\]+[\/\\\\][^\/\\\\]+)[\/\\\\]composer.json/', $composerJsonFileAbsolute, $output_array)) { |
There was a problem hiding this comment.
@BrianHenryIE Only now I realised why the relative paths were not created correctly: the regexes only worked for forward slash paths. Here is the fix to make it work with backslash paths also.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #94
The problem
Not all paths on Windows contain backslash (
\) as a directory separator. Some paths (e.g. vendor dir returned from Composer) still contain trailing forward slash (/). Thats why some code such as here does not work.It produces a path which ends with
/\combo. It wouldn't be a problem in arequirecall etc, because multiple slashes get resolved correctly, so paths likeC:\path\\to/\file.phpget resolved asC:\path\to\file.phpand it would work BUT here there is astr_replacecall which aims to subtract the$sourcePathfrom$sourceAbsoluteFilepathto create a relative path and fails to do so due to the slash-backslash combo presence at the end of the$sourcePath.The solution
The solution is simple: just replace all occurences of one or more slash or backslash combinations with
DIRECTORY_SEPARATORto create a normalized path, just like this one does.Another issue about paths
IDK how it works on MacOS or Linux, but on Windows each path of this
if-elsecondition fails, so the$this->relativePathis alwaysnullfor all packages. It seems like the$vendorDirectoryis something entirely different on different platforms... (??) For me it contains an absolute path of each package's vendor directory, e.g.C:\project-root\vendor\brianhenrieie\strauss\vendor...The solution is to just use a package name as a relative path if anything else failed. On Windows it works like a charm, I also confirmed that it didn't break anything on MacOS. I didn't delve into this much, I don't know all the possible configuration options of composer etc... but couldn't it be used as a relative package name by default? I mean, each package has a name, which has a form of
vendor-name/package-nameand it gets installed into{project-root}/{composer-vendor-dir}/vendor-name/package-name... Isn't it always the case?