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

Support PHP 8.4 #191

Merged
merged 3 commits into from
Oct 17, 2024
Merged

Support PHP 8.4 #191

merged 3 commits into from
Oct 17, 2024

Conversation

andrewnicols
Copy link
Contributor

@andrewnicols andrewnicols commented Oct 2, 2024

PHP 8.4 is now in release candidate and due for release in about 6 weeks. Features are now sealed.

This pull request:

  • Add PHP 8.4 tests in the GitHub Action
  • Explicitly declares nullable parameters as nullable

Note: There is still a warning in relation to the GreedyCacheStrategy because the second argument ($defaultTtl) is required, but the first argument is optional. I'm not sure how you wish to resolve this so I'll defer to a separate issue. Options are to either stop the first argument (CacheStorageInterface $cache) from having a default value (but still allowing nullability), or to provide a default of the default ttl.

@Kevinrob
Copy link
Owner

Kevinrob commented Oct 4, 2024

Thank you @andrewnicols for this PR.

I think that we can add a default value to defaultTtl.
If we remove the default value to cache, this is potentially a breaking change.

Maybe something like 60.

@andrewnicols
Copy link
Contributor Author

Cheers,

I've set a default value in the constructor of 60. I was tempted to set the type hint (int) but that would be a breaking change so avoided that.

@Kevinrob
Copy link
Owner

Kevinrob commented Oct 7, 2024

There are some errors with 8.4, lowest. Maybe we need to drop some versions on dependencies too?

* Add tests in GitHub Action

* Explicitly declare nullable parameters as nullable
Many of the dependencies were outdated and did not support the full
range of PHP versions supported by this project at both extremes,
specifically the lowest optioned versions of the following dependencies
did not support PHP 8.4:

- guzzlehttp/guzzle
- guzzlehttp/promises
- guzzlehttp/psr7
- phpunit/phpunit
- symfony/phpunit-bridge

Meanwhile the highest possible version of each of the above already
supported PHP 8.1 and above, including PHP 8.4.
@andrewnicols
Copy link
Contributor Author

Thanks @Kevinrob ,

I saw these the other day but didn't get a chance to trouble-shoot. I ended up having to reduce some alternate versions, but most of the existing lowest options were unsupported, and all of the highest version options were fully supported.

  • phpunit/phpunit: 8.5.40 does seem to support PHP 8.4 but 9.6 also supports all versions so I felt it best to just bump the version
  • symfony/phpunit-bridge: 5.4 does seem to support PHP 8.4 but 7.1.4 also supports all versions
  • guzzlehttp/guzzle: Version 6.x is no longer maintained. Minimum version now needs to be 7.9.2 to proivde support for the required range of PHP versions
  • guzzlehttp/promises: Version 1.x is no longer supported. Minimum version required is now 2.0.3
  • guzzlehttp/psr7: Version 1.x is no longer maintained. Minimum version requireds is now 2.7.0

Copy link
Owner

@Kevinrob Kevinrob left a comment

Choose a reason for hiding this comment

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

Nice! Thank you!

@Kevinrob Kevinrob merged commit 61305ed into Kevinrob:master Oct 17, 2024
8 checks passed
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