Strengthen typing #3718
Merged
Strengthen typing #3718
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…calarReturnExprRector
And quite a bit more manual changes. The idea is that typing of our tests can be a bit more loose, so we assume PHPDoc is mostly correct. If that happens to be wrong, it should be caught by the tests themselves.
While we might never be able to have 100% of our code strict, we can at the very least do it for all of our tests. This ensures that our tests are using our API with the types as intended by the test author, and not silently be cast to what our API requires.
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Sep 7, 2023
Add strict types to this new test, consistent with work being done in PR PHPOffice#3718.
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Sep 7, 2023
Add strict types to this new test, consistent with work being done in PR PHPOffice#3718.
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Sep 7, 2023
Consistent with work being done in PR PHPOffice#3718.
oleibman
added a commit
that referenced
this pull request
Sep 8, 2023
* ListWorksheetInfo/Names for Html/Csv/Slk Fix #3706. ListWorksheetInfo is implemented for all Readers except Html. For most (not all), ListWorksheetInfo is more efficient than reading the spreadsheet. I can't think of a way to make that so for Html, but that shouldn't be a reason to leave it unimplemented. ListWorksheetNames is not implemented for Html, Csv, or Slk. It isn't terribly useful for those formats, but that isn't a reason to omit it. The requester's use case consists of using IOFactory to create a reader for a file of unknown format and determining the first sheet name. That seems legitimate, but it is currently not possible without extra user code if the file is Html, Csv, or Slk; this PR will make it possible. When Excel opens a Slk or Csv file, the sheet name is based on the file name. PhpSpreadsheet does this for Slk, but it uses a default name for Csv. I am not interested in creating a break for that behavior, but I have added a new boolean property `sheetNameIsFileName` with a setter to Csv Reader. The requester actually mentioned that possibility in our discussion, although it is not essential to the request. As an adjunct to the issue, the requester wishes to use the worksheet name in `setLoadSheetsOnly`. That is already possible for Html, Csv, and Slk, but that particular property is ignored for those formats. I do not see a reason to change that behavior. This treatment is now explicitly noted in the documentation for property `loadSheetsOnly`. There had been no tests for what happens when `loadSheetsOnly` is specified but no sheets match the criteria for the formats for which this makes sense (Xlsx, Xls, Ods, Gnumeric, Xml). The behavior was not consistent - some formats threw an Exception while others continued with a single empty worksheet. All cases attempt to set the active sheet, and they will now all throw identical Exceptions when they attempt to do so in this situation. Tests are added for each. There also had been no tests for `loadSheetsOnly` returning more than one sheet. One is added. * Update LoadSheetsOnlyTest.php Add strict types to this new test, consistent with work being done in PR #3718. * Update LoadSheetsOnlyTest.php Add strict types to this new test, consistent with work being done in PR #3718.
oleibman
added a commit
that referenced
this pull request
Sep 8, 2023
* Inconsistency Between Actual and Declared Type - Minor Break Fix #3711. User set a cell value to float (implicitly by default value binder), then used `setDataType` to change its type to string. This caused a problem for Xlsx Writer, which uses the string cell values as an index into a Shared String array. However, as the cell actually contained a floating point value, Php treated it as an integer index; such a treatment is both deprecated, and leads to invalid values in the spreadsheet. The use case for `setDataType` is not strong. The user always has the option to use `setValueExplicit` if the type is important. Setting a type afterwards, i.e. irrespective of the value, seems like a peculiar action. Indeed, there are no tests whatever for such use in the unit test suite. There are two possible approaches to fixing this problem. The first is to add casts to the 3 or 4 places in Writer Xlsx which might be affected by this problem (hoping that you've found them all and realizing that similar changes might be needed for other Writers). The second is to change `setDataType` to call `setValueExplicit` using the current value of the cell, thereby possibly changing the cell value. I have gone with the second option - it seems like a much more logical approach, and guarantees that the content of the cell will always be consistent with its declared type. It is, however, a breaking change; if, for example, you have a cell with a string or numeric value and specify `boolean` to `setDataType`, the cell's value will change to `true` or `false` with no way to get back to the original. * Strict Types for New Tests Consistent with work being done in PR #3718. * Improve Test Better match to original issue. * Typo
But only on GitHub Actions, not in local... This makes zero sense at all ¯\_(ツ)_/¯
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Jan 16, 2024
I check from time to time. There are a number of problems now, mostly due to the elimination of Php 7.4 and replacement of doc-block typing with explicit Php typing. - Bitwise functions were particularly affected by PR PHPOffice#3718 and PR PHPOffice#3793. - Chart/Axis and Writer/Xlsx were amusingly affected by PR PHPOffice#3836, which added a scaling option which included an array indexed by the known allowable factors, one of which is 1 trillion, which cannot be represented as an integer on a 32-bit system. Issue3833Test, introduced by the same PR (and not suffering any errors) was expanded to test this value. - Some minor changes to Reader/Xls and Shared/OLE/PPS to accommodate hex values which are negative in 32-bit but which Php-32 may wind up casting to large floating point numbers; it is not clear to me why these hadn't shown up as problems previously. Possibly this is the result of changes in the most recent Php versions. - BitAndTest, BitOrTest, BitXorTest and Shared/DateTest were adversely affected by PR PHPOffice#3859 when arguments and/or expected results too large for a 32-bit integer were supplied. - ImExpTest required a slightly reduced precision for 32-bit. No idea why this hadn't shown up earlier.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is:
Checklist:
Why this change is needed?
Improve static analysis for our own project but also projects using PhpSpreadsheet.