Added parsing of PHPDoc blocks based on nikic/php-parser and PHPDoc parser by phpstan. #41306

zinigor · 2025-01-24T12:41:22Z

Proposed changes:

Makes the doc parser that hasn't previously been used switch to nikic/php-parser with help from phpstan/phpdoc-parser.
Adds detection of apply_filters and do_action calls.
Removes the Markdown doc parsing because it's not used anymore, or ever.
Removes some unneeded dependencies.
Changes the Jetpack CLI docs command to require a Jetpack production checkout path instead.

Other information:

Have you written new tests for your changes, if applicable?
Have you checked the E2E test CI results, and verified that your changes do not break them?
Have you tested your changes on WordPress.com, if applicable (if so, you'll see a generated comment below with a script to run)?

Jetpack product discussion

Does this pull request change what data or activity we track or use?

No.

Testing instructions:

Checkout this branch.
Run jetpack docs /path/to/jetpack-production/checkout
Observe the phpdoc.json file generated in the folder where you're running the command from.

github-actions · 2025-01-24T12:42:34Z

Thank you for your PR!

When contributing to Jetpack, we have a few suggestions that can help us test and review your patch:

✅ Include a description of your PR changes.
✅ Add a "[Status]" label (In Progress, Needs Team Review, ...).
✅ Add a "[Type]" label (Bug, Enhancement, Janitorial, Task).
✅ Add testing instructions.
✅ Specify whether this PR includes any changes to data or privacy.
✅ Add changelog entries to affected projects

This comment will be updated as you work on your PR and make changes. If you think that some of those checks are not needed for your PR, please explain why you think so. Thanks for cooperation 🤖

The e2e test report can be found here. Please note that it can take a few minutes after the e2e tests checks are complete for the report to be available.

Follow this PR Review Process:

Ensure all required checks appearing at the bottom of this PR are passing.
Choose a review path based on your changes:
- A. Team Review: add the "[Status] Needs Team Review" label
  - For most changes, including minor cross-team impacts.
  - Example: Updating a team-specific component or a small change to a shared library.
- B. Crew Review: add the "[Status] Needs Review" label
  - For significant changes to core functionality.
  - Example: Major updates to a shared library or complex features.
- C. Both: Start with Team, then request Crew
  - For complex changes or when you need extra confidence.
  - Example: Refactor affecting multiple systems.
Get at least one approval before merging.

Still unsure? Reach out in #jetpack-developers for guidance!

tools/cli/helpers/doc-parser/src/class-doc-parser.php

tbradsha

I can confirm it generates both a phpdoc.json and a markdown.json. I'm not familiar enough with AST parsing to be confident as to whether it's doing what it should at a low level, though.

I'll note that there are two deprecation notices from the scribu library, and several instances of this:

Warning: DOMDocument::loadHTML(): Tag summary invalid in Entity, line: 376 in /some/path/jetpack/tools/cli/helpers/doc-parser/runner.php on line 72

…des.

tbradsha

I'm not confident in my understanding of the actual parsing, but it gave no errors or warnings when run as indicated. Various spot-checked hooks and filters seem to match, including those in jetpack_vendor.

I did get a PHP fatal when I didn't pass a path, so it'd probably be good to catch that with a simple check (see inline comment).

Non-blocking thoughts:

It dumps phpdoc.json in the monorepo root rather than in the current directory, which was unexpected for me.
It'd be nice to be able to specify the name of the output file in case we want to generate and compare two files easily.

tools/cli/commands/docs.js

tools/cli/helpers/doc-parser/src/class-doc-parser.php

anomiex

Most of the comments (e.g. anything talking about adding diagnostics for a potential CI run) can be left for a followup as far as I'm concerned. 🙂

anomiex · 2025-01-31T18:59:23Z

composer.lock

Looks like this lock file accidentally got updated? Doesn't matter much if it does, it's just unexpected.

Yes, I think I did it by running composer update in root instead of the doc parser.

tools/cli/helpers/doc-parser/src/class-doc-parser.php

anomiex · 2025-01-31T20:12:18Z

tools/cli/helpers/doc-parser/src/class-doc-parser.php

+				|| $argument->value instanceof Node\Expr\ConstFetch
+				|| $argument->value instanceof Node\Expr\ClassConstFetch


These cases probably aren't too useful as-is for the documentation. We want the doc to say "stats_array" or "earn_premium_content_subscription_service", not "self::STATS_ARRAY_TO_STRING_FILTER" or "PAYWALL_FILTER".

Looks like those are the only two we currently have in jetpack-production, BTW. In both cases the constant is defined in the same file, so (probably in a followup) we could probably have some code check for definitions.

Anyway, leaving all that for a followup seems fine to me.

tools/cli/helpers/doc-parser/src/class-doc-parser.php

anomiex · 2025-01-31T20:26:40Z

tools/cli/helpers/doc-parser/src/class-doc-parser.php

+			}
+			return $result;
+
+		} elseif ( $argument->value instanceof Node\Expr\BinaryOp\Concat ) {


This whole case seems like it won't be terribly useful for documentation either.

Most of the existing examples could be turned into interpolated strings easily enough.

'jetpack_module_loaded_' . $module 'jetpack_module_configuration_url_' . $module 'jetpack_learn_more_button_' . $module 'jetpack_module_more_info_' . $module 'jetpack_search_terms_' . $module 'jetpack_module_configurable_' . $module 'jetpack_module_more_info_' . $module_slug 'site_settings_endpoint_update_' . $key 'jetpack_feature_' . $feature . '_enabled' 'jetpack_sync_before_send_' . $item[0] 'jetpack_sync_before_send_queue_' . $queue->id 'jitm_' . $envelope->CTA->hook 'jitm_' . $envelope->content->hook 'pre_update_jetpack_option_' . $name 'jetpack_learn_more_button_' . $item['module'] 'jetpack_module_more_info_' . $item['module'] 'jetpack_module_configurable_' . $plugin['module']

A few would need assigning to a temporary variable for the interpolation, which would probably be worthwhile.

'jetpack_comment_form_enabled_for_' . get_post_type() 'jetpack_comment_form_enabled_for_' . get_post_type() 'jetpack_comment_form_enabled_for_' . get_post_type($post_array['comment_post_ID'])

In this case, I'd personally probably make a separate PR to fix all those to interpolated-strings so we can drop this case.

anomiex · 2025-01-31T20:27:41Z

tools/cli/helpers/doc-parser/src/class-doc-parser.php

 		}

-		return $output;
+		throw new \Exception( 'Unexpected function call argument of type ' . get_class( $argument->value ) );


When we do the diagnostics, this should probably be turned into one instead of killing the whole process. 😉

tools/cli/helpers/doc-parser/src/class-doc-parser.php

Co-authored-by: tbradsha <[email protected]>

Co-authored-by: Brad Jorsch <[email protected]>

…-based-on-ast' into add/docblock-parser-based-on-ast

zinigor · 2025-02-05T17:21:22Z

@tbradsha and @anomiex I think I have implemented blocking review items, and I would like to leave everything else for the next PRs if you don't mind.

anomiex

Works for me to merge this and iterate.

anomiex · 2025-02-05T18:53:28Z

tools/cli/helpers/doc-parser/src/class-doc-parser.php

+			$name_string = $this->pretty_print_hook_name( $hook_name );
+
+			if ( false === $name_string ) {
+				continue;
+			}

 			// Purging any comments that could have been attributed to this argument.
 			$hook_name->setAttribute( 'comments', null );


Suggested change

$name_string = $this->pretty_print_hook_name( $hook_name );

if ( false === $name_string ) {

continue;

}

// Purging any comments that could have been attributed to this argument.

$hook_name->setAttribute( 'comments', null );

// Purging any comments that could have been attributed to this argument.

$hook_name->setAttribute( 'comments', null );

$name_string = $this->pretty_print_hook_name( $hook_name );

if ( false === $name_string ) {

continue;

}

anomiex · 2025-02-05T18:53:54Z

tools/cli/helpers/doc-parser/src/class-doc-parser.php

+				$n          = $n->getAttribute( 'parent' );
+				$docComment = $n->getDocComment();
+			}
+
 			$new_block = array(
 				'type'      => $node->name->name === 'apply_filters' ? 'filter' : 'action',
 				'line'      => $node->getLine(),
 				'end_line'  => $node->getEndLine() > 0 ? $node->getEndLine() : $node->getLine(),
 				'name'      => $this->pretty_print_hook_name( $hook_name ),


Suggested change

'name' => $this->pretty_print_hook_name( $hook_name ),

'name' => $name_string,

anomiex · 2025-02-05T19:01:02Z

tools/cli/helpers/doc-parser/src/class-doc-parser.php

 				}
 			}
 			return $result;
+		} elseif ( $argument->value instanceof Node\Expr\Variable ) {
+
+			// We don't care about variable names, we can't really document them.


Alternatively, we should avoid using variables so we can document the hooks correctly?

tbradsha

Looks good to merge. I wasn't actually expecting you to incorporate my suggestions in this PR. :^)

Please do merge, and consider the following non-blocking feedback for future iterations:

When I run jetpack docs (no optional params), it stays silent for quite some time, so it's unclear it's working. It'd be good to give some kind of immediate feedback to the user.
I'd probably still require the path-to-project.
For the destination path, I was expecting to be able to specify the output filename, not just the path. In a perfect world, I'd be able to specify a path (and it uses that path + default filename) or a path + filename.
If I provide an invalid path I'd expect an immediate error, but it does something weird:

$ jetpack docs asdf
Can't find source directory at: asdf
Preparing doc-parser package...
Can't find source directory at: asdf

Initial commit.

b2b3f0c

github-actions bot added [Status] In Progress [Tools] Development CLI The tools/cli to assist during JP development. labels Jan 24, 2025

github-actions bot added the [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! label Jan 24, 2025

zinigor added 4 commits January 24, 2025 17:12

Added arguments to description, fixed bugs.

231c038

Explain types to Phan.

324d26a

Removed unneeded phpcs ignore.

09f5dba

Fixed more bugs.

bb880a9

zinigor requested a review from a team January 24, 2025 17:42

zinigor marked this pull request as ready for review January 24, 2025 17:43

github-actions bot added the [Status] In Progress label Jan 24, 2025

zinigor commented Jan 24, 2025

View reviewed changes

tools/cli/helpers/doc-parser/src/class-doc-parser.php Outdated Show resolved Hide resolved

zinigor removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Jan 24, 2025

tbradsha reviewed Jan 24, 2025

View reviewed changes

zinigor added 9 commits January 27, 2025 19:27

Merge branch 'trunk' into add/docblock-parser-based-on-ast

bafcb22

Added since tags.

72d4458

Added a couple of prop types, expanded the flattening routine.

ac29a44

Moved to nikic/php-parser instead of AST to simplify flattening of no…

4aca56f

…des.

Merge branch 'trunk' into add/docblock-parser-based-on-ast

646c4dc

Fixed tags, added pretty printing, multiple other fixes.

8f09f5d

Updated the CLI command and removed markdown generation.

236cdcf

Merge branch 'trunk' into add/docblock-parser-based-on-ast

02c05d6

Added actions too.

2eb6aae

zinigor changed the title ~~Added parsing of PHPDoc blocks based on AST and PHPDoc parser by phpstan.~~ Added parsing of PHPDoc blocks based on nikic/php-parser and PHPDoc parser by phpstan. Jan 30, 2025

Removed the Markdown parser library for now.

dcaec22

Added comment purging to purify hook names.

29805b0

tbradsha reviewed Jan 31, 2025

View reviewed changes

tools/cli/commands/docs.js Outdated Show resolved Hide resolved

tools/cli/commands/docs.js Outdated Show resolved Hide resolved

tbradsha reviewed Jan 31, 2025

View reviewed changes

tools/cli/helpers/doc-parser/src/class-doc-parser.php Outdated Show resolved Hide resolved

anomiex reviewed Jan 31, 2025

View reviewed changes

zinigor and others added 15 commits February 3, 2025 20:32

Update tools/cli/commands/docs.js

b5f021e

Co-authored-by: tbradsha <[email protected]>

Update tools/cli/helpers/doc-parser/src/class-doc-parser.php

c04a1f9

Co-authored-by: Brad Jorsch <[email protected]>

Update tools/cli/helpers/doc-parser/src/class-doc-parser.php

6e75fc3

Co-authored-by: Brad Jorsch <[email protected]>

Added path defaults, added output destination parameter.

524a748

Merge remote-tracking branch 'refs/remotes/origin/add/docblock-parser…

d9c915c

…-based-on-ast' into add/docblock-parser-based-on-ast

Merge branch 'trunk' into add/docblock-parser-based-on-ast

464fe22

Fixed the definition.

1a5d82a

Merge branch 'trunk' into add/docblock-parser-based-on-ast

1b9c215

Renamed the method to better reflect its purpose.

e78fc54

Using UnexpectedValueException.

721fdad

Removed unnecessary ignores.

1685c42

Added parser-powered docblock extraction.

2628ee7

Removed old unused format switch.

ed9d743

Added a suppression on the line instead of in the function.

b4bc7ae

Added more esoteric filter and action calls.

9bc4dbb

zinigor added [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. and removed [Status] Needs Author Reply We would need you to make some changes or provide some more details about your PR. Thank you! labels Feb 5, 2025

zinigor requested review from tbradsha and anomiex February 5, 2025 17:54

anomiex approved these changes Feb 5, 2025

View reviewed changes

tbradsha approved these changes Feb 5, 2025

View reviewed changes

zinigor merged commit 258adb2 into trunk Feb 6, 2025
60 checks passed

zinigor deleted the add/docblock-parser-based-on-ast branch February 6, 2025 11:44

github-actions bot removed the [Status] Needs Review To request a review from fellow Jetpack developers. Label will be renamed soon. label Feb 6, 2025

zinigor self-assigned this Feb 6, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added parsing of PHPDoc blocks based on nikic/php-parser and PHPDoc parser by phpstan. #41306

Added parsing of PHPDoc blocks based on nikic/php-parser and PHPDoc parser by phpstan. #41306

zinigor commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

tbradsha left a comment

tbradsha left a comment •

edited

Loading

anomiex left a comment

anomiex Jan 31, 2025

zinigor Feb 3, 2025

anomiex Jan 31, 2025

anomiex Jan 31, 2025

anomiex Jan 31, 2025

zinigor commented Feb 5, 2025

anomiex left a comment

anomiex Feb 5, 2025

anomiex Feb 5, 2025

anomiex Feb 5, 2025

tbradsha left a comment

		\|\| $argument->value instanceof Node\Expr\ConstFetch
		\|\| $argument->value instanceof Node\Expr\ClassConstFetch

	'name' => $this->pretty_print_hook_name( $hook_name ),
	'name' => $name_string,

Added parsing of PHPDoc blocks based on nikic/php-parser and PHPDoc parser by phpstan. #41306

Added parsing of PHPDoc blocks based on nikic/php-parser and PHPDoc parser by phpstan. #41306

Conversation

zinigor commented Jan 24, 2025 • edited Loading

Proposed changes:

Other information:

Jetpack product discussion

Does this pull request change what data or activity we track or use?

Testing instructions:

github-actions bot commented Jan 24, 2025 • edited Loading

tbradsha left a comment

Choose a reason for hiding this comment

tbradsha left a comment • edited Loading

Choose a reason for hiding this comment

anomiex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zinigor commented Feb 5, 2025

anomiex left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbradsha left a comment

Choose a reason for hiding this comment

zinigor commented Jan 24, 2025 •

edited

Loading

github-actions bot commented Jan 24, 2025 •

edited

Loading

tbradsha left a comment •

edited

Loading