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

Support writing to resource handles in all IWriter implementations #1292

Merged
merged 7 commits into from
May 16, 2020

Conversation

basbl
Copy link
Contributor

@basbl basbl commented Dec 16, 2019

Looking for feedback

This is:

  • a bugfix
  • a new feature

Checklist:

Why this change is needed?

Some writers already had support for writing to resource handles passed through IWriter::save. The ones preventing this from becoming a supported feature among all of them were the XLSX and the ODS writers which are zipped collections of files and \ZipArchive does not support writing to streams.

By taking on an extra dependency, namely maennchen/zipstream-php, this could be supported.

@rosstuck
Copy link

rosstuck commented Jan 6, 2020

Just wanted to throw in that this would be super useful since it would allow folks to generate XLSX and other formats without needing write access to a temp directory anymore. If there's anything we can do to help nudge this along, please say the word, we're happy to put in the work on it. ❤️

@PowerKiKi
Copy link
Member

This seems to be a move in the right direction. @basbl, you created this PR as a draft. What else did you plan to do before completion ?

@PowerKiKi PowerKiKi added writer/csv Writer for CSV files writer/html writer/ods Writer for Open/LibreOffice spreadsheet files (OASIS) writer/pdf writer/xls Writer for MS BIFF-format (xls) spreadsheet files writer/xlsx Writer for MS OfficeOpenXML-format (xlsx) spreadsheet files labels Mar 2, 2020
@basbl
Copy link
Contributor Author

basbl commented Mar 2, 2020

Mainly just gathering feedback and fixing the build, @PowerKiKi

@basbl basbl force-pushed the write-to-stream-support branch from 6a0c583 to 52025c5 Compare April 2, 2020 20:03
@PowerKiKi
Copy link
Member

Let us know when you feel it can be reviewed, or if you need help from the community for something.

Also, feel free to add typings in existing code when it is not breaking, and definitely add them for all new code.

@PowerKiKi PowerKiKi mentioned this pull request Apr 26, 2020
5 tasks
@basbl basbl force-pushed the write-to-stream-support branch from 52025c5 to 195409d Compare April 27, 2020 09:48
basbl added 3 commits April 27, 2020 21:26
The built-in ZipArchive class does not have the ability
to accept streams. This means that we would always have to
write the zip to disk. The ZipStream library does offer
support for writing to streams.
@basbl basbl force-pushed the write-to-stream-support branch from 195409d to 31066b4 Compare April 27, 2020 19:27
@basbl basbl force-pushed the write-to-stream-support branch from 31066b4 to b60b1d2 Compare April 27, 2020 19:49
@basbl
Copy link
Contributor Author

basbl commented Apr 28, 2020

@PowerKiKi I could use some help with getting the final Scrutinizer issues resolved. There's also a false positive in there like:

IssueId: 42375536 Message: It seems like $pFilename can also be of type resource; however, parameter $filename of fopen() does only seem to accept string, maybe add an additional type check? Filename: src/PhpSpreadsheet/Writer/Ods.php LineNumber: 108 Link: https://scrutinizer-ci.com/g/PHPOffice/PhpSpreadsheet/inspections/609cbf0c-f57a-46f7-b13b-2b06d52f5e5e/issues/files/src/PhpSpreadsheet/Writer/Ods.php?status=new&orderField=path&order=asc&honorSelectedPaths=0&issueId=42375536

@@ -126,6 +132,8 @@ public function save($pFilename)
}

// Close file
rewind($fileHandle);
Copy link
Member

Choose a reason for hiding this comment

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

Why did you choose to rewind here and in other writers ?

@PowerKiKi PowerKiKi marked this pull request as ready for review May 16, 2020 11:25
@PowerKiKi PowerKiKi merged commit 8967696 into PHPOffice:master May 16, 2020
@PowerKiKi
Copy link
Member

Thanks ! I completed it a bit and merge it.

@basbl
Copy link
Contributor Author

basbl commented May 16, 2020

Thanks a bunch!

PowerKiKi added a commit that referenced this pull request May 31, 2020
### Added

- Support writing to streams in all writers [#1292](#1292)
- Support CSV files with data wrapping a lot of lines [#1468](#1468)
- Support protection of worksheet by a specific hash algorithm [#1485](#1485)

### Fixed

- Fix Chart samples by updating chart parameter from 0 to DataSeries::EMPTY_AS_GAP [#1448](#1448)
- Fix return type in docblock for the Cells::get() [#1398](#1398)
- Fix RATE, PRICE, XIRR, and XNPV Functions [#1456](#1456)
- Save Excel 2010+ functions properly in XLSX [#1461](#1461)
- Several improvements in HTML writer [#1464](#1464)
- Fix incorrect behaviour when saving XLSX file with drawings [#1462](#1462),
- Fix Crash while trying setting a cell the value "123456\n" [#1476](#1481)
- Improved DATEDIF() function and reduced errors for Y and YM units [#1466](#1466)
- Stricter typing for mergeCells [#1494](#1494)

### Changed

- Drop support for PHP 7.1, according to https://phpspreadsheet.readthedocs.io/en/latest/#php-version-support
- Drop partial migration tool in favor of complete migration via RectorPHP [#1445](#1445)
- Limit composer package to `src/` [#1424](#1424)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
writer/csv Writer for CSV files writer/html writer/ods Writer for Open/LibreOffice spreadsheet files (OASIS) writer/pdf writer/xls Writer for MS BIFF-format (xls) spreadsheet files writer/xlsx Writer for MS OfficeOpenXML-format (xlsx) spreadsheet files
Development

Successfully merging this pull request may close these issues.

3 participants