-
Notifications
You must be signed in to change notification settings - Fork 26
feat(version upgrade)!: Bump to PHP8.5 #98
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
Conversation
| $responseCode = $httpResponse->getResponseCode(); | ||
|
|
||
| //close connection | ||
| curl_close($ch); |
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.
No longer required as per: https://www.php.net/manual/en/migration85.deprecated.php#curl
This is default behaviour since PHP 8.0: https://www.php.net/manual/en/class.curlhandle.php
| } | ||
|
|
||
| public function decideResponseCases(): array | ||
| public static function decideResponseCases(): array |
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.
Enforced by PHPUnit 11
| use PostHog\Consumer\ForkCurl; | ||
| use PostHog\Consumer\LibCurl; | ||
| use PostHog\Consumer\Socket; | ||
| use Symfony\Component\Clock\Clock; |
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.
total PHP noob
(well, i ran a drupal site like 18 years ago)
is it necessary to go from standard library time to Symfony?
is there something that could trip us up here?
since the time of an event is so critical just worried about a silly mistake in the change (genuinely asking from a place of ignorance though :)))
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.
It's a sneaky one, the https://github.com/slope-it/clock-mock#mocked-functionsmethods library modifies the standard library and replaced the time() functionality with some custom logic. E.g. it intercepts the function at C level in order to be a "drop-in replacement" (context).
We needed to switch this to the Symfony clock in order to be able to manipulate it during our testing (same with the case in FeatureFlag.php), however I'm doing some testing to confirm that the behaviour is the same, as it's been a while since I've played with time based things in PHP 😄
| /** | ||
| * Trait providing time mocking functionality for tests using Symfony Clock. | ||
| * This replaces the slope-it/clock-mock dependency which required the uopz extension. | ||
| */ |
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.
ahhhhhh
ok, so the switch to symfony is for this mocking?
in which case, alongside the "is it correct/safe" question, is this (and i hate this word) idiomatic for PHP
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.
Yeah traits are quite a common pattern within PHP, and they are frequently used to house "common / reusable methods" that span across a hierarchical / compositional object structure. + The added benefit here is that we cna have it act as a drop in replacement without changing much in the underlying function calls 😄 .
Here is a blog post of how Laravel does it (from 2023), which is one of the most popular PHP frameworks: https://laraveldaily.com/post/traits-laravel-eloquent-examples (or rather how spatie does it, but they are a very big contributor to Laravel).
| backupStaticAttributes="false" | ||
| backupStaticProperties="false" | ||
| colors="true" | ||
| convertErrorsToExceptions="true" |
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.
Explanation of the chnages:
┌─────────────────────────────┬────────────────────────┬────────────────────────┬─────────────────────────────────┐
│ Change │ Old (PHPUnit 9) │ New (PHPUnit 11) │ Reason │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ Schema │ phpunit.xsd 9.3 │ phpunit.xsd 11.5 │ Updated to match PHPUnit 11 │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ backupStaticAttributes │ backupStaticAttributes │ backupStaticProperties │ Renamed in PHPUnit 10 │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ convertErrorsToExceptions │ true │ removed │ Now always true, option removed │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ convertNoticesToExceptions │ true │ removed │ Now always true, option removed │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ convertWarningsToExceptions │ true │ removed │ Now always true, option removed │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ forceCoversAnnotation │ false │ removed │ Removed in PHPUnit 10 │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ timeoutForSmallTests │ 1 │ removed │ Removed in PHPUnit 10 │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ timeoutForMediumTests │ 10 │ removed │ Removed in PHPUnit 10 │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ timeoutForLargeTests │ 60 │ removed │ Removed in PHPUnit 10 │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ verbose │ false │ removed │ Now a CLI-only option │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ cacheDirectory │ none │ .phpunit.cache │ Required in PHPUnit 10+ │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ processUncoveredFiles │ processUncoveredFiles │ includeUncoveredFiles │ Renamed │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ <include> location │ Inside <coverage> │ Inside new <source> │ Moved in PHPUnit 10 │
├─────────────────────────────┼────────────────────────┼────────────────────────┼─────────────────────────────────┤
│ <logging/> │ Empty tag │ removed │ Was unused, removed │
└─────────────────────────────┴────────────────────────┴────────────────────────┴─────────────────────────────────┘
Summary: PHPUnit 10 simplified the config by removing options that are now always-on or CLI-only, and reorganized where source directories are specified (moved from <coverage> to <source>). The changes are all structural to match PHPUnit 11's schema - no behavioral changes to how tests run.
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.
so we need to bump to version 4 in here or it's automated?
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.
It should work out of the box once I follow the steps listed here: https://github.com/PostHog/posthog-php/blob/master/RELEASING.md!
There is nothing happening automatically, but I will obviously tag it with v4 😄 !
pauldambra
left a comment
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.
we should update the README to callout change in support
and i have no idea how to version and release but we'll need to do that as v4
(assuming that's happening then :approved.stamp:
rafaeelaudibert
left a comment
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.
Make sure you follow through and do a major bump here. Since you're already deep into this, it'd be ideal if you could upgrade posthog-php to follow the new approval workflow outlined in https://posthog.com/handbook/engineering/sdks/releases. This will make releasing this library much safer/straight-forward.
TL;DR
Adds PHP 8.5 support by replacing
slope-it/clock-mock(which required theuopzextension that doesn't support PHP 8.5) withsymfony/clock.Minimum PHP version bumped to 8.2 since both 8.0 and 8.1 are EOL.
Changes
Dependencies:
php:>=8.0→>=8.2phpunit/phpunit:^9.0→^11.0symfony/clock:^6.2|^7.0slope-it/clock-mock(requiredext-uopzwhich doesn't support PHP 8.5)overtrue/phplint(unused in the SDK)CI:
[8.0, 8.1, 8.2, 8.3, 8.4]→[8.2, 8.3, 8.4, 8.5]uopzextension (no longer needed)--no-suggestflagWhy these changes?
slope-it/clock-mockdepends on theuopzPHP extension, which doesn't support PHP 8.5. We switched tosymfony/clockwhich provides time mocking without requiring any PHP extensions.Breaking changes
symfony/clockrequires PHP 8.1+Proof of changes:
Made a simple test script (or rather Claude made it):
PHP Output comparison
Running it locally gave me the following output:
Output
Giving me confidence that this is a safe "drop in replacement" change.