Skip to content

PHP: Support variadic marked variables in PHPDoc#9487

Open
matthiasblaesing wants to merge 2 commits into
apache:masterfrom
matthiasblaesing:php_varargs
Open

PHP: Support variadic marked variables in PHPDoc#9487
matthiasblaesing wants to merge 2 commits into
apache:masterfrom
matthiasblaesing:php_varargs

Conversation

@matthiasblaesing

Copy link
Copy Markdown
Contributor

Variadic variables (...$variable) were not correctly parsed by the PHPDoc parser. The issue manifests like this:

  • variadic variables in PHPDoc are not highlighted by usage marker
  • renaming does not work for variadic marked variables

Closes: #9437

Variadic variables (...$variable) were not correctly parsed by the
PHPDoc parser. The issue manifests like this:

- variadic variables in PHPDoc are not highlighted by usage marker
- renaming does not work for variadic marked variables

Closes: apache#9437
@matthiasblaesing matthiasblaesing added this to the NB31 milestone Jul 5, 2026
@matthiasblaesing matthiasblaesing added PHP [ci] enable extra PHP tests (php/php.editor) ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) labels Jul 5, 2026
@matthiasblaesing

Copy link
Copy Markdown
Contributor Author

@junichi11 @tmysik it would be great if you could have a look.

ArrayList<String> types = new ArrayList<>();
if (tokens.length > 0 && (isReturnTag(tagType) || !tokens[0].startsWith("$"))) { //NOI18N
if (tokens.length > 0 && (isReturnTag(tagType)
|| !(tokens[0].startsWith("$") /* variable */ || tokens[0].startsWith("...$") /* variadic variable */ ))) { //NOI18N

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just braninstorming - would it make sense to have a utility method to detect "special" variables? I mean this tokens[0].charAt(0) == '$' || tokens[0].startsWith("...$"), we repeat it several times here.

@tmysik tmysik left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your work!

@matthiasblaesing

Copy link
Copy Markdown
Contributor Author

@tmysik thanks. I added a commit that consolidates the tests into a single method. I think it improves readability.

ArrayList<String> types = new ArrayList<>();
if (tokens.length > 0 && (isReturnTag(tagType)
|| !(tokens[0].startsWith("$") /* variable */ || tokens[0].startsWith("...$") /* variadic variable */ ))) { //NOI18N
if (tokens.length > 0 && (isReturnTag(tagType) || !isVariableName(tokens[0]))) { //NOI18N

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: //NOI18N can be removed.

return variable;
}

public static boolean isVariableName(String token) {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick: do we need //NOI18N to be used in this method?

@tmysik

tmysik commented Jul 7, 2026

Copy link
Copy Markdown
Member

@matthiasblaesing looks better to me now, thank you!

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

Labels

ci:dev-build [ci] produce a dev-build zip artifact (7 days expiration, see link on workflow summary page) PHP [ci] enable extra PHP tests (php/php.editor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[PHP] Refactor/Rename fails for PHPDoc @param on variadics

2 participants