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

ZEND_COMPILE_GUARDS compiler option #1528

Closed
wants to merge 1 commit into from
Closed

ZEND_COMPILE_GUARDS compiler option #1528

wants to merge 1 commit into from

Conversation

krakjoe
Copy link
Member

@krakjoe krakjoe commented Sep 20, 2015

See PR #1524 for conversation, this is the tidiest solution, we think.

@krakjoe
Copy link
Member Author

krakjoe commented Sep 20, 2015

@zenovich is this okay for you (I have tested) ??

@dstogov @nikic anything missing ??

@zenovich
Copy link

Yeah, this is wonderful!

@laruence laruence added the Bug label Sep 21, 2015
@weltling
Copy link
Contributor

@krakjoe, @zenovich do i get it right - you want to be able to patch some class at run time, but only overriding the predefined object handlers? So or is it also about a particular object, not class in general? And for that in some runkit-alike extension you'll be donig in MINIT like CG(compiler_options) |= ZEND_COMPILE_GUARDS; ?

Just to ensure i read it right in the previous discussion in #1524. Maybe you could provide some use case code to understand what is intended to be done?

Thanks.

@krakjoe
Copy link
Member Author

krakjoe commented Sep 21, 2015

The use case is runkit and uopz like extensions that want to be able to add magic methods to class entries at runtime.

Yeah, the intended use is to set compiler options in MINIT.

Here is a usage gist, to be clear.

@zenovich
Copy link

@weltling Currently Runkit can't patch a particular object. It's able to patch classes at runtime, but this affects all objects of patched classes automatically due to PHP internal mechanics.

Here and in #1524 we are talking about adding magic methods such as __get, __set, __isset or __unset at runtime. Look at this code:

class A {}
$a = new();
runkit_method_add('A', '__get', function ($name) {return $name;});
echo $a['property'];

Here we added a getter-method to the class, but the object has no guards (the last item) allocated in the properties_table, so PHP will try to access past the object.

Having ZEND_COMPILE_GUARDS compiler option in Zend Engine, Runkit will wrap functions zend_compile_file and zend_compile_string to set this flag and resolve the issue.

@weltling
Copy link
Contributor

@krakjoe I see, the absense of such a hook would effectively block the uopz port. Your implementation looks smooth. If Dmitry and Nikita are ok with the implementation, i'd be going with 7.0 /cc @KalleZ

Thanks.

@weltling
Copy link
Contributor

@zenovich are you porting the runkit ext to 7.0?

Thanks.

@zenovich
Copy link

@weltling Probably I will. For now I'm studying the posibilities of this porting.

@dstogov
Copy link
Member

dstogov commented Sep 22, 2015

Looks fine. I suggest to commit this into PHP-7.0

On Sun, Sep 20, 2015 at 8:29 AM, Joe Watkins notifications@github.com
wrote:

@zenovich https://github.com/zenovich, is this okay for you ??

@dstogov https://github.com/dstogov @nikic https://github.com/nikic
anything missing ??


Reply to this email directly or view it on GitHub
#1528 (comment).

@Tyrael
Copy link
Member

Tyrael commented Sep 22, 2015

Just my two cents and nothing against this specific PR, but we should really stop adding features into php-7.0, even if they are internal facing and/or approved by Dmitry.

@weltling
Copy link
Contributor

This patch is fixing a blocker for some extensions and does no harm otherwise, so is fine. Yeah, Ferenc, otherwise I guess we should get much more restrictive, with RC5 anyway. Some even small features can have bugs. Better we miss one or another new feature at this point than lose a iota on stability :)

Cheers.

@dstogov
Copy link
Member

dstogov commented Sep 22, 2015

some missing features are required by external extensions.
They may be completely broken and couldn't work with php-7 anymore.
We should try to satisfy their needs with minimal changes (if possible).
I agree, that most other new features and optimizations should be frozen.

On Tue, Sep 22, 2015 at 9:45 AM, Ferenc Kovacs notifications@github.com
wrote:

Just my two cents and nothing against this specific PR, but we should
really stop adding features into php-7.0, even if they are internal facing
and/or approved by Dmitry.


Reply to this email directly or view it on GitHub
#1528 (comment).

@php-pulls
Copy link

Comment on behalf of bwoebi at php.net:

Merged as 1a5d6ac

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

Successfully merging this pull request may close these issues.

7 participants