PHP 8.0 | File::getMethodParameters(): bug fix for attributes leaking into typehint

[jrfnl]

File::getMethodParameters(): add test documenting behaviour for comments

This test demonstrates and documents the existing behaviour of the method when comments are encountered within a parameter declaration.

File:;getMethodParameters(): bug fix for attributes leaking into typehint

This commit adds handling of parameter attributes to the File::getMethodParameters() method as per option [2] discussed in issue #3298.

In practice this means that:

• [New] A new attributes index is introduced into the returned array which will hold a boolean value indicating whether attributes are attached to the parameter.
• [Unchanged] The content index in the returned array includes the textual representation of any attributes attached to a parameter.
• [Fixed] The type_hint and type_hint_token indexes will no longer be polluted (set incorrectly) with information belonging to the attribute(s) instead of to the type declaration.

Includes minor efficiency fix for handling of parenthesis and brackets in default values.

Includes dedicated unit test.

Fixes #3298

[gsherwood]

Do you think the new array index would be better named "has_attributes" to indicate that it's a boolean? I could make that change once I go to merge this if you agree.

[jrfnl]

Do you think the new array index would be better named "has_attributes" to indicate that it's a boolean?

I'm not fussed about the name either way, but yes, has_attributes did occur to me to.

I based the current name attributes on the existing naming of index keys used in this method. To be more specific: the other boolean values don't have a key with a verb attached.
I.e. the "is the variable passed by reference ?" index is pass_by_reference not is_reference. Similarly, the "is the type nullable" indicator is called nullable_type not is_nullable.

[jrfnl]

@gsherwood Shall I otherwise just remove the extra array index key for now ? We can always circle back to that if and when there are explicit requests for it.

[gsherwood]

Shall I otherwise just remove the extra array index key for now ? We can always circle back to that if and when there are explicit requests for it.

I think it's fine to leave it - the work has been done - but I will likely rename it.

I think pass_by_reference and nullable_type aren't great, but I don't think they would be confused with actually containing a non-boolean value because there is already another array index with the value extracted into it. But I think attributes could be confused with actually containing the value of the attributes themselves.

The actual value of the MR is in the fix itself, but I still think the new index is a nice addition if we can agree on a name. Given you had the exact same naming thought, if you're ok with some inconsistency in the naming (hopefully I explained why I am) then I think the index should stay.

[gsherwood]

Thanks a lot for this. I did end up changing the name of the array index to has_attributes in case attributes is ever needed to contain the attribute value itself.