Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Meaningful data type for _parameter_reference's "required" field #15

Open
wants to merge 2 commits into from

6 participants

@paulbarbu

Hi.

The aim of this patch is to fix a problem related to the _parameter_reference
structure and its required field.

The required field, until now, stored how many required parameters the function, it belongs to, has. I think this is rather problematic because it's a feature of the function to know how many required parameters it has, not of the parameter itself. The parameter should only say if it's required or optional (among other unrelated things).

Also storing the function's number of required parameters in every parameter was redundant since the _zend_function structure already has that information. And storing the same value (number of required parameters) across multiple variables is inefficient and could lead to inconsistencies.

So I tried to solve this problem by declaring the required field as zend_bool (see 7fe93ca66dd806a197).

While working on this I noticed some inconsistency between a comment and the code, bb59a5f5256df4d2e6.

I also updated the related reflection tests and everything seems ok (59fa4989f2ebb8d0).

While updating the tests I noticed a duplicate so I deleted it, see f613f01157abfc495.

Finally I thought var_dump() calls on a ReflectionParameter must be consistent with echo and print so I came up with db3c26b4e62361a.

PS:
You can assign the above stated problem with the following analogy.
A vehicle must know about its number of required or optional (in case of flats) wheels (parameters), the wheel itself should not know how many of it it takes to assemble the vehicle(the function) in order to run.
It only says that it's required when it's assembled on the vehicle and optional when serving as a back up wheel.

@paulbarbu

I added that to be consistent with echo or print, they already printed information about the "requiredness" of a parameter (because of __toString()) while var_dump() did not.

@paulbarbu

Done.
Out of curiosity, why are you saying it's odd?

@nikic
Owner

@dsp I'm not really sure that it's @paullik's fault. I think this is GitHub messing with the tabs. As you can see all tabs were converted to two whitespaces in the PR diff.

@paulbarbu

Oh, you're talking about the indentation?
On my machine everything is OK, I use (g)vim to program so the mode lines did their job.

@dsp
Owner

bump, did anyone look into it yet?

@nikic
Owner

I agree with @smalyshev that a public required property should not be exposed. All other info like whether it is a by-ref param or which typehint and default it has, aren't exposed as properties either. So that would be inconsistent.

@paulbarbu

Well, OK, but then why _parameter_string still "displays" whether that's a required parameter or not?

From my POV it's an inconsistency var_dump vs print/echo.

Anyway, waiting for advice.

@nikic
Owner

@paullik If you want to provide this info for var_dump and print_r a better way would be to register a get_debug_info object handler. It allows you to create a HT with pseudo-properties to be shown in var_dump output. Here is an example of what is done for closures: http://lxr.php.net/opengrok/xref/PHP_TRUNK/Zend/zend_closures.c#zend_closure_get_debug_info

@paulbarbu

Oh, it makes sense now.
I think I understand what I have to do in order to provide that information [1] and why my reflection_update_property way to solve the var_dump()'s output was inappropriate.

Thank you.

[1] I need to add a debug_info field to either _parameter_reference or to reflection_object, tough I'm not sure about this.

I'll update my patch once I get some free time.

@nikic
Owner

@paullik For now I'd just remove that part of the change so that the rest can be merged :) The debug handler(s) can always be added later :)

@paulbarbu

Done.
I reverted the public property thing and re-run the tests, everything seems OK.

@paulbarbu

Bump.

ext/reflection/tests/025.phpt
@@ -1,114 +1,34 @@
--TEST--
-ReflectionFunction basic tests
---SKIPIF--
-<?php extension_loaded('reflection') or die('skip'); ?>
+ReflectionExtension::info()
@smalyshev Owner

What is the purpose of this line?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@smalyshev
Owner

I'm not sure I understand - why you moved test 26 into test 25? Was something wrong with test 25?

@paulbarbu

Hmmm, I must have done something wrong somewhere because when I checked those two tests were the same.
I'm sorry for this mistake.

Maybe I should close this issue and post a new one with the good commits?

@reeze

@paullik maybe we can rebase this pull branch ?

ext/reflection/php_reflection.c
@@ -2024,9 +2034,17 @@ static int _zval_array_to_c_array(zval **arg, zval ****params TSRMLS_DC) /* {{{
array_init(return_value);
for (i = 0; i < fptr->common.num_args; i++) {
zval *parameter;
+ zend_bool required;
+
+ if(i >= fptr->common.required_num_args){
+ required = 0;
+ }
+ else{
+ required = 1;
+ }
@nikic Owner
nikic added a note

Btw, this could also be replaced by required = (i >= fptr->common.required_num_args) ? 0 : 1;.

If we want to bikeshed this, the proper suggestion would be required = (i < fptr->common.required_num_args).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@paulbarbu

@nikic updated to use the ternary operator.
@reeze rebased.

@paulbarbu

@cataphract Well, come on...

@smalyshev smalyshev commented on the diff
ext/reflection/php_reflection.c
((19 lines not shown))
if (arg_info->name) {
ZVAL_STRINGL(name, arg_info->name, arg_info->name_len, 1);
} else {
ZVAL_NULL(name);
}
+
+ ZVAL_BOOL(req, required);
@smalyshev Owner

why req is created here? doesn't look like it is going anywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dstogov dstogov referenced this pull request from a commit
@dstogov dstogov Added GC checks and improvements 276080e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 5, 2012
  1. @paulbarbu
  2. @paulbarbu

    changed _parameter_reference's 'required' field to a meaningful data …

    paulbarbu authored
    …type (zend_bool) for its purpose
This page is out of date. Refresh to see the latest.
Showing with 25 additions and 12 deletions.
  1. +25 −12 ext/reflection/php_reflection.c
View
37 ext/reflection/php_reflection.c
@@ -189,7 +189,7 @@ typedef struct _property_reference {
/* Struct for parameters */
typedef struct _parameter_reference {
zend_uint offset;
- zend_uint required;
+ zend_bool required;
struct _zend_arg_info *arg_info;
zend_function *fptr;
} parameter_reference;
@@ -693,10 +693,10 @@ static zend_op* _get_recv_op(zend_op_array *op_array, zend_uint offset)
/* }}} */
/* {{{ _parameter_string */
-static void _parameter_string(string *str, zend_function *fptr, struct _zend_arg_info *arg_info, zend_uint offset, zend_uint required, char* indent TSRMLS_DC)
+static void _parameter_string(string *str, zend_function *fptr, struct _zend_arg_info *arg_info, zend_uint offset, zend_bool required, char* indent TSRMLS_DC)
{
string_printf(str, "Parameter #%d [ ", offset);
- if (offset >= required) {
+ if (!required) {
string_printf(str, "<optional> ");
} else {
string_printf(str, "<required> ");
@@ -720,7 +720,7 @@ static void _parameter_string(string *str, zend_function *fptr, struct _zend_arg
} else {
string_printf(str, "$param%d", offset);
}
- if (fptr->type == ZEND_USER_FUNCTION && offset >= required) {
+ if (fptr->type == ZEND_USER_FUNCTION && !required) {
zend_op *precv = _get_recv_op((zend_op_array*)fptr, offset);
if (precv && precv->opcode == ZEND_RECV_INIT && precv->op2_type != IS_UNUSED) {
zval *zv, zv_copy;
@@ -766,7 +766,8 @@ static void _parameter_string(string *str, zend_function *fptr, struct _zend_arg
static void _function_parameter_string(string *str, zend_function *fptr, char* indent TSRMLS_DC)
{
struct _zend_arg_info *arg_info = fptr->common.arg_info;
- zend_uint i, required = fptr->common.required_num_args;
+ zend_uint i, num_required = fptr->common.required_num_args;
+ zend_bool required;
if (!arg_info) {
return;
@@ -775,6 +776,8 @@ static void _function_parameter_string(string *str, zend_function *fptr, char* i
string_printf(str, "\n");
string_printf(str, "%s- Parameters [%d] {\n", indent, fptr->common.num_args);
for (i = 0; i < fptr->common.num_args; i++) {
+ required = (i >= num_required) ? 0 : 1;
+
string_printf(str, "%s ", indent);
_parameter_string(str, fptr, arg_info, i, required, indent TSRMLS_CC);
string_write(str, "\n", sizeof("\n")-1);
@@ -1234,21 +1237,28 @@ static void reflection_extension_factory(zval *object, const char *name_str TSRM
/* }}} */
/* {{{ reflection_parameter_factory */
-static void reflection_parameter_factory(zend_function *fptr, zval *closure_object, struct _zend_arg_info *arg_info, zend_uint offset, zend_uint required, zval *object TSRMLS_DC)
+static void reflection_parameter_factory(zend_function *fptr, zval *closure_object, struct _zend_arg_info *arg_info, zend_uint offset, zend_bool required, zval *object TSRMLS_DC)
{
reflection_object *intern;
parameter_reference *reference;
zval *name;
+ zval *req;
if (closure_object) {
Z_ADDREF_P(closure_object);
}
+
MAKE_STD_ZVAL(name);
+ MAKE_STD_ZVAL(req);
+
if (arg_info->name) {
ZVAL_STRINGL(name, arg_info->name, arg_info->name_len, 1);
} else {
ZVAL_NULL(name);
}
+
+ ZVAL_BOOL(req, required);
@smalyshev Owner

why req is created here? doesn't look like it is going anywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+
reflection_instantiate(reflection_parameter_ptr, object TSRMLS_CC);
intern = (reflection_object *) zend_object_store_get_object(object TSRMLS_CC);
reference = (parameter_reference*) emalloc(sizeof(parameter_reference));
@@ -1980,7 +1990,7 @@ ZEND_METHOD(reflection_function, returnsReference)
/* }}} */
/* {{{ proto public bool ReflectionFunction::getNumberOfParameters()
- Gets the number of required parameters */
+ Gets the total number of parameters */
ZEND_METHOD(reflection_function, getNumberOfParameters)
{
reflection_object *intern;
@@ -2024,9 +2034,12 @@ ZEND_METHOD(reflection_function, getParameters)
array_init(return_value);
for (i = 0; i < fptr->common.num_args; i++) {
zval *parameter;
+ zend_bool required;
+
+ required = (i >= fptr->common.required_num_args) ? 0 : 1;
ALLOC_ZVAL(parameter);
- reflection_parameter_factory(_copy_function(fptr TSRMLS_CC), intern->obj, arg_info, i, fptr->common.required_num_args, parameter TSRMLS_CC);
+ reflection_parameter_factory(_copy_function(fptr TSRMLS_CC), intern->obj, arg_info, i, required, parameter TSRMLS_CC);
add_next_index_zval(return_value, parameter);
arg_info++;
@@ -2254,7 +2267,7 @@ ZEND_METHOD(reflection_parameter, __construct)
ref = (parameter_reference*) emalloc(sizeof(parameter_reference));
ref->arg_info = &arg_info[position];
ref->offset = (zend_uint)position;
- ref->required = fptr->common.required_num_args;
+ ref->required = position >= fptr->common.required_num_args ? 0 : 1;
ref->fptr = fptr;
/* TODO: copy fptr */
intern->ptr = ref;
@@ -2499,7 +2512,7 @@ ZEND_METHOD(reflection_parameter, isOptional)
}
GET_REFLECTION_OBJECT_PTR(param);
- RETVAL_BOOL(param->offset >= param->required);
+ RETVAL_BOOL(!param->required);
}
/* }}} */
@@ -2520,7 +2533,7 @@ ZEND_METHOD(reflection_parameter, isDefaultValueAvailable)
{
RETURN_FALSE;
}
- if (param->offset < param->required) {
+ if (param->required) {
RETURN_FALSE;
}
precv = _get_recv_op((zend_op_array*)param->fptr, param->offset);
@@ -2549,7 +2562,7 @@ ZEND_METHOD(reflection_parameter, getDefaultValue)
zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC, "Cannot determine default value for internal functions");
return;
}
- if (param->offset < param->required) {
+ if (param->required) {
zend_throw_exception_ex(reflection_exception_ptr, 0 TSRMLS_CC, "Parameter is not optional");
return;
}
Something went wrong with that request. Please try again.