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 #441

Closed
wants to merge 1 commit into from

Conversation

Ayesh
Copy link
Contributor

@Ayesh Ayesh commented Mar 18, 2024

Fixes all issues that emit deprecation notices on PHP 8.4 for implicit nullable parameter type declarations.

See:

The changes are modified on the generated files. This PR is merely to show the diffs.

Fixes all issues that emit deprecation notices on PHP 8.4 for implicit nullable parameter type declarations.

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)
@staabm
Copy link
Collaborator

staabm commented May 25, 2024

just came here to submit a similar PR. would be great to have this one merged :-)

@ophian
Copy link

ophian commented Aug 26, 2024

me too... 😅 I should have had a look earlier and it would have saved me an hour of work... (I did this to get rid of the deprecations in PhpMyAdmin)

@Ayesh Ayesh marked this pull request as ready for review August 29, 2024 01:33
@shish
Copy link
Collaborator

shish commented Sep 9, 2024

This is my current attempt at updating the generator to handle this case:

--- a/generator/src/Parameter.php
+++ b/generator/src/Parameter.php
@@ -75,7 +75,7 @@ class Parameter

     public function isNullable(): bool
     {
-        return $this->phpStanType->isNullable();
+        return $this->phpStanType->isNullable() || $this->getDefaultValue() === "null";
     }

     /*
diff --git a/generator/src/WritePhpFunction.php b/generator/src/WritePhpFunction.php
index d0775fb..ed4f11a 100644
--- a/generator/src/WritePhpFunction.php
+++ b/generator/src/WritePhpFunction.php
@@ -143,6 +143,9 @@ class WritePhpFunction
             }

             if ($paramAsString !== '') {
+                if ($param->isNullable() && $paramAsString[0] !== "?") {
+                    $paramAsString = "?" . $paramAsString;
+                }
                 $paramAsString .= ' ';
             }

@shish
Copy link
Collaborator

shish commented Sep 10, 2024

I needed this for my own use, and I don't see it getting merged any time soon, so I've fixed this (and merged a bunch of the other open PRs) and released as shish/safe (flagged as a replacement package so it should be drop-in compatible)

@duncanmcclean
Copy link

PHP 8.4 has been released and will be announced today. Is there anything we can do to help get this merged?

@shish
Copy link
Collaborator

shish commented Nov 21, 2024

Given that there are still php8.0 fixes waiting to be merged, I think your best hope of php8.4 support is switching over to a not-abandoned fork

@mfn
Copy link

mfn commented Nov 22, 2024

I've tested shish/safe and can confirm "it just works" as a drop-in.

However, it does not work together with https://github.com/thecodingmachine/phpstan-safe-rule because that one has (understandably, I guess) hard-coded logic to locate thecodingmachine/safe and read the function list from it, etc. You just get errors from phpstan.
(On top of that, phpstan-safe-rule at some point also needs to be adapted to work with PHPStan 2)

In the past I was involved with https://github.com/PHP-Open-Source-Saver , we forked over https://github.com/PHP-Open-Source-Saver/jwt-auth and try to be active/maintain it there. A few other forks also found their home there.

If we've enough interested ppl, I'm happy to join a group there who wants to have a more joint-maintained version (incl. a proper generator fix etc.).

@shish
Copy link
Collaborator

shish commented Nov 22, 2024

However, it does not work together with https://github.com/thecodingmachine/phpstan-safe-rule because that one has (understandably, I guess) hard-coded logic to locate thecodingmachine/safe and read the function list from it, etc. You just get errors from phpstan.

That is correct, I ended up forking that too - https://github.com/shish/phpstan-safe-rule/

@BafS
Copy link

BafS commented Nov 25, 2024

@Kharhamel @moufmouf would it be possible to merge this PR please?

@danepowell
Copy link

danepowell commented Nov 25, 2024

shish/safe isn't a drop-in replacement for all projects, as it bumps the minimum PHP version from 8.0 to 8.2.

@shish can you please at least support 8.1, which has security support for another year? My project depends on the safe library and has to support PHP 8.1 for compliance reasons.

@shish
Copy link
Collaborator

shish commented Nov 26, 2024

Heads up that the original maintainers have returned from hibernation in #452 and are looking at merging my updates upstream \o/

As to php8.1 support, that is harder than it looks because php's standard library has backwards-incompatible changes at the syntax level - but I have flagged that as something for the new maintenance team to discuss and decide on before they put out a new version

@staabm
Copy link
Collaborator

staabm commented Nov 26, 2024

@shish @OskarStark @silasjoisten I think this PR here is a no-brainer and should be merged right away.

it doesn't impact BC was already confirmed by people in here to be working.

we should just open a corrsponding issue at the phpstan-safe-rule so we don't forget about reflecting the changes there.

agree?

@OskarStark
Copy link
Collaborator

I agree 👍 Feel free to merge it

@OskarStark OskarStark added the bug Something isn't working label Nov 26, 2024
@shish
Copy link
Collaborator

shish commented Nov 26, 2024

IIRC (I'm still travelling and away from laptop) this PR doesn't actually fix the issue with generating incompatible code - it manually edits the auto-generated files, but as soon as somebody runs the generator again, they will go back to being broken

@staabm
Copy link
Collaborator

staabm commented Nov 28, 2024

@Ayesh picked your changes into #464 and closing here

thank you

@staabm staabm closed this Nov 28, 2024
@staabm
Copy link
Collaborator

staabm commented Nov 28, 2024

@shish @OskarStark @silasjoisten I will work thru all open PRs and merge what I can do so we can move forward.

I don't it makes much sense that we look with 4 people above anything.
Feel free to disagree and start a discussion in case you don't like that. I want this things to be done right now, so I am moving ahead for now.

after we got everything sorted we should have a more organized process ;)

@OskarStark
Copy link
Collaborator

Thanks, go ahead, happy to help, but also quite busy right now with @silasjoisten 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants