Add type hinting

[greg0ire]

I am targetting this branch, because this is a major BC-break.

Changelog

### Added
- Type hinting on most methods

Subject

@core23 @jordisala1991 this is just to show you how big a pain in the ass this is going to be. I'm not sure we should do it anymore…

[greg0ire]

Guys, are we really gonna do this? It's a big amount of work, and an enormous source of BC-breaks…

[jordisala1991]

This is huge, but at some point this might be done right? It is a BC adding this kind of stuff? I mean the return types

[greg0ire]

Yes, even the return types. See the sf slack on channel #random…

[jordisala1991]

I am not sure on how to manage it on the bigger bundles like SonataAdmin or SonataCore.

It looks nice, but as you said, its gonna be huge BC break.

IMO we could start marking all internal classes, that are not ment to be extended or public methods with @internal. That might help in the future if we want to add those typehints or return types.

For the public api, it is a matter of time, but I think the new stuff in PHP is here to stay and soon or later we will move to it. Are they saying that in PHP 7.2 that will not break BC?

[greg0ire]

Are they saying that in PHP 7.2 that will not break BC?

Yep, there is an RFC for making some non-LSP-breaking signature changes no longer trigger warnings. But it looks hard to implement. There is a closed PR https://github.com/php/php-src/pull/2265

[jordisala1991]

Is implemented already: https://github.com/php/php-src/commit/2edc3cf8d2dd75501bf1049cf1b3ca57f11f1234

[greg0ire]

I think we should postpone this to next next major (and drop php < 7.2 support then)

[jordisala1991]

Maybe we can use 4.0 to close the api as much as we can then.

[jordisala1991]

People on that PR were not so happy with it tho.

[greg0ire]

Oh but it was merged. And if you talk about https://github.com/php/php-src/pull/2265#issuecomment-272725427, I think the author did simply not understand the PR, which is fully LSP-compliant IMO.

[greg0ire]

I think we should postpone this to next next major (and drop php < 7.2 support then)

milestones could be handy here @core23

[core23]

Is implemented already: php/php-src@2edc3cf

WTF? A different method signature is not breaking anymore in PHP 7.2? oO My Java heart is bleeding

[greg0ire]

Your Java heart? I heard java is even more permissive.

EDIT: I remember now, it's ok in Java because it's considered a separate method signature.

WTF? A different method signature is not breaking anymore in PHP 7.2? oO My Java heart is bleeding

The real WTF here is that it was not possible before. When you have signature mismatch, the error message when there is one, complains about signatures not being compatible. But two signatures can be different and compatible. For instance function ($test) is compatible with function (A $test) in theory, and will be in 7.2 . function (B $test) is compatible with function (A $test) if B extends A in theory, but that still won't be possible in 7.2

[greg0ire]

@OskarStark @core23 @jordisala1991 : so ? Do we do it before next major, or do we wait for 7.2 ?

[jordisala1991]

This PR does not contain all signature changes right?

PHP 7.2 will help with type hinting, but not with return types AFAIK.

There is also another thing to take into consideration. PHP 7.1 defined void return type and nullable return type like ?string.

So if we do this now, we are still doing it another time on PHP 7.1...

[greg0ire]

This PR does not contain all signature changes right?

Right. It's only a tiny bit to show you the ordeal this will be.

PHP 7.2 will help with type hinting, but not with return types AFAIK.

I don't think it will, but it's still a start.

There is also another thing to take into consideration. PHP 7.1 defined void return type and nullable return type like ?string.

Ok, so it would be better to wait I think.

Also, what we can do and what I'm doing right now on GoogleAuthenticator is close the API (make some classes final). https://github.com/sonata-project/GoogleAuthenticator/pull/45/files shouldn't have been merged (my bad), but once I have deprecated extension, it will be fine.

[greg0ire]

Whew! Finished. Please review again. Also, I removed the details from the UPGRADE guide, otherwise it would be too long (and I'm lazy)

[greg0ire]

I discussed this with @Soullivaneuh and we think the wait for 7.2 would be too long ( plus it would actually the wait for the 7.1 drop), so I went ahead and added type hinting where necessary.

[greg0ire]

@core23 please review again

[OskarStark]

@greg0ire the commit section looks strange, is this fine? 12 commits? just asking

[greg0ire]

@OskarStark not normal, and fixed :P

[greg0ire]

@jordisala1991 please review again

[OskarStark]

@jordisala1991 please review again

[jordisala1991]

Should I merge it? I did a complete review and it seems like everything is okay

@core23 requested changes, but all done so far.

[OskarStark]

Please merge 👍

[greg0ire]

Cool! The next major can be released.

[jlamur]

@greg0ire void return type is a >= PHP7.1 feature.

I see on master that ^7.0 is required. If this is the case for the next release, we must remove them.

[greg0ire]

@jlamur right! Can you take care of that for me? I'm on vacation

[jlamur]

Yes of course :)