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

Use openspout for Excel exports #381

Open
wants to merge 18 commits into
base: master
Choose a base branch
from
Open

Conversation

chriscpty
Copy link

@chriscpty chriscpty commented Jan 3, 2025

Scope

This pull request includes a

  • Bug fix
  • New feature

Changes

The following changes were made (this change is also documented in the change log):

Related Issues

If this is related to an existing ticket, include a link to it as well.

This is mainly about #357 , I also fixed #378 in the process.

+ use openspout for excel and CSV exports, rather than PHPSpreadsheet
+ various minor fixes
+ support more style options in OpenspoutHelper::createStyleFromPhpSpreadsheetOptions
+ autoWidth for export
+ adjusted autoWidth for export
@chriscpty
Copy link
Author

chriscpty commented Jan 3, 2025

@kartik-v maybe instead of overriding the export type FORMAT_EXCEL_X, we should add a new type FORMAT_EXCEL_X_FAST that uses openspout? As it stands, openspout lacks some of the PHPSpreadsheet features the exporter uses, such as the AutoFilter function.

most of these we can hack solutions for (I already got a hacky autoSize to work and the box border styling should be doable too, albeit with some pain :D), but particularly with AutoFilter I don't really see how we'd be able to add that without going deep into openspouts internals.

Having FORMAT_EXCEL_X_FAST be a separate export type would allow users to choose performance or those additional features - plus the onInit* and onRender* hooks can more transparently not happen for that type specifically (rather than my current hack, where if any of those callbacks are set, we fall back to using PHPSpreadsheet)

What do you think?

@kartik-v
Copy link
Owner

kartik-v commented Jan 3, 2025

@kartik-v maybe instead of overriding the export type FORMAT_EXCEL_X, we should add a new type FORMAT_EXCEL_X_FAST that uses openspout? As it stands, openspout lacks some of the PHPSpreadsheet features the exporter uses, such as the AutoFilter function.

most of these we can hack solutions for (I already got a hacky autoSize to work and the box border styling should be doable too, albeit with some pain :D), but particularly with AutoFilter I don't really see how we'd be able to add that without going deep into openspouts internals.

Having FORMAT_EXCEL_X_FAST be a separate export type would allow users to choose performance or those additional features - plus the onInit* and onRender* hooks can more transparently not happen for that type specifically (rather than my current hack, where if any of those callbacks are set, we fall back to using PHPSpreadsheet)

What do you think?

Yes I did mention in my comment on the other similar PR... that there must be an option to activate openspout selectively like a class property useOpenSpout ... I think the CSV, TEXT(Delimited) and XLSX formats can be easily derived from open spout . So it may not just be the XLSX format. For PDF output it may need formatting - need to put some effort to think about that later. Open spout also offers an additional output format ODS. So its better if we can include all natively supported formats by the library: i.e. when useOpenSpout is true or then following formats can be set - else exception to be thrown:

  • FORMAT_EXCEL_X
  • FORMAT_CSV
  • FORMAT_TEXT
  • FORMAT_ODS

@chriscpty
Copy link
Author

I don't think it should throw an exception if useOpenSpout is true for a non-openspout format - instead, it should fall back to PHPSpreadsheet.
otherwise we run into a problem when you want a widget to support both openspout- and PHPSpreadsheet-exclusive formats - you'd have to set useOpenSpout in the export request to not throw an exception, rather than just setting it to true and leaving it there.

+ add useOpenspout option
+ add support for ODS exports
o rename _objExcel* objects to _objOpenspout* for clarity
! checks to prevent non-Excel openspout exports from crashing
o refactor out mergeCells function
+ add default options for ods export
! fix various minor issues
+ add empty i18n for openoffice
! fix ods export crashing if a cell has no border style
o minor improvements for ods export
+ add box styles for openspout export
+ add header styles back, refactor out outline/inside border logic
o last row check fix
src/ExportMenu.php Outdated Show resolved Hide resolved
@chriscpty
Copy link
Author

The good news: This is mostly working now and I think I've added all the functionality openspout supports to the openspout export.

The bad news: I don't think this actually performs better than PHPSpreadsheet? Maybe this is an implementation issue, I'll try and find out, but as of now the openspout export runs into memory issues at about the same point PHPSpreadsheet did in my testing :/

o fix german i18n
! fix bug with batching
@chriscpty
Copy link
Author

Nevermind the part about the performance not keeping up, i still had YII_DEBUG set 🤦

This should be ready to review/merge now, I think

@chriscpty chriscpty marked this pull request as ready for review January 8, 2025 15:43
@chriscpty chriscpty changed the title WIP: Use openspout for Excel exports Use openspout for Excel exports Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants