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.

Sign up for GitHub

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

Jump to bottom

Handle zips with Windoze directory separators (\) rather than the standard linux (/) #3482

Merged
merged 3 commits into from
Mar 31, 2023
Merged

Handle zips with Windoze directory separators (\) rather than the standard linux (/) #3482

merged 3 commits into from
Mar 31, 2023

Conversation

MarkBaker
Copy link
Member

This is:

  • a bugfix
  • a new feature
  • refactoring
  • additional unit tests

Checklist:

  • Changes are covered by unit tests
    • Changes are covered by existing unit tests
    • New unit tests have been added
  • Code style is respected
  • Commit message explains why the change is made (see https://github.com/erlang/otp/wiki/Writing-good-commit-messages)
  • CHANGELOG.md contains a short summary of the change and a link to the pull request if applicable
  • Documentation is updated as necessary

Why this change is needed?

Issue #3472

The sample file uses Windows directory separators rather than linux separators in the zip, so files can't be found in the archive

@MarkBaker
Copy link
Member Author

Needs Tests still

@MarkBaker MarkBaker force-pushed the Xlsx-Reader_Windows-Folder-Separator-in-Zip branch from e72a269 to fea38ef Compare March 24, 2023 07:24
Fighter456
Fighter456 suggested changes Mar 24, 2023
View reviewed changes
Copy link

@Fighter456 Fighter456 left a comment

Choose a reason for hiding this comment

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

Trapped about this during development. Seems to work properly while applying those changes manually.

I've fixed some typos.

src/PhpSpreadsheet/Reader/Xlsx.php
if ($contents === false) {
$contents = $archive->getFromName(substr($fileName, 1), 0, ZipArchive::FL_NOCASE);
}

// Has the file been saved with Windoze directory separators rather than unix?
Copy link

Choose a reason for hiding this comment

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

Suggested change
// Has the file been saved with Windoze directory separators rather than unix?
// Has the file been saved with Windows directory separators rather than unix?

src/PhpSpreadsheet/Shared/File.php
@@ -156,7 +156,11 @@ public static function assertFile(string $filename, string $zipMember = ''): voi
if ($zipMember !== '') {
$zipfile = "zip://$filename#$zipMember";
if (!self::fileExists($zipfile)) {
throw new ReaderException("Could not find zip member $zipfile");
// Has the file been saved with Windoze directory separators rather than unix?
Copy link

Choose a reason for hiding this comment

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

Suggested change
// Has the file been saved with Windoze directory separators rather than unix?
// Has the file been saved with Windows directory separators rather than unix?

src/PhpSpreadsheet/Shared/File.php
return true;
}

// Has the file been saved with Windoze directory separators rather than unix?
Copy link

Choose a reason for hiding this comment

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

Suggested change
// Has the file been saved with Windoze directory separators rather than unix?
// Has the file been saved with Windows directory separators rather than unix?

@MarkBaker
Copy link
Member Author

MarkBaker commented Mar 24, 2023 •
edited
Loading

Trapped about this during development. Seems to work properly while applying those changes manually.

I still don't know how this file was created. All versions of Excel itself have always used the unix /, as do Open/LibreOffice and Gnumeric, when saving to Xlsx; as do Numbers and Google Docs. Likewise all the recognised libraries that I'm aware of for generating Xlsx files using other languages.
The only way I've been able to recreate this is by unzipping and zipping manually using a standard Windows zip utility that isn't Office-aware.

I've fixed some typos.

Not so much typos, slightly more deliberate ;-)

@Fighter456
Copy link

I still don't know how this file was created. All versions of Excel itself have always used the unix /, as do Open/LibreOffice and Gnumeric, when saving to Xlsx; as do Numbers and Google Docs. Likewise all the recognised libraries that I'm aware of for generating Xlsx files using other languages. The only way I've been able to recreate this is by unzipping and zipping manually using a standard Windows zip utility that isn't Office-aware.

I'm using WSL. Before the latest (windows) update everything works fine. Maybe @cameronjohnson-mz is using WSL, too?

My reproducer file gets automatically created each night during an automatic process (using an older version of PHPSpreadsheet and) running under WSL. But I don't find the reason why the directory separator gets changed if it hardcoded in the XLSXWriter. The magic only could happen in ZipStream.

@MarkBaker MarkBaker merged commit e35b39a into master Mar 31, 2023
@MarkBaker MarkBaker deleted the Xlsx-Reader_Windows-Folder-Separator-in-Zip branch April 6, 2023 12:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Fighter456 Fighter456 Fighter456 requested changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@MarkBaker @Fighter456