Generate hook names using parsed Node instead of pretty-printed name.

[dmsnell]

Description

Previously, the hook-name printer was reyling on the parser's pretty printer to generate names, and then post-processing the name to extract string literal values and concatenations.

Unfortunately this left some cases unhandled, specifically the case where concatenations are joining function calls with string literals. Any number of other unexpected situations might arise, and there can be defects in the redundant code attempting to parse PHP syntax.

In this patch the pretty-printed version of the expression is used only as a fallback in unrecognized situations. Primarily, a direct encoding from the parsed syntax tree to string is used to rely on the parser's own handling of syntax, and making it clearer how to add additional support for other syntaxes.

Testing

In order to compare the output against WordPress Core I wrote a report that stores every hook name as it generated. The following version of Hook_Reflector::getName() will write such a report.

public function getName() {
	if ( null === $this->report ) {
		$this->report = fopen( $path_to_report, 'a' );
	}

	$name = $this->cleanupName( $this->node->args[0]->value );
	fwrite( $this->report, $name . "\n" );
	return $name;
}

After running the docs generation with and without this patch, I found four differences by comparing the output reports with diff -u hook-names.txt hook-names-patch.txt.

--- hook-names.txt	2024-03-20 11:37:54
+++ hook-names-patch.txt	2024-03-20 10:21:01
@@ -82,7 +82,7 @@
 wpmuadminedit
-'handle_network_bulk_actions-' . get_current_screen()->id
+handle_network_bulk_actions-{get_current_screen()->id}
 activate_blog
@@ -90,7 +90,7 @@
 wpmu_options
-'handle_network_bulk_actions-' . get_current_screen()->id
+handle_network_bulk_actions-{get_current_screen()->id}
 pre_network_site_new_created_user
@@ -98,7 +98,7 @@
 wpmuadminedit
-'handle_network_bulk_actions-' . get_current_screen()->id
+handle_network_bulk_actions-{get_current_screen()->id}
 wpmu_update_blog_options
@@ -108,7 +108,7 @@
 network_site_users_created_user
-'handle_network_bulk_actions-' . get_current_screen()->id
+handle_network_bulk_actions-{get_current_screen()->id}
 show_network_site_users_add_existing_form

As is evident, the impact of this patch on Core is small, but might make for a more obvious path to adding further support for other ways that the hook functions may be called. In this case, I have arbitrarily chosen to embed the call to get_current_screen()->id as if it were interpolated into the string in the same way that a variable would be, less the $.

[jrfnl]

phpdoc-parser/composer.json

Line 23 in 7fc2227

"php" : ">=5.4",

[dmsnell]

@jrfnl can you provide some context or clarification for your comment?

are you suggesting we update the minimum supported version in this parser to what WordPress supports, to 7.4?

[jrfnl]

I'm not suggesting anything. I'm only pointing out that as things are at the moment, this change cannot go in.

Either the minimum PHP version needs to change or the syntax/functionality used needs to be made compatible with the minimum supported PHP version.

[dmsnell]

updated the minimum version required in d57e2ed

[jrfnl]

I don't know where you are getting your information from, but WP Core still has a minimum supported PHP version of PHP 7.0....

[jrfnl]

As a side-note: I believe that whether or not it is acceptable to change the minimum PHP version of this package should be a separate discussion and should not be mixed in with a proposal for a (functional) change.

[dmsnell]

I don't know where you are getting your information from

that's why I left the link in the original message 😄
but thanks for correcting me: I was reviewing the recommended setup, not the minimum.

As a side-note: I believe that whether or it is acceptable to change the minimum PHP version of this package should be a separate discussion and should not be mixed in with a proposal for a (functional) change.

sounds like a good suggestion, which is why I asked if you meant something when you called out the line. I've updated this to avoid the ?? in 80ded80. thanks!

[kkmuffme]

This code introduces a bug for do_action( 'woocommerce_admin_field_' . $value['type'], $value ); which will be returned as woocommerce_admin_field_$value['type'] instead of woocommerce_admin_field_{$value['type']} as it did previously, but that can be easily fixed by just adding a condition for array.

I would suggest to keep the original "cleanupName" as a fallback for the return $name; case to avoid accidentally breaking stuff for lots of people.

A variety of other common cases that should get handled in elseif in this PR can be found in the function here
https://github.com/psalm/psalm-plugin-wordpress/blob/master/Plugin.php#L378 since those cases are things that we or people reporting an issue there encountered in the wild. You can basically just copy paste the conditions from there I guess.