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

[PHP 8.4] Fixes for implicit nullability deprecation #717

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Ayesh
Copy link

@Ayesh Ayesh commented Mar 14, 2024

Fixes all issues that emits a deprecation notice on PHP 8.4.

See:

Fixes all issues that emits a deprecation notice on PHP 8.4.

See:
 - [RFC](https://wiki.php.net/rfc/deprecate-implicitly-nullable-types)
 - [PHP 8.4: Implicitly nullable parameter declarations deprecated](https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated)
@DannyvdSluijs
Copy link

Thanks for the contribution @Ayesh. We are currently hard at wok in reviving the repository and are currently focussing on cleaning up any old issues and pull request. After which we are going to work on merging PR's.

This PR would work and resolve issues for PHP 8.4 without a doubt. But this repository also supports older version of PHP (currently still even PHP 5.3) which don't support the ? (nullable type declaration). We are looking into supporting only more current versions of PHP but for Composer we would still need to support all the way back to PHP 7.2 (which is already EOL). You can check the comment
Luckily the nullable type declaration was introduces in PHP &.1 so we should be good once we can shed the older versions.

But still very much appreciated to put in the effort and bringing this to our attention.

@Ayesh
Copy link
Author

Ayesh commented Mar 14, 2024

Thank you for the quick response @DannyvdSluijs. I was pleasantly surprised to see this library supporting PHP versions that old.

I jumped to submit the fix when I saw the notices when running Composer, but you are right, it probably needs coordination to release a version with a higher minimum PHP version.

I'm not sure if closing this PR will help. If that's the case, I will happily submit a new PR against the new branches if we are about to release a PHP 7.1+ version.

Thank you.

@nicolas-grekas
Copy link

It would be nice to move quickly on this one so that the rest of us can enjoy a deprecation-free composer experience ASAP 🙏

@Seldaek
Copy link
Collaborator

Seldaek commented Mar 19, 2024

Yeah this is a tricky one as it will require the php version bump, it is gonna have to be for v6, which does not exist yet, and there are quite a few open things before it could be tagged https://github.com/justinrainbow/json-schema/projects/2#card-11979917

But IMO this could be merged as soon as the php requirement is bumped, and perhaps an alpha version tagged at some point so we could use that in composer.

@mvorisek
Copy link

In theory, this PR can be implemented using traits for all supported PHP versions. 35 changes, so max. 35 traits :)

@Seldaek
Copy link
Collaborator

Seldaek commented Mar 22, 2024

We already have problems finding enough time to maintain this lib, so let's please avoid suggesting ways to waste even more time :)

@stof
Copy link

stof commented Mar 22, 2024

This would require bumping the PHP requirement to at least PHP 7.1+, which can be made compatible with the composer needs of supporting PHP 7.2.

@nicolas-grekas
Copy link

What about releasing v5.3.0 with the bump to 7.1?

@erayd
Copy link
Collaborator

erayd commented Mar 22, 2024

What about releasing v5.3.0 with the bump to 7.1?

Absolutely not. Increasing the PHP minimum version that far is a breaking change, and therefore necessitates a major version change for this library. We can't just drop it into the 5.x.x branch and not care about what we break.

It could go in the v6 branch though (although please see this comment in relation to that).

@stof
Copy link

stof commented Mar 25, 2024

If the increase is done without forgetting to update the composer metadata, it won't break BC for projects installing it through composer as composer takes the PHP requirement into account when resolving dependencies (unlike what npm does with the node compatibility info).

@Seldaek
Copy link
Collaborator

Seldaek commented Mar 25, 2024

Yeah I am also a proponent of just bumping the php requirement in minor semver version bumps, which I think is completely fine as people can still rely on the old versions with older php versions, and it won't just update to the new one if they aren't on a modern enough php so this shouldn't break anything.

From what I understood tho @erayd wrote that support for 5.3+ is still desirable for master/v6. So I don't know, what I would personally do but that's just my 2c:

  • Drop php <7.2 in v5, and tag a v5.3.0 with php8.4 support, 5.2.x can still be supported with backports for patches if really needed
  • Tag a v6.0.0-alpha from master so anyone relying on php<7.2 support can still use that as it is now, and forever, but without receiving further patches because really sorry but what are you still doing on such an old php version 🙈
  • Then also drop php <7.2 from master, continue cleanups and whatever is necessary to get a v6 stable out, that could take longer but at least master would have php8.4 support and 5.3.0 as well, so users all can have a version that fits whatever they need.

I think this could all be done without much work and I'm happy to even do it but I would need @erayd's blessing :)

@andypost
Copy link

it affects Drupal, hope it will be sorted out soon

$ composer why justinrainbow/json-schema
drupal/drupal     11.x-dev requires (for development) justinrainbow/json-schema (^5.2)    
composer/composer 2.7.4    requires                   justinrainbow/json-schema (^5.2.11) 

@Seldaek
Copy link
Collaborator

Seldaek commented May 1, 2024

@erayd any opinion on my post above? #717 (comment)

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.

None yet

8 participants