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

Fix PHP 8.4 deprecations for v2, bump minimal version to PHP 7.1 #108

Open
wants to merge 2 commits into
base: version/2
Choose a base branch
from

Conversation

wmouwen
Copy link

@wmouwen wmouwen commented Jan 10, 2025

Version 2 of this package supports PHP version 5.4 and higher. In PHP version 7.1 the explicit nullable notation was added and not using it started throwing deprecation warnings in version 8.4.

The package league/oauth2-server supports league/events v3 starting from league/oauth2-server v9. However, due to a bug in league/oauth2-server upgrading is blocked for those using the client credentials flow and we're stuck on v8.

Upgrading to PHP 8.4 with v3 is blocked due to league/oauth2-server, so upgrading to PHP 8.4 with v2 is the next best thing, but the deprecation warnings are keen on displaying themselves in the response body. They prepend the JSON token returned causing the JSON to become invalid. Although for us it isn't a problem on production, it is quite an annoyance on local development environments since tokens are more often broken than not.

2025-01-10-135001_1521x346_scrot

This PR is an attempt to fix v2 of this package to stay compatible with PHP 5.4 through PHP 8.4 but without the deprecation warnings being thrown.

@wmouwen wmouwen marked this pull request as draft January 10, 2025 13:04
@grizzm0
Copy link

grizzm0 commented Jan 15, 2025

We're also in need of this.

We're using mezzio/mezzio-authentication-oauth2 which still uses thephpleague/oauth2-server:^8.3.5 which in turn uses 2.2 of this repo. This is the last repo blocking us from upgrading to PHP 8.4.

@wmouwen
Copy link
Author

wmouwen commented Jan 15, 2025

Realized that even though the if-statement might work, the syntax of the question mark prefix isn't accepted by PHP version 7.0 and before. Given that the package supports PHP 5.4 and beyond fixing this problem would require dropping support for older version.

A workaround for this problem is to edit the file after installing it. I would advise against using these kinds of tricks but it solves our problem with PHP 8.4 deprecation warnings. For us the workaround was adding a script to the pre-autoload-dump of composer in the composer.json file:

{
    "scripts": {
        "pre-autoload-dump": [
            "sed -i 's/setEmitter(EmitterInterface \\$emitter/setEmitter(?EmitterInterface \\$emitter/g' vendor/league/event/src/EmitterAwareInterface.php vendor/league/event/src/EmitterAwareTrait.php"
        ]
    }
}

@grizzm0
Copy link

grizzm0 commented Jan 15, 2025

As 3.0 has been out for more than 4 years I don't see an issue in changing the php requirement to ^7.1 || ~8.0.0 || ~8.1.0 || ~8.2.0 || ~8.3.0 || ~8.4.0 and ship it as 2.3. Composer will take care of the rest for all projects depending on it. Those on 7.0.99 and below will install 2.2 while those on 7.1 and above will install 2.3.

@wmouwen
Copy link
Author

wmouwen commented Jan 15, 2025

As 3.0 has been out for more than 4 years (...)

The share of daily installs for v3 is only around 9% according to packagist's stats, the majority is still downloading v2.

(...) I don't see an issue in changing the php requirement to ^7.1 || ~8.0.0 || ~8.1.0 || ~8.2.0 || ~8.3.0 || ~8.4.0 and ship it as 2.3.

According to the same stats the share of downloads for PHP version 7.0 or older is 0.5%. For PHP itself, the lowest supported version is 8.1.

Changed this PR to fix the deprecation warning for PHP 8.4 but with that requiring PHP 7.1 or later.

@wmouwen wmouwen marked this pull request as ready for review January 15, 2025 19:35
@wmouwen wmouwen changed the title Fix PHP8.4 deprecations for v2 Fix PHP 8.4 deprecations for v2, bump minimal version to PHP 7.1 Jan 15, 2025
@frankdejonge
Copy link
Member

Hi, I'm OK with increasing the minimum version, 5.x support can be dropped.

@wmouwen
Copy link
Author

wmouwen commented Jan 21, 2025

Good to hear @frankdejonge. Note that support for 7.0 needs to be dropped as well given the syntax change.

The PR is ready to be reviewed/merged. Unfortunately the flow in Scrutinizer is broken so I can't fix the failing test.

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.

3 participants