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

Migrate zend_*_property APIs to accept zend_object* instead of zval* #5953

Closed
wants to merge 5 commits into from

Conversation

nikic
Copy link
Member

@nikic nikic commented Aug 7, 2020

As pointed out by @derickr, the current state of property APIs in PHP 8 is inconsistent. We've migrated large parts of the object-related API to accept zend_object* instead of zval*. However, the zend_*_property() family of functions still accepts zvals. This leaves us in an odd situation where the read_property() object handler accepts zend_object*, while the zend_read_property function accepts zval*. This is inconsistent and causes friction in places where such APIs interact, because it may be necessary to create dummy zval wrappers.

This patch changes the zend_*_property family of functions to require zend_object* as well, in line with underlying APIs.

Copy link
Contributor

@morrisonlevi morrisonlevi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strongly approve. I know we're getting closer and closer to API/ABI stability cutoffs for PHP 8 (we've released beta 1), but I would much rather make this tiny window of time more difficult and have it fixed.

Copy link
Contributor

@sgolemon sgolemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM; This is the sort of API break that belongs in a .0 release, so go ahead and merge when ready for inclusion in 8.0.0

@php-pulls php-pulls closed this in 45ece5b Aug 7, 2020
@Jan-E
Copy link
Contributor

Jan-E commented Aug 19, 2020

At least one extension is affected: microsoft/msphpsql#1154 (comment)

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

Successfully merging this pull request may close these issues.

4 participants