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

Fixes for implicit nullability deprecation #1152

Merged
merged 6 commits into from
Nov 22, 2024

Conversation

duncanmcclean
Copy link
Contributor

@duncanmcclean duncanmcclean commented Nov 19, 2024

Summary

Implicitly nullable parameter declarations have been deprecated in PHP 8.4.

This pull request fixes that by explicitly declaring nullable parameters, preventing deprecation warnings.


Type of change:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update
  • Misc. change (internal, infrastructure, maintenance, etc.)

Checklist:

  • Existing tests have been adapted and/or new tests have been added
  • Add a CHANGELOG.md entry
  • Update the README.md
  • Code style has been fixed via composer fix-style

@duncanmcclean duncanmcclean changed the title Explicit nullable parameter declarations to fix PHP 8.4 deprecation Fixes for implicit nullability deprecation Nov 19, 2024
@mfn
Copy link
Collaborator

mfn commented Nov 19, 2024

Uh nice, thanks, I just added also 8.4 to CI via #1153

AFAIK this will require a major version bump, as the signature changes will break ones extending any of these classes/methods.

But I am also curious: how can we make them "visible"?

In my 8.4 PR there's no deprecation emitted anywhere (at least not from this library). Do we have to enable this somewhere?

@duncanmcclean
Copy link
Contributor Author

duncanmcclean commented Nov 19, 2024

But I am also curious: how can we make them "visible"?

In my 8.4 PR there's no deprecation emitted anywhere (at least not from this library). Do we have to enable this somewhere?

We're using PHPUnit's --display-deprecation flag to show the deprecations in our test suite. Although, I can't get it to show deprecations when running this package's tests. 🤔

When you get it working, you'll likely see a lot of deprecation warnings from thecodingmachine/safe which hasn't been updated yet, see thecodingmachine/safe#441.

@mfn
Copy link
Collaborator

mfn commented Nov 19, 2024

@duncanmcclean I just merged #1154 , can you give it a try with your PR, if we see the deprecations now?

Or, rather: if we see some reduced, my PR makes them visible.

But OTOH might though to spot in the logs as there are so many emitted…

(As in: can you merge master again into your PR, I'll then kick of the workflow trigger finally)

@duncanmcclean
Copy link
Contributor Author

Done!

@mfn
Copy link
Collaborator

mfn commented Nov 19, 2024

It's green, that's good.

But now that I looked closer, comparing the master run e.g. https://github.com/rebing/graphql-laravel/actions/runs/11916496933/job/33209292621 with this one https://github.com/rebing/graphql-laravel/actions/runs/11916994210/job/33211713836?pr=1152 , already in the master version I don't see deprecations from the code being changed here 🤔

(because if we don't see them, how can we make sure to fix them all [except running in some external project])

@duncanmcclean
Copy link
Contributor Author

I originally ran ino these deprecation warnings when running our own test suite:

Deprecated: Rebing\GraphQL\Support\Field::authorize(): Implicitly marking parameter $resolveInfo as nullable is deprecated, the explicit nullable type must be used instead in /Users/duncan/Code/Statamic/cms/vendor/rebing/graphql-laravel/src/Support/Field.php on line 37

Deprecated: Rebing\GraphQL\Support\Field::authorize(): Implicitly marking parameter $getSelectFields as nullable is deprecated, the explicit nullable type must be used instead in /Users/duncan/Code/Statamic/cms/vendor/rebing/graphql-laravel/src/Support/Field.php on line 37

Then, locally, when I pulled down graphql-laravel and ran the tests, I saw another deprecation warning about MakeCommandAssertionTrait::assertMakeCommand, so I fixed that as well.

As for the others, I didn't actually see those deprecation warnings first-hand but thought they might be worth fixing at the same time...

I would have thought they'd show up in PHPUnit though 🤔

@mfn
Copy link
Collaborator

mfn commented Nov 19, 2024

Thanks, that helps. I hope I'll find time to dig a bit deeper here.

@mfn
Copy link
Collaborator

mfn commented Nov 20, 2024

For some reason when running code via phpunit it in fact does not emit any deprecations from this library (except from test classes).

But when use just php CLI run this, besides the gazzillion deprecations from Safe\*, I see the ones you ought to get rid of:

 php -r 'require "vendor/autoload.php"; echo "\n\n\n------------------------------------------\n\n\n"; new \Rebing\GraphQL\Tests\Unit\Input\UserUpdateMutation();'
…
thousands of lines later
…
PHP Deprecated:  Rebing\GraphQL\Support\Field::authorize(): Implicitly marking parameter $resolveInfo as nullable is deprecated, the explicit nullable type must be used instead in /Users/neo/src/graphql-laravel/src/Support/Field.php on line 37

Deprecated: Rebing\GraphQL\Support\Field::authorize(): Implicitly marking parameter $resolveInfo as nullable is deprecated, the explicit nullable type must be used instead in /Users/neo/src/graphql-laravel/src/Support/Field.php on line 37
PHP Deprecated:  Rebing\GraphQL\Support\Field::authorize(): Implicitly marking parameter $getSelectFields as nullable is deprecated, the explicit nullable type must be used instead in /Users/neo/src/graphql-laravel/src/Support/Field.php on line 37

Deprecated: Rebing\GraphQL\Support\Field::authorize(): Implicitly marking parameter $getSelectFields as nullable is deprecated, the explicit nullable type must be used instead in /Users/neo/src/graphql-laravel/src/Support/Field.php on line 37

This is from master; and with your branch I don't get them 👍🏼


So: what is phpunit doing here with the code in src/ 🤔

@duncanmcclean
Copy link
Contributor Author

Hmm, that's strange. 🤔

@mfn
Copy link
Collaborator

mfn commented Nov 20, 2024

On a greenfield phpunit project I did locally, deprecations automatically show up as D unless restrictDeprecations="true" (or in 11.x: ignoreIndirectDeprecations) is set on the <source> element. But that's not the case here.

To see the actual message it requires displayDetailsOnTestsThatTriggerDeprecations='true" on the root element, but this one has already been added.

I also checked error_reporting level but couldn't spot anything.

Basically, all we (probably) should need to know is laid out on https://docs.phpunit.de/en/10.5/error-handling.html#limiting-issues-to-your-code
(although I also don't understand why it always reports the Safe\* functions even when I enabled restrictDeprecations 🤷🏼 )

Something somewhere must be messing with the settings or so.

@mfn
Copy link
Collaborator

mfn commented Nov 20, 2024

I finally figured it out, or at least some things -> master now contains #1155 which shows the deprecations.

Laravel takes over this stuff, that's why regular phpunit config does not really help much here; at least this is my understanding.

Can you please merge master once more?

@duncanmcclean
Copy link
Contributor Author

Oh, that's interesting. I guess it sorta makes sense in an app context, but I suppose it's not that helpful in the context of a package.

Can you please merge master once more?

Done 🙂

@mfn
Copy link
Collaborator

mfn commented Nov 20, 2024

Nice, yes I see them gone.

Now I can finally get onto the review how to proceed after I take a break 😅

@duncanmcclean
Copy link
Contributor Author

Gotta love debugging your test suite 🫠

@duncanmcclean
Copy link
Contributor Author

Related to this issue - the thecodingmachine/safe package is also throwing some deprecation warnings (quite a lot of them, actually).

The PR to fix them (thecodingmachine/safe#441) has been open for a while now, but unfortunatley, it doesn't look like the maintainers are actively looking at issues or pull requests. 😞

What are your thoughts on potentially using a forked version of the safe package instead? It looks like someone on that PR has creatred their own fork, with the deprecations fixed.

@mfn mfn mentioned this pull request Nov 22, 2024
9 tasks
Copy link
Collaborator

@mfn mfn left a comment

Choose a reason for hiding this comment

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

Thanks for the PR and your patience!

To me it was important to find a reliable way to be sure we fix all them and/or they don't come back with other accidental changes.

I tried improving things with a couple of PRs (#1153 , #1154 , #1155) but wasn't satisfied it finds all the cases. I finally found salvation with #1156 : phpstan on 8.4 seems most reliable to uncover this particular deprecation, as AFAICS it also finds cases not covered in this PR.

In #1152 (comment) I wrote

AFAIK this will require a major version bump, as the signature changes will break ones extending any of these classes/methods.

After reading https://php.watch/versions/8.4/implicitly-marking-parameter-type-nullable-deprecated and doing some tests I found out this is not true, so there's no BC concern.

TL;DR: will proceed merging this, handle potential leftovers in #1156 and it fall good, try to roll a release ASAP.

Thanks 🙏🏼

@mfn mfn merged commit 79da816 into rebing:master Nov 22, 2024
23 checks passed
@mfn
Copy link
Collaborator

mfn commented Nov 22, 2024

Released as 9.7.0!

@mfn
Copy link
Collaborator

mfn commented Nov 22, 2024

Regarding this matter:

Related to this issue - the thecodingmachine/safe package is also throwing some deprecation warnings (quite a lot of them, actually).

Thanks, I've written my thoughts over at thecodingmachine/safe#441 (comment) !

@duncanmcclean
Copy link
Contributor Author

duncanmcclean commented Nov 22, 2024

Thank you for looking at this PR so quickly! 🙂

I've subscribed to the issue on the safe repository.

@duncanmcclean duncanmcclean deleted the explicit-nullable-param-declarations branch November 22, 2024 09:36
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

2 participants