-
Notifications
You must be signed in to change notification settings - Fork 7.8k
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
WIP: Enhance gen_stub.php #7700
base: master
Are you sure you want to change the base?
Conversation
m6w6
commented
Nov 28, 2021
•
edited
Loading
edited
- prop decls for ctor promoted props
- fix collision of namespaced functions
- fix generating multiple declaration prefixes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And please add a test case in zend_test
@@ -2369,7 +2369,7 @@ function parseFunctionLike( | |||
function parseProperty( | |||
Name $class, | |||
int $flags, | |||
Stmt\PropertyProperty $property, | |||
Stmt\PropertyProperty|Node\Param $property, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We aim for PHP 7.1 compatibility, so union types shouldn't be used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not understand. PHP-7 is in security-fix-only mode, so this PR is not going to land in PHP-7.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, but one can run gen_stubs.php via an earlier PHP version: e.g. I always use the version which is available on my the system (it's 8.0 in my case though).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, but ... 🤷
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not a negligible issue: gen_stub.php is automatically run by the build system during compilation via the php executable which is on path.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, what do you suggest? A runtime check for instanceof Stmt\PropertyProperty or Node\Param?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO a phpdoc would be enough, just like we did elsewhere, but I'm OK with a runtime check as well.
This reverts commit ad5021b.
930889a
to
d60b561
Compare
What's the status here? |
Kinda stuck. I'll have to find my most current stash and update it... |