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? 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

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
@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!

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

Successfully merging this pull request may close these issues.

2 participants