-
Notifications
You must be signed in to change notification settings - Fork 8
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
Refactor, bump minimum required PHP version to 8.2 and support 8.4 #27
Conversation
@dbu Please approve the workflow. |
59d1a5e
to
d7e62aa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks a lot for this!
not sure about that code coverage complaint from phpunit. if we can disable the check that would make sense to me. we have 1 class in the codebase, no need to specify what we cover. |
@dbu Pushed again, please approve workflow. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice, thanks. i was pondering if we want to make a new major version, but there is actually only the removal of BC code in the class, the other changes are all about the test.
as we drop legacy versions, i suggest we do it as a new minor version. (thats more flexible in case we for some reason really want to fix something for php 8.1 or older)
$this->assertSame("Sending request:\nGET http://example.com/ 1.1", $this->logger->logMessages[0]['info']); | ||
// Ensure there's an error log for the exception | ||
$this->assertStringContainsString("Error:\nNot Found", $this->logger->logMessages[1]['error']); | ||
throw $exception; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i usually would have done a $this->fail(...)
inside the try block, on the line after )->wait();
but this works as well.
Good idea! |
thanks a lot, tagging a release now |
This bumps the minimum required PHP version to 8.2.
PHPSpec is remover in favor of PHPUnit.
Scrutinizer CI is removed because we no longer have coverage.