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 compatibility #701

Closed
13 tasks done
ondrejmirtes opened this issue Oct 30, 2020 · 51 comments
Closed
13 tasks done

PHP 8 compatibility #701

ondrejmirtes opened this issue Oct 30, 2020 · 51 comments
Assignees
Labels
dependencies Pull requests that update a dependency file enhancement
Milestone

Comments

@ondrejmirtes
Copy link
Contributor

ondrejmirtes commented Oct 30, 2020

I already did most of these steps in my fork because I want PHPStan to be ready for PHP 8 on day one or sonner.

This is what needs to be done in BetterReflection in order to fully support PHP 8:

  • Adapter signatures compatibility (ondrejmirtes@0acd452)
  • mixed type (ondrejmirtes@fabd259)
  • static type (ondrejmirtes@e6f9b72)
  • Reflecting union types (ondrejmirtes@ce858eb - this commit needs to be broken up because it mixes more things)
  • ReflectionType::__toString breaking change in PHP 8 - nullable type includes ? again (https://3v4l.org/CmTA1)
  • ReflectionSourceStubber misses to generate return types (ondrejmirtes@6a87721)
  • isBuiltin() is supposed to be only on ReflectionNamedType in PHP 8 (ondrejmirtes@86c5ce7)
  • Stringable (ondrejmirtes@e9f99da)
  • PhpStormStubsSourceStubberTest will fail a lot when building on PHP 8 because a lot of stuff changed between PHP 8 and PHP 7
  • Promoted properties in constructor - I plan to solve this by making ReflectionProperty an interface with two implementations - one based on Property node, the other based on Param node.
  • Only __construct methods can be considered constructors
  • Support for reading Attributes
  • Constant of value null as a default parameter value no longer makes the parameter type nullable

The Stringable commit and the obsoleted old-style constructors require an if based on the PHP version. I did that with a public static property on BetterReflection but a different way can be fine too.

Some of these items require a new major version, but some of them can be done in a minor version before that.

I'm creating this issue because I plan to send PRs for these (in case someone isn't faster than me) but I'd like to hear your opinions first :)

@Ocramius
Copy link
Member

I'm creating this issue because I plan to send PRs for these (in case someone isn't faster than me) but I'd like to hear your opinions first :)

I mean, PHP 8 support is certainly to be added, but no work has been done here so far because we're all quite busy with other work at the moment, so I would expect us to be somewhere close to compliance in January or such :D

If the patches are QA'd properly (reads: tests + acceptable MT) and made separate, they are very welcome! 👍

@limingxinleo
Copy link

limingxinleo commented Nov 26, 2020

PHP8.0 was released.

@ghost
Copy link

ghost commented Nov 26, 2020

please support PHP8, thanks

@Ocramius
Copy link
Member

As I mentioned above:

so I would expect us to be somewhere close to compliance in January or such :D

If you want to help out, send patches for the bullet points that @ondrejmirtes wrote above

@Ocramius
Copy link
Member

Marking this issue as a blocker for 4.12.0 - we'll try getting PHP 8 support in that version.

@Ocramius Ocramius modified the milestones: 4.12.0, 4.13.0 Dec 14, 2020
@huangzhhui
Copy link

Marking this issue as a blocker for 4.12.0 - we'll try getting PHP 8 support in that version.

I notice that 4.12.0 has been released in 16 days ago, any progress about PHP 8 compatibility ?

@Ocramius
Copy link
Member

Ocramius commented Dec 30, 2020

@huangzhhui nay: busy with other work, but please send patches, if you can help out.

@DanielBadura
Copy link
Contributor

I think the result we are aiming here is to support ^7.4 and ^8.0 or do we want to drop 7.4 support?
Im asking because of the ::export method from ReflectionClass, ReflectionFunction etc. got dropped in 8.0 and this would result in big if/else on PHP_VERSION_ID i suppose.

@Ocramius
Copy link
Member

Ocramius commented Jan 21, 2021

@DanielBadura at first ^7.4 || ~8.0.0, and after that only ~8.0.0 in a new minor release: that allows upgrading easily.

@lisachenko
Copy link

Hello, gentlemen! Are there any ETA on when PHP8 will be supported by BR? @ondrejmirtes maybe you can work on this? Unfortunately, I don't have too much time now to maintain my libraries and goaop/parser-reflection is one of them which can be replaced with BR, but if BR won't have PHP8 support soon, I might opt to keep supporting it...

@Ocramius
Copy link
Member

Ocramius commented Mar 27, 2021 via email

@lisachenko
Copy link

Thank you @Ocramius! I can completely understand this, no worries.

It is very important to prevent OSS burn out, especially for you, as half of most popular PHP libraries were invented/developed by you, so want just to say thank you for working on all of them 👍

It's a spring time, so relax a little bit, drink your favourite beer, prepare a BBQ and of course, stay healthy!

PS. There are small chances that I'll have some time for OSS, but want to work more on my own libraries and laminas-code package a little bit, preparing changes for the next major version for your review.

@WyriHaximus
Copy link
Contributor

@kukulich Ok awesome, then let you get that in first before we divide and conquer. Unless there is something I can already start on. Have time reserved for this tomorrow so I'll also get familiar with the code and the changes Ondřej did.

@kukulich
Copy link
Collaborator

@WyriHaximus I think you can try anything based on my branch. I will try to make the build green as a start.

@WyriHaximus
Copy link
Contributor

@kukulich Cool, starting with ReflectionNamedType::isBuiltin

@WyriHaximus
Copy link
Contributor

Now that I'm working on it, will probably do the named and union type changes in one go for reflection type

@WyriHaximus
Copy link
Contributor

WyriHaximus commented Sep 13, 2021

@kukulich Build #752 on your work, probably have to rebase it when you're done

@kukulich
Copy link
Collaborator

kukulich commented Sep 14, 2021

These are fixed/implemented in #750 and #752 :

  • Reflecting union types
  • ReflectionType::__toString breaking change in PHP 8
  • ReflectionSourceStubber misses to generate return types
  • isBuiltin() is supposed to be only on ReflectionNamedType
  • PhpStormStubsSourceStubberTest will fail a lot
  • Constant of value null as a default parameter value no longer makes the parameter type nullable

@kukulich
Copy link
Collaborator

#759

  • Only __construct methods can be considered constructors

@kukulich
Copy link
Collaborator

Stringable: #770

@kukulich
Copy link
Collaborator

@WyriHaximus Btw I'll try Promoted properties in constructor as next.

@WyriHaximus
Copy link
Contributor

@kukulich Came across that in a few places already <3

Ocramius added a commit to Ocramius/GeneratedHydrator that referenced this issue Sep 19, 2021
@Ocramius
Copy link
Member

I think we're only missing:

  • promoted properties
  • attributes

Then we're probably ready to go with 8.0

@kukulich
Copy link
Collaborator

Promoted properties: #785

@kukulich
Copy link
Collaborator

Attributes: #787

@WyriHaximus
Copy link
Contributor

@Ocramius @kukulich Sorry got side tracked for a few days, is there anything besides attributes that needs to be done?

@kukulich
Copy link
Collaborator

kukulich commented Oct 3, 2021

@WyriHaximus Nope, PHP 8 is “done”. You can try to do something in PHP 8.1 - eg. intersection types :)

@WyriHaximus
Copy link
Contributor

@kukulich Looks like a royal pain in the ass, I'm on it

@kukulich
Copy link
Collaborator

kukulich commented Oct 3, 2021

@WyriHaximus you can start here: #767

@WyriHaximus
Copy link
Contributor

@kukulich Cool will rebase it off that one later. So far it looks very similar to the work I've done on the named and union types so this could be a lot easier than I thought.

@WyriHaximus
Copy link
Contributor

@Ocramius @kukulich Was just looking at ReflectionUnionType and AFAIK union types can't be nested, noir does that make sense. Then shouldn't we just filter out the ReflectionNamedTypes like this? WyriHaximus-labs@356777b#diff-7a5518cccd601dbefec3f22042c8486f69b20ebd3fccabc87bf0f44b9c2cbedf

@Ocramius
Copy link
Member

#787 is the last bit missing here, but beware that we will only support php:^8.0.12 onwards.

@Ocramius Ocramius self-assigned this Oct 24, 2021
@Ocramius
Copy link
Member

Closing here: we'll probably still finish up PHP 8.1 support before releasing, but all the topics in here are pretty much exhausted 👍

@ondrejmirtes
Copy link
Contributor Author

Thank you, I'm really happy about this 😊 I'm probably gonna have to devote November and December to bringing PHP 8.1 support into PHPStan, and this is gonna help me a lot 😊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file enhancement
Projects
None yet
Development

No branches or pull requests

10 participants