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

PHP8 breaks Writer/Xlsx/LocaleFloatsTest; Probably Not Fixable #1663

Closed
oleibman opened this issue Oct 1, 2020 · 7 comments
Closed

PHP8 breaks Writer/Xlsx/LocaleFloatsTest; Probably Not Fixable #1663

oleibman opened this issue Oct 1, 2020 · 7 comments

Comments

@oleibman
Copy link
Collaborator

oleibman commented Oct 1, 2020

This is:

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

What is the expected behavior?

Unit test sets locale to France, sets cell to 1.1, and reads contents of cell expecting "1,1".
Test succeeds under PHP7; it fails ("1.1") with PHP8. The PHP RFC authorizing the breaking change is:
https://wiki.php.net/rfc/locale_independent_float_to_string
Quoting from there, "In our opinion, the backward compatibility break won't be very serious in practice ..."
It isn't all that difficult to "fix" the test - skip it for PHP8, or delete it as you probably will have to eventually,
or use sprintf (except that will be testing PHP itself, not PhpSpreadsheet), probably others.
The problem is that, even if the test is changed, users depending on the current behavior will view this as a
PhpSpreadsheet problem.
In North America, I am pretty much insulated from this problem. I cannot guess how seriously it might be seen in Europe.

What is the current behavior?

"1,1" with PHP7, "1.1" with PHP8.

What are the steps to reproduce?

Existing unit test Writer/Xlsx/LocaleFloatsTest.php demonstrates the problem.

Which versions of PhpSpreadsheet and PHP are affected?

PHP8, all versions of PhpSpreadsheet.

@stale
Copy link

stale bot commented Dec 25, 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 Dec 25, 2020
@oleibman
Copy link
Collaborator Author

Undoing stalebot.

@stale stale bot removed the stale label Dec 25, 2020
@oleibman
Copy link
Collaborator Author

My initial tests had been with a Release Candidate version of PHP8. Current testing suggests that var_dump has changed since then to produce locale-aware output, which would mean that no changes are required to unit tests. I don't see anything in the change logs which suggest that this behavior has been changed, so I will leave the issue open for now and continue to monitor. The affected test is skipped in the PhpUnit tests on GitHub, because trying to set the locale fails, presumably because only one locale is installed on the test system (or, at least, fr_FR.UTF-8 isn't available). This appears to be the case for all PHP releases which are part of the unit test, not just 8.0/8.1. This isn't ideal; I don't know what can be done to change that. The tests are executed on my local systems.

@oleibman
Copy link
Collaborator Author

oleibman commented Mar 1, 2021

And, in an interesting development, it turns out that var_dump is locale-aware in 64-bit PHP8, but not in 32-bit PHP8. I would report the discrepancy, but, if they decided they should act the same, I am not sure whether they would adopt the 64-bit approach for both, or the 32-bit. So I'm going to let this sleeping dog lie. The test could be changed to be skipped on 32-bit PHP8+; for now, that seems premature.

@stale
Copy link

stale bot commented Jun 26, 2021

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 Jun 26, 2021
@oleibman
Copy link
Collaborator Author

As mentioned above, this remains an issue in 32-bit PHP8. It also might be an issue for 8.1 regardless of word size.

@oleibman
Copy link
Collaborator Author

oleibman commented Aug 9, 2021

The "resolution" in #2231 is probably as good as this will get, so I am closing this issue.

  • Use of implicit or explicit cast is different between 7- and 8+. Nobody has raised it as an issue, and I think that 8 has been available long enough that somebody might have by now if it were a concern.
  • Use of var_dump was supposed to change in 8, but somehow didn't for 8.0 64-bit. It did change for 8.0 32-bit, and, although not yet official, seems poised to change for 8.1. I do not believe that this is a "normal" programming technique.
  • Use of sprintf is stable across PHP versions. 2231 changed applicable test to use sprintf rather than var_dump, and that seems a reasonable path moving forward.

@oleibman oleibman closed this as completed Aug 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

1 participant