Autofilter Part 1 #2141
Merged
Autofilter Part 1 #2141
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
While researching an issue, I noticed that coverage of Document/Properties was poor. Further, the use of int timestamps will eventually lead to problems for 32-bit PHP (see issue PHPOffice#1826). Coverage Changes: - Many property types with no special handling are enumerated but not tested. These are removed, but will continue to function as before. - Existing code theoretically allows property to be set to an object, but there is no means to read or write such a property, and, even if there were, I don't believe Excel supports it. Setting a property to an object will now be changed to a no-op (can throw an exception if preferred). - Since the Properties object now has no members which are themselves objects, there is no need for a deep clone. The untested __clone method is removed. - Large switch statements are replaced with associative arrays. Scrutinizer will like that. - Coverage is now 100%. <!-- end of coverage changes list --> Timestamp Changes: - Timestamps will be stored as int if possible, or float if not. This is, or will soon be, needed for 32-bit systems. Tests have been added for beyond-epoch dates, and run successfully with 32-bit. - LibreOffice doesn't quite get the Created/Modified properties correct. These are written to the file as a string which includes offset from UTC, but LibreOffice ignores the offset portion when displaying them. Code had been generating these in UTC, but now generates them in default timezone, which should meet user's expectations. <!-- end of timestamp changes list --> Other Changes: - Custom properties added to ODS Writer. - Samples had not been generating any ODS files. One is now generated. - Ods uses a single 'keywords' property rather than multiple 'keyword' properties. - Breaking change - default company is changed to null string from Microsoft Corporation. - Breaking change of sorts - PropertiesTest incorrectly tested a custom date property against a string, Reader/XlsxTest correctly tested against a timestamp converted to a string. PropertiesTest was defective, and will no longer work as coded; anyone using it as a model will likewise have a problem. - PHP8.1 has been complaining for weeks about a time zone conversion test. I have now downloaded a version, and changed the code so that it will work in 8.1 as well as prior releases. (It is still likely that the existing code should work in 8.1, but I haven't yet figured out how to file a bug report.) In the course of testing, 3 additional 8.1 problems were reported (all along the lines of "can't pass null to strpos"), and are fixed with null coercion. - Two Calculation tests failed because of large results on 32-bit system. These are corrected by allowing the functions involved to return float|int rather than int. I suspect that there are other functions with this problem, and will investigate as a follow-up activity. - See issue PHPOffice#2090. I believe that changes between 17.1 and master will merely cause the problematic spreadsheet to fail in a different way. I believe that enclosing in quotes some variables passed to Document/Properties by Reader/Xlsx will eliminate the problem, but, in the absence of an example file, cannot say for sure. - Properties tests are now separated out from Reader/XlsxTest and Reader/OdsTest, and now test both Read and Write (via reload). <!-- end of other changes list --> Miscellaneous Notes: - There remains no support for Custom Properties in Xls Reader or Writer. - We now have default timezones for all of PHP itself, Shared/Date, and Shared/Timezone. That is least one too many. I was unable to disentangle the latter two for this change, but will look into deprecating one or the other in future.
3 minor errors that hadn't blocked the request.
Most of the remaining 32-bit-unsafe date handling that remains in PhpSpreadsheet is in AutoFilter. Cleaning this up demonstrated that there are a lot of problems with AutoFilter, and I will do it in (probably two) pieces. In this PR: - Dynamic date processing was really wrong. There were no tests nor samples to exercise this code. (If you need details, you can try running the new sample against old code.) It is completely re-written. - ThisYear/Month/Week/Quarter had been omitted. - Rules such as AUTOFILTER_RULETYPE_DYNAMIC_MONTH_2 were almost correct, but showed some off-by-1 errors. I suspect these were timezone-related, and therefore more obvious to those of us far away from Greenwich. - All Autofilter tests are moved to a single directory. - The documentation suggested using null with the Dynamic Date setup, but Phpstan did not like that in my new tests/samples. Rather than change the doc block, I changed the documentation to suggest null string. - I created a new sample to generate sheets using all the dynamic filters. - I have added some new unit tests for each of the dynamic filters. I would love to be able to add some "time travel" tests because the dynamic nature of the filter makes most of the results change from day to day, which presents significant challenges in writing comprehensive unit tests (the same is true for code coverage). I was not able to find a good way to simulate time within PhpUnit, but the Linux 'faketime' package was extraordinarily easy and helpful in allowing me to confirm some edge cases. I had less satisfactory results with some Windows equivalents, but was still able to run some tests. - Code coverage increases from below 60% to above 80%. To be done: - Some 32-bit unsafe dates remain in filterTestInDateGroupSet. - Also in some of the existing AutoFilter samples. - Study existing unit tests for AutoFilter which use mocking to see if they can/should be replaced with 'real' tests. - Improve code coverage in AutoFilter, AutoFilter/Column, and AutoFilter/Common/Rule.
Fix 5 minor errors.
oleibman
added a commit
to oleibman/PhpSpreadsheet
that referenced
this pull request
Jun 15, 2021
Most of the remaining 32-bit-unsafe date handling that remains in PhpSpreadsheet is in AutoFilter. Cleaning this up demonstrated that there are a lot of problems with AutoFilter, and I will do it in two pieces. Part 1 was PR PHPOffice#2141 which I have just merged. In this PR: - Fix remaining 32-bit dates in filterTestInDateGroupSet. - Also in some of the existing AutoFilter samples. Note that the comments in two of those said the filter was being set for the first day of each month, but the code specifies the last day - I have corrected the comments. - Remove mocking in unit tests for AutoFilter in favor of 'real' tests. - Code coverage is now 100% in all of AutoFilter, AutoFilter/Column, and AutoFilter/Common/Rule. - No remaining AutoFilter(/Column(/Rule)) exceptions in Phpstan baseline. - Documentation for escaping of asterisk, question mark, and tilde in text filters included spurious backslashes which are now removed. - Text filter escaping of question mark did not work. There had been no unit tests for any text filtering. - Likewise there had been no testing for TopTen. - Above- and below- average filters were not working because they acquired their Calculation instance incorrectly. There had been no tests. - Several unchanging private static arrays in Rule were changed to private const arrays. - Clones are now tested. - RuleTest is moved to same directory as other tests.
Closed
MarkBaker
pushed a commit
that referenced
this pull request
Jun 24, 2021
Most of the remaining 32-bit-unsafe date handling that remains in PhpSpreadsheet is in AutoFilter. Cleaning this up demonstrated that there are a lot of problems with AutoFilter, and I will do it in two pieces. Part 1 was PR #2141 which I have just merged. In this PR: - Fix remaining 32-bit dates in filterTestInDateGroupSet. - Also in some of the existing AutoFilter samples. Note that the comments in two of those said the filter was being set for the first day of each month, but the code specifies the last day - I have corrected the comments. - Remove mocking in unit tests for AutoFilter in favor of 'real' tests. - Code coverage is now 100% in all of AutoFilter, AutoFilter/Column, and AutoFilter/Common/Rule. - No remaining AutoFilter(/Column(/Rule)) exceptions in Phpstan baseline. - Documentation for escaping of asterisk, question mark, and tilde in text filters included spurious backslashes which are now removed. - Text filter escaping of question mark did not work. There had been no unit tests for any text filtering. - Likewise there had been no testing for TopTen. - Above- and below- average filters were not working because they acquired their Calculation instance incorrectly. There had been no tests. - Several unchanging private static arrays in Rule were changed to private const arrays. - Clones are now tested. - RuleTest is moved to same directory as other tests.
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?
Autofilter Part 1
Most of the remaining 32-bit-unsafe date handling that remains in PhpSpreadsheet is in AutoFilter. Cleaning this up demonstrated that there are a lot of problems with AutoFilter, and I will do it in (probably two) pieces.
In this PR:
To be done: