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

Autofilter Part 2 #2162

Merged
merged 5 commits into from
Jun 24, 2021
Merged

Autofilter Part 2 #2162

merged 5 commits into from
Jun 24, 2021

Conversation

oleibman
Copy link
Collaborator

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.

This is:

- [x] a bugfix
- [ ] a new feature

Checklist:

Why this change is needed?

oleibman added 3 commits June 15, 2021 07:13
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.
24 minor problems, almost all of them unused code in tests.
Copy link
Member

@MarkBaker MarkBaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Id recommend switching to use of the WildCard class for handling wildcards in text searches, because it handles a lot of edge cases that this original simplistic method didn't take into account

src/PhpSpreadsheet/Worksheet/AutoFilter.php Outdated Show resolved Hide resolved
Per suggestion from @MarkBaker.

WildcardMatch did not handle double tilde correctly. It has been changed to do so and its logic simplified (and commented).

Existing AutoFilter test covered this situation, but I added a test for MATCH as well.
@oleibman oleibman mentioned this pull request Jun 22, 2021
@MarkBaker
Copy link
Member

Note that we have a few issues with PHP8.1 nightly that will need addressing before the planned November release of that PHP version; although some of these issues are with phpunit dependencies

@MarkBaker MarkBaker merged commit d0dd5b4 into PHPOffice:master Jun 24, 2021
@oleibman oleibman deleted the moredatefilter3 branch July 1, 2021 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants