Meaningful data type for _parameter_reference's "required" field

[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]

@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]

bump, did anyone look into it yet?

[nikic]

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]

@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]

@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.

[smalyshev]

What is the purpose of this line?

[smalyshev]

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 ?

[nikic]

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

[cataphract]

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

[paulbarbu]

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

[paulbarbu]

@cataphract Well, come on...

[smalyshev]

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

[raven0034]

is this ever going to get closed?

[php-pulls]

Comment on behalf of pollita at php.net:

Updated and merged e52d2b8