rap2hpoutre / fast-excel

🦉 Fast Excel import/export for Laravel
MIT License
2.08k stars 246 forks source link

fix: configure reader/writer options spout v4 #318

Closed atymic closed 12 months ago

atymic commented 1 year ago

In #294, openspout was upgraded to v4. In openspout v4, you can no longer configure options on a already created writer/reader instance.

This was kind of corrected, as #294 passes the options object, not a reader/writer object as the docblock incorrectly states, however since the $reader_or_writer instanceof ReaderInterface will never eval to true (since it is an options object) you can no longer configure options.

This PR corrects that by adding a method, configureOptionsUsing which sets the options config callback, allowing you to change settings. I also no-oped and marked as deprecated the configureWriterUsing and configureReaderUsing methods since they can't possibly actually do anything anymore. Happy to remove them instead but figured can avoid a BC break by leaving them.

Also added a test to cover the options config :)

ahurov commented 1 year ago

up
that's important fix

atymic commented 1 year ago

@rap2hpoutre any chance of a review? Been using our fork in prod with no issues :)

atymic commented 1 year ago

Now arguably that method using the writer/reader as argument has been broken since https://github.com/rap2hpoutre/fast-excel/pull/311 merged on Feb 9, so maybe backward compatibility isn't a strong issue... I don't know. I had to pin my version to v5.1.0, just before https://github.com/rap2hpoutre/fast-excel/pull/311, to keep using that method as documented.

That was my thought, as it's already broken. I guess the other option is just deleting those broken methods completely and just releasing a new major version?

Would be happy to split it out if @rap2hpoutre would like

atymic commented 1 year ago

@rap2hpoutre Any chance of a merge/review?

adrianpaiva1 commented 1 year ago

any update on this getting merged? I need to be able to configure column widths :)

atymic commented 1 year ago

I have been using my branch for ages in prod waiting for this to get merged.

 {
            "type": "github",
            "url": "https://github.com/atymic/fast-excel"
        },
        "rap2hpoutre/fast-excel": "5.0.1",

@rap2hpoutre

rap2hpoutre commented 12 months ago

Thank you for your contribution and sorry for late answer! Reviewed, accepted, merged!

rap2hpoutre commented 12 months ago

Available in 5.3.0 (since it's not really breaking change as you stated!)

robinmoisson commented 12 months ago

Awesome, thank you @rap2hpoutre!

atymic commented 12 months ago

Thanks @rap2hpoutre!