Skip to content

Make zend_closure public #3751

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

Closed
wants to merge 1 commit into from
Closed

Make zend_closure public #3751

wants to merge 1 commit into from

Conversation

sergeyklay
Copy link

Make zend_closure structure public so that third party extensions will have full access to this data type.

Make `zend_closure` structure public so that third party extensions will
have full access to this data type.
@nikic
Copy link
Member

nikic commented Jan 17, 2019

What is the specific use-case here? Which parts of the closure do you need to access?

@sergeyklay
Copy link
Author

@nikic It is my personal use case, however, I am sure that I am not alone :)

/* Imported since PHP is so nice to define this in a .c file ¯\_(ツ)_/¯ */
typedef struct _zend_closure {
	zend_object       std;
	zend_function     func;
	zval              this_ptr;
	zend_class_entry *called_scope;
#if PHP_VERSION_ID >= 70300
	zif_handler       orig_internal_handler;
#else
	void (*orig_internal_handler)(INTERNAL_FUNCTION_PARAMETERS);
#endif
} zend_closure;

int create_closure_ex(zval *return_value, zval *this_ptr, zend_class_entry *ce, const char *method_name, zend_uint method_length)
{
	zend_function *function_ptr;
	zend_closure *closure;

	if ((function_ptr = zend_hash_str_find_ptr(&ce->function_table, method_name, method_length)) == NULL) {
		ZVAL_NULL(return_value);
		return FAILURE;
	}
	
	zend_create_closure(return_value, function_ptr, ce, ce, this_ptr);
	
	// Make sure we can use a closure multiple times
	closure = (zend_closure*)Z_OBJ_P(return_value);
	closure->func.internal_function.handler = closure->orig_internal_handler;
	
	return SUCCESS;
}

@nikic
Copy link
Member

nikic commented Jan 17, 2019

@sergeyklay This code looks rather suspect to me. The closure internal handler is there for a reason... Most likely, you are working around a bug in the code that is calling the closure here (which likely fails to increase the closure refcount prior to the call).

@sergeyklay
Copy link
Author

Yes, he is really suspicious. You're right. However, it works and solves the particular business task. In general access to the API is not so bad, isn't? I could be wrong, however I see no obvious reasons to make zend_closure internal. For me personally, the reasons why this structure is private are not clear. I assumed that they do not exist. That is why I decided to correct this misunderstanding. Could you elaborate on that?

@nikic
Copy link
Member

nikic commented Jan 17, 2019

Yes, he is really suspicious. You're right. However, it works and solves the particular business task. In general access to the API is not so bad, isn't? I could be wrong, however I see no obvious reasons to make zend_closure internal. For me personally, the reasons why this structure is private are not clear. I assumed that they do not exist. That is why I decided to correct this misunderstanding. Could you elaborate on that?

All structures are private unless there is a reason for them to be public. There are two motivations for this: First, it allows us to change the structure in a patch release. Otherwise this would break our (strict) source and ABI compatibility guarantees for patch releases. Second, it forces developers to interact with internal structures only through exported public APIs, preventing mistakes and misuse.

@sergeyklay
Copy link
Author

sergeyklay commented Jan 17, 2019

@nikic Sounds reasonable. Well, although this patch can not be merged into the upstream, at least it works for me :) I will try to deal with this on my own, perhaps I will get rid of the need to get access to the zend_closure. Thank you

@sergeyklay sergeyklay closed this Jan 17, 2019
@sergeyklay sergeyklay deleted the fix/make-zend-closure-public branch January 17, 2019 13:39
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.

2 participants