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

Fix for specifying a directory in a package when deploying from windows #73

Merged
merged 3 commits into from
May 22, 2022

Conversation

w00key
Copy link
Contributor

@w00key w00key commented May 17, 2022

When using specifying a directory in the package function windows paths becomes a mix of \ and /.

This commit changes the way the Finder class retrieves the path.

Since SplFileInfo::getPathname returns a mixture of paths on windows and creating zips on windows really likes / . I replaced the call to getPathname() with $file->getPath() . '/'. $file->getFilename() both functions on SplFileInfo

This means the \ that gets added between the folder and file in question is always a /.

This change may need testing on other platforms but should be okay.

@w00key w00key changed the title Fix for specifying a direct in a package when deploying from windows Fix for specifying a directory in a package when deploying from windows May 17, 2022
@aarondfrancis
Copy link
Owner

Thanks for continuing to run this down. What a pain!

The only thing I can't quite figure out is if this solves for directory separators in the path? I see that the last one between the path and the directory is replaced, but what about ones in the path?

I'm wondering if we should do something like

str_replace(DIRECTORY_SEPARATOR, "/", $file->getPathname())

that would ensure we get them all, right?

Lemme know what you think about that!

@w00key
Copy link
Contributor Author

w00key commented May 22, 2022

I'm not sure where is the best place for some of these fixes, this first PR looked like the main place however didn't resolve folders, because it looks like the Finder class retrieves the path differently when dealing with folders.

Finder path for the file seemed to be \folders\folders\file.js, the folder PR weirdly is happy I think as long as its /file.js not \file.js presumably to do with the removal of the base path at some point as others have suggested.

So if I'm understanding you correctly, the downstream fix of detecting any \ and fixing for windows could work, I'll test it out.

@w00key
Copy link
Contributor Author

w00key commented May 22, 2022

ok did a quick test, fixing all \ to / at the Finder::yieldSelectedFiles level breaks the path for windows to retrieve the file, same if its applied in Package::prependBasePath from the original fix.

@aarondfrancis
Copy link
Owner

I wonder if we should just do the directory replacement as the file is going into the zip?

That way PHP can always read from the filesystem, and Lambda always ends up with /.

Would that solve it in every case?

@w00key
Copy link
Contributor Author

w00key commented May 22, 2022

Adding your suggested fix to Package::upload does also resolve the issue.

$zip->addFileFromPath(
    str_replace(DIRECTORY_SEPARATOR, '/', $this->removeBasePath($file)), $file, $options
);

Thats for the foreach ($this->files() as $file) { loop.

Likely need to also change the other $zip->addFileFromPath calls in upload, this resolves for using a folder and a path.

You can also revert the previous DIRECTORY_SEPARATOR change along with not using this suggested PR.

I can make a new PR if you want.

@aarondfrancis
Copy link
Owner

Nice! I think this is the way to go. Maybe we add a new method, something like normalizeSeparators or something? And use it wherever we add to the zip?

@w00key
Copy link
Contributor Author

w00key commented May 22, 2022

Ok think I've updated this PR, added normalizeSeparators and its called in upload when adding to the zip. Still new to making PRs but glad to help out.

@aarondfrancis
Copy link
Owner

Thanks for seeing this through! 💪

@aarondfrancis aarondfrancis merged commit df8a9c9 into aarondfrancis:main May 22, 2022
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