Skip to content

Fix After/Before expanders error message when date/time is equal to boundary #478

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

StephaneLeveugle
Copy link
Contributor

Change Log

Added

  • Test coverage of both expanders

Fixed

  • Error handling of after/before expanders when value was equal to boundary

Changed

  • Moved much of the code into an abstract class

Removed

  • Explicit handling of Time

Deprecated

Security


Description

Hi,

I encountered a bug trying to use the after/before expanders when it wouldn't match because the value was equals to the boundary.
No error was set by the expander, which would result to a type error in DateMatcher line 54 (matcherFailed method expected a string as its 4th parameter and null was given)

$this->backtrace->matcherFailed(self::class, $value, $pattern, $this->error);

While trying to fix it I saw that the explicit handling of Time was never reached because in the constructor it first checks if the boundary is a DateTime and any Time will be a valid DateTime as we can see in the Time::fromString method

$this->boundaryDateTime = DateTime::fromString($boundary);

I added tests to increase the coverage of the expanders and removed any explicit handling of Time.

Finally, given how similar the expanders were I created an abstract class to remove the code duplication I hope that's fine by you.

Please tell me if I missed something and thank you for this much needed library!

@norberttech
Copy link
Member

Hey @StephaneLeveugle good catch!!
Could you take a look at PHP 8.2 and 8.3 tests before I can fully review it and merge?

Finally, given how similar the expanders were I created an abstract class to remove the code duplication I hope that's fine by you.

Normally I'm not a huge fan of abstract classes since I prefer composition over inheritance but I don't mind it here that much. If you could extract common logic to a standalone, testable class and then pass it to different types of comparisons it would be perfect but that's not blocking merging of this PR from my point of view.

@StephaneLeveugle
Copy link
Contributor Author

What do you think of traits? The abstract class could easily be a trait.

To be honest I'd rather have a trait because I don't really like inheritance as well, a glorified copy paste is more what I had in mind originally.

I'll take a look at the tests, that's weird because I had php 8.3 installed locally 🤔

@norberttech
Copy link
Member

trait works as well, I prefer it over abstract class 👍

@StephaneLeveugle
Copy link
Contributor Author

The tests fail because I used the non deprecated method isAfterOrEqualTo instead of isAfterOrEqual and the method doesn't exist in older versions of Aeon Calendar library

So we can:

  • keep using the non deprecated method and up the requirement of the aeon-php/calendar library in composer.json
  • use the deprecated method instead

I'd rather use option 1 but that's up to you

@norberttech
Copy link
Member

lets go with option number one

@norberttech
Copy link
Member

huh I have no idea what's happening with the mutation tests, @StephaneLeveugle let me know if you need any help with resolving that

@StephaneLeveugle
Copy link
Contributor Author

I'll take a look, hopefully tonight

@StephaneLeveugle
Copy link
Contributor Author

There was an incompatibility between nikic/php-parser which has been upgraded to v5 and infection/infection which was still in v0.26

I tried to upgrade infection to v0.28 to support nikic/php-parser v5, but I couldn't because of vimeo/psalm which requires v4 of nikic/php-parser

So I decided to lock nikic/php-parser to v4 in the project, are you fine with it?

@norberttech norberttech merged commit 3571fc2 into coduo:6.x Apr 25, 2024
12 checks passed
@norberttech
Copy link
Member

So I decided to lock nikic/php-parser to v4 in the project, are you fine with it?

That's perfectly fine, I'm anyway going to split tools into standalone composer.json files like I did here to avoid such conflicts.

Thanks for your contribution!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants