Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add sanity check in regenerating doc pages #518

Closed

Conversation

shreya0204
Copy link

Fixes: #507

  • As per the issue, the generation of internal-api was failing because of the warnings thrown by the following code.
  • The warnings originates from an assumption that preg_match would always find a match.
  • A sanity check has been added in the parse_docblock method to handle invalid JSON output gracefully.

@shreya0204 shreya0204 requested a review from a team as a code owner June 25, 2024 13:03
@@ -620,7 +620,7 @@ private static function parse_docblock( $docblock ) {
$ret['description'] .= PHP_EOL . "{$extra_line}{$info}";
} else {
preg_match( '/@(\w+)/', $info, $matches );
$param_name = $matches[1];
$param_name = count( $matches ) > 1 ? $matches[1] : null;
Copy link
Member

Choose a reason for hiding this comment

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

As per #507 (comment), the code used to work but has recently started generating PHP warnings. Can we investigate the root cause of this issue? The preg_match function may be returning an unexpected null value, and this safeguard may prematurely terminate the required documentation generation.

Copy link
Author

Choose a reason for hiding this comment

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

Hi @thelovekesh

Actually this ran a preg_match and in the next line we are getting the 2nd value of the $matches value without any proper conditional check if the 2nd index is present or not.

So, that's why these warnings were getting generated in the local, that "can't access this index of $matches".

So, therefore I am just adding a sanity check over here to check if the index is present, then only take that value, else assign it as null, which was anyhow happening if undefined index was accessed. Therefore as far I am seeing there shouldn't be any functional change in this change.

Please let me know if I am missing something and this can cause any regression functional change, as per you have mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

I tested this change in a separate PR. Yes, PHP error is now gone but parsing of docs of internal-api is not correct. If you like you can check this diff

Copy link
Author

Choose a reason for hiding this comment

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

@ernilambar Thank you for pointing this out. Not sure how this is causing such a significant change. Will surely look into it to find the root cause of it.

@ernilambar
Copy link
Member

This PR should fix the issue #520

@ernilambar ernilambar closed this Jun 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error regenerating doc pages
3 participants