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

Make the ABI compatibility of generated arginfo files configurable #8931

Merged
merged 1 commit into from
Jul 12, 2022

Conversation

kocsismate
Copy link
Member

@kocsismate kocsismate commented Jul 5, 2022

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), or 80200 (PHP 8.2). If there is no version constraint then 70000 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.

@kocsismate kocsismate changed the title Fix GH-8842 Make the ABI compatibility of generated arginfo files config Fix GH-8842 Make the ABI compatibility of generated arginfo files configurable Jul 5, 2022
@bwoebi
Copy link
Member

bwoebi commented Jul 5, 2022

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?

Copy link
Member

@TimWolla TimWolla left a 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.

Comment on lines +4095 to +4107
static function (array $value): bool {
return !empty($value);
});
Copy link
Member

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.

@kocsismate kocsismate changed the title Fix GH-8842 Make the ABI compatibility of generated arginfo files configurable Make the ABI compatibility of generated arginfo files configurable Jul 6, 2022
@kocsismate
Copy link
Member Author

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?

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.

@bwoebi
Copy link
Member

bwoebi commented Jul 6, 2022

like intersection types, as we cannot simply get rid of the type declaration completely IMO

well, do you have any better idea?

Copy link
Member

@remicollet remicollet left a 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.

build/gen_stub.php Outdated Show resolved Hide resolved
Copy link
Member

@bwoebi bwoebi left a 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 ...)

@kocsismate
Copy link
Member Author

kocsismate commented Jul 9, 2022

well, do you have any better idea?

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 (?).

@kocsismate
Copy link
Member Author

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.

Looks good to me, can be iteratively improved upon for more fine-grained version differences. (once the segfault is fixed ...)

It turned out that the segfault happens because PHP_VERSION_ID wasn't available in the arginfo files. So I added php_version.h in 52e61ba and now, there shouldn't be any test failures.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants