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

Closed
wants to merge 1 commit into 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
Collaborator

DannyvdSluijs commented Mar 14, 2024

Thanks for the contribution @Ayesh. We are currently hard at work 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 introduced in PHP 7.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
Contributor

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
Contributor

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
Contributor

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
Contributor

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
Contributor

Seldaek commented May 1, 2024

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

@DannyvdSluijs DannyvdSluijs added the Open discussions There are still one (or more) open discussion which touch upon this issue/PR label May 27, 2024
@DannyvdSluijs
Copy link
Collaborator

See #726 for the ongoing discussions on this topic.

This was referenced Jul 6, 2024
@Seldaek
Copy link
Contributor

Seldaek commented Jul 6, 2024

For anyone waiting for this, bump to 5.3.0 to fix this https://github.com/jsonrainbow/json-schema/releases/tag/5.3.0 for now while we wait for the master branch here to catch up :)

@Ayesh
Copy link
Author

Ayesh commented Jul 7, 2024

Thank you for working out the changes in 5.3 series. I just updated to latest Composer snapshot, and had zero warnings/notices on latest php-src build!

@DannyvdSluijs
Copy link
Collaborator

I've ported this PR in #746, this addresses the raising of the minimum PHP version as a whole including pipelines.

DannyvdSluijs added a commit that referenced this pull request Aug 27, 2024
* ci: Add PHP 8.0 and greater to build matrix

* ci: Remove PHP 5.3 - 7.1 from workflows

* build: Require minimum PHP 7.2

* build: Upgrade to PHPUnit 8.5

* refactor: Add now required void return types for setup() methods

* build: Include phpspec/prophecy dependency

* refactor: Replace setExpectedException with expectException/expectExceptionMessage

* refactor: Replace @ExpectedException annotation for expectException method

* refactor: Replace assertInternalType for assertIsArray

* refactor: Replace getMock for createMock

* test: Improve test assertions

* fix: Solve return type issues with Objectiterator (port of #682)

See #682

* build: Update icecave/parity to ^3.0 as 1.0 uses deprecated each() method

* style: Correct code style issues

* fix: Fix deprecation notices found from GHA workflow run

See https://github.com/jsonrainbow/json-schema/actions/runs/10216569969/job/28268331091

* fix: Add fallback to empty string when null value is passed in UriResolver::parse

* fix: Port #717: Fixes for implicit nullability deprecation

See #717

* ci: Avoid GHA run on each push and pull request; Include PHP 8.4 in matrix

* ci(Drop-PHP-8.4-from-matrix): This PR adds phpspec/prophecy as an explicit dependency which is restrictive and doesnt support upcoming PHP versions

* refactor: Replace ternary variable with explicit cast to string
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Open discussions There are still one (or more) open discussion which touch upon this issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants