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

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

Closed
fishpowered opened this issue Feb 26, 2020 · 2 comments · Fixed by #4185
Closed

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

fishpowered opened this issue Feb 26, 2020 · 2 comments · Fixed by #4185

Comments

@fishpowered
Copy link

fishpowered commented Feb 26, 2020

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
Copy link

stale bot commented Apr 26, 2020

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.

@stale stale bot added the stale label Apr 26, 2020
@stale stale bot closed this as completed May 3, 2020
@oleibman
Copy link
Collaborator

oleibman commented Oct 2, 2024

I think this idea deserves some consideration. Reopening.

@oleibman oleibman reopened this Oct 2, 2024
@stale stale bot removed the stale label Oct 2, 2024
oleibman added a commit to oleibman/PhpSpreadsheet that referenced this issue Oct 6, 2024
Fix PHPOffice#1395, a 2020 issue which had been marked stale and is now re-opened. Static valueBinder property of Cell isn't ideal. It would be more flexible to make it a dynamic property of the spreadsheet. Static property will continue to be used, but dynamic property will be used first if it is set. Readers will also be changed to add a valueBinder property which they pass to the spreadsheet; however, it will make a difference only for Csv/Html/Slk, since the other readers use setValueExplicit which ignores valueBinder.

Documentation is updated in several places to note that dynamic property is preferred over static.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging a pull request may close this issue.

2 participants