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 problem with zip file creation on Windows #606

Merged
merged 4 commits into from
Apr 30, 2024
Merged

Fix problem with zip file creation on Windows #606

merged 4 commits into from
Apr 30, 2024

Conversation

t-hamano
Copy link
Contributor

@t-hamano t-hamano commented Apr 28, 2024

Fixes #602

This PR fixes an issue where files are not created correctly when using certain file archivers on Windows OS.

Where the problem occurs, the file is generated by ZipArchive::addFromString. The PHP documentation mentions the following:

Note: For maximum portability, it is recommended to always use forward slashes (/) as directory separator in ZIP filenames.

I don't know the exact cause, but I think backslashes are causing problems with certain file archivers.

Testing Instructions

  • Check out this branch on Windows.
  • Download & install 7-Zip.
  • Run "Export Zip" in the Site Editor.
  • Unzip the file using 7-zip.

Screenshot

Before After
image image

admin/create-theme/theme-zip.php Outdated Show resolved Hide resolved
admin/create-theme/theme-zip.php Outdated Show resolved Hide resolved
@nicholasingham
Copy link

Just a thought. The plugin code doesn't use the path_join function at the moment.

Syntax such as $a. '/' . $b or 'pattern/' . $c is used throughout.

For consistency, would it be better to continue to use the . '/' . syntax for this PR and decide whether to change to path_join everywhere later?

@vcanales
Copy link
Member

Just a thought. The plugin code doesn't use the path_join function at the moment.

Syntax such as $a. '/' . $b or 'pattern/' . $c is used throughout.

For consistency, would it be better to continue to use the . '/' . syntax for this PR and decide whether to change to path_join everywhere later?

We're introducing it as a part of another PR as well #601.

There are also places where it might not be suitable to use it ie. where DIRECTORY_SEPARATOR is in use.

I don't see a drawback in introducing it here, and also gradually whenever we need to make changes to path handling logic.

t-hamano and others added 2 commits April 29, 2024 10:54
Co-authored-by: Vicente Canales <1157901+vcanales@users.noreply.github.com>
Co-authored-by: Vicente Canales <1157901+vcanales@users.noreply.github.com>
@nicholasingham
Copy link

Makes sense. Many thanks.

Copy link
Contributor

@pbking pbking left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Took longer to get my windows environment working again then it did to test it (and for just a 2 line change at that 🤣 )

Works as expected. The unpacked zip archive looks as expected.

Thanks for this fix.

@pbking pbking merged commit 35cbb46 into WordPress:trunk Apr 30, 2024
2 checks passed
@t-hamano t-hamano deleted the fix/download-zip-windows branch May 1, 2024 01:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

Problem with zip file creation on Windows
4 participants