Skip to content

Conversation

sveldhuisen
Copy link
Contributor

Fix PHP 8.4 deprecation: Implicitly marking parameter as nullable is deprecated, the explicit nullable type must be used instead

Fix PHP 8.4 deprecation: Implicitly marking parameter as nullable is deprecated, the explicit nullable type must be used instead
@BenjaminFavre
Copy link
Owner

Thank you for your contribution !
Would you make it pass all the checks please ?

@sveldhuisen
Copy link
Contributor Author

sveldhuisen commented Mar 26, 2025 via email

@sveldhuisen
Copy link
Contributor Author

Psalm validation errors are now fixed: we have a green light >8-)

@sveldhuisen sveldhuisen changed the title Update OAuthHttpClient.php for PHP 8.4 Make OAuth 2 decorator compatible with PHP 8.4 Jun 25, 2025
@sveldhuisen
Copy link
Contributor Author

@BenjaminFavre : are you ok with approving the outstanding workflow?

@BenjaminFavre
Copy link
Owner

Hi @sveldhuisen,

Sorry, I've been busy. Thank you again for your contribution!

May I suggest a last update please ?
I am not sure it is a good idea to make the classes final.
I don't think it is necessary, and it may prevent sound extensions.

Unless you argue otherwise, I would be pleased to merge your code this week if you remove the finals.
I guess you will have to tell Psalm not to complain about it.

Thank you again!

@sveldhuisen
Copy link
Contributor Author

Hi @BenjaminFavre,

Hi @sveldhuisen,

Sorry, I've been busy. Thank you again for your contribution!

No problem, it's been quiet from my side of the pond too >8-)

May I suggest a last update please ? I am not sure it is a good idea to make the classes final. I don't think it is necessary, and it may prevent sound extensions.

As always, it is a matter of opinions. I always make my classes Final, unless I really want to explicitly allow extensions by others. As this is a strict implementation of the oAuth 2.0 RFC I'm not sure why somebody else should be able to extend those classes?

But please enlighten me on some use cases for extensions: maybe I'm overlooking some application to extend those classes.

Unless you argue otherwise, I would be pleased to merge your code this week if you remove the finals. I guess you will have to tell Psalm not to complain about it.

Great! I'm more than happy to remove those finals.

Thank you again!

You're welcome!

@BenjaminFavre
Copy link
Owner

But please enlighten me on some use cases for extensions: maybe I'm overlooking some application to extend those classes.

I can see one specific reason and one general reason not to make those classes final.

The specific reason is that, you are right, this is an implementation of a standard, though from my experience, sadly, there is a lot of sloppy server implementations of the protocol, and the need to depart from the standard arise quite often.

The general reason is that, although I cannot think right now of a precise good case where someone would like to extend those classes, from my experience, someday, someone will find one, and it is very frustrating when the need arise and the lib closed the possibility.

Finally, beside those potential downsides, I cannot see any benefits to make those classes final.
Unless you think of real benefits to make the classes final that I have overlooked, if the decision has to be taken solely on habitus and preferences, then as the owner of the repo I would prefer without final classes :)

Thank you again!

@sveldhuisen
Copy link
Contributor Author

As far as I'm concerned it's completely clear: thanks for your explanation.

I don't want to sound stubborn, but it helps me personally to understand the trade-off better if I know what the underlying thought process is. In that way, my own code also evolves in a positive way.

I'll revert the finals!

@BenjaminFavre BenjaminFavre merged commit 6e50730 into BenjaminFavre:master Jul 7, 2025
3 checks passed
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.

2 participants