search

PHPOffice / PhpSpreadsheet

A pure PHP library for reading and writing spreadsheet files
https://phpspreadsheet.readthedocs.io
MIT License
13.36k stars 3.47k forks source link

The cell value binder being static can easily lead to bugs/confusing behaviour #1395

Closed fishpowered closed 1 month ago

fishpowered commented 4 years ago

This is:

- [ ] a bug report
- [x] a feature request
- [ ] **not** a usage question (ask them on https://stackoverflow.com/questions/tagged/phpspreadsheet or https://gitter.im/PHPOffice/PhpSpreadsheet)

What is the current behavior?

Cell::getValue() can return quite different values for certain things (e.g. dates and times) depending on the IValueBinder used. This is fine, but a trap that is easy to fall into (in my opinion), is that this binder is saved statically, so if you are reading multiple file types it is quite easy to forget to set the cell value binder as it's not required when instantiating the reader.

For example, if you have some piece of code that instantiates PhpOffice\PhpSpreadsheet\Reader\Csv and don't specify a the value binder, it'll work fine reading dates and times from the file, they get returned as a string, but then if another developer comes along and writes a piece of code preceding the CSV code, that instantiates PhpOffice\PhpSpreadsheet\Reader\Xlsx and sets the AdvancedValueBinder then they are unknowingly affecting the way the Csv code is fetching date time values.

Maybe this sounds unlikely but it's quite easy to happen for things like unit tests, scheduled jobs etc.

It's hard to expect developers to know that they have to go and manually set Cell::setValueBinder() before reading spreadsheets so we are currently working around it by having factory methods which ensure it gets set but there's still nothing from stopping someone who doesn't know about these to instantiate the reader classes directly and forget to set it.

What is the preferred behavior?

That the binder is not saved statically, and perhaps an argument in the constructor?

What are the steps to reproduce?

N/A

Which versions of PhpSpreadsheet and PHP are affected?

Tested on 1.10.0 but I presume any version where Cell::$valueBinder is static.

Anyway, hope I'm not being stupid and thanks for the great library btw :)

stale[bot] commented 4 years ago

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is still an issue for you, please try to help by debugging it further and sharing your results. Thank you for your contributions.

oleibman commented 1 month ago

I think this idea deserves some consideration. Reopening.