-
Notifications
You must be signed in to change notification settings - Fork 7.9k
Make the ABI compatibility of generated arginfo files configurable #8931
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
Conversation
That's great, I'll look more in detail tomorrow. At a quick glance though, would it be possible to make it a bit more fine-grained, in particular 7.2 gained a different representation for object arginfo and PHP 7.0 does not understand IS_VOID yet? |
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.
Thanks for the review request, but I don't feel qualified to review this for correctness. I've glanced over it, didn't see anything obviously wrong, but that doesn't really mean anything.
static function (array $value): bool { | ||
return !empty($value); | ||
}); |
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.
Closing brace should probably be on the line above.
Huh, yes, it is possible, and quite a few other things are still intentionally missing from my PR. The best course of action would be to make incremental improvements later. Also, I'm uncertain about what to do with features that cannot easily be "disabled" that easily, like intersection types, as we cannot simply get rid of the type declaration completely IMO. For now, I didn't deal with these kind of problematic cases, and only focused about the easiser stuff. |
well, do you have any better idea? |
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.
Excepted comment about minor cosmetic, seems OK from a quick test on Memcached ext.
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.
Looks good to me, can be iteratively improved upon for more fine-grained version differences. (once the segfault is fixed ...)
Not really :) I also thought about forbidding constructs which don't satisfy the minimum version constraints, but I guess this is not exactly what 3rd party extensions would need (?). |
In 8ac17ec I made a few smaller fixes so that legacy arginfo files won't even contain some of the PHP 8.0 constructs, while 7d47e50 addresses one of the comments.
It turned out that the segfault happens because |
This PR makes it possible for stubs of 3rd party extensions to declare their minimum PHP version compliance. This way, gen_stub.php won't generate symbols into arginfo files which are unavailable for older PHP versions.
In order to achieve this, the
@generate-legacy-arginfo
attribute is extended so that it takes a version ID parameter:70000
(PHP 7.0),80000
(PHP 8.0),80100
(PHP 8.1), or80200
(PHP 8.2). If there is no version constraint then70000
is assumed. Using this value is compatible with the current behavior (a legacy arginfo file is created too). However, if PHP 8.0+ compatibility is required then newer features (attributes, enums, readonly properties etc.) are conditionally enabled according to the version preferences.This means that 3rd party extensions can get rid of a bunch of forward compatibility code that they had to maintain just because gen_stub.php generated code for newer PHP versions.