Skip to content

Infrastructure/11293 phpcs rule fullstop#13148

Open
ankitrox wants to merge 5 commits into
developfrom
infrastructure/11293-phpcs-rule-fullstop
Open

Infrastructure/11293 phpcs rule fullstop#13148
ankitrox wants to merge 5 commits into
developfrom
infrastructure/11293-phpcs-rule-fullstop

Conversation

@ankitrox

Copy link
Copy Markdown
Collaborator

Summary

Addresses issue:

Relevant technical choices

PR Author Checklist

  • My code is tested and passes existing unit tests.
  • My code has an appropriate set of unit tests which all pass.
  • My code is backward-compatible with WordPress 5.2 and PHP 7.4.
  • My code follows the WordPress coding standards.
  • My code has proper inline documentation.
  • I have added a QA Brief on the issue linked above.
  • I have signed the Contributor License Agreement (see https://cla.developers.google.com/).

Do not alter or remove anything below. The following sections will be managed by moderators only.

Code Reviewer Checklist

  • Run the code.
  • Ensure the acceptance criteria are satisfied.
  • Reassess the implementation with the IB.
  • Ensure no unrelated changes are included.
  • Ensure CI checks pass.
  • Check Storybook where applicable.
  • Ensure there is a QA Brief.
  • Ensure there are no unexpected significant changes to file sizes.

Merge Reviewer Checklist

  • Ensure the PR has the correct target branch.
  • Double-check that the PR is okay to be merged.
  • Ensure the corresponding issue has a ZenHub release assigned.
  • Add a changelog message to the issue.

@github-actions

github-actions Bot commented Jul 20, 2026

Copy link
Copy Markdown

🤖 This comment is automatically updated by CI workflows. Each section is managed independently.

📦 Build files for b1fcdc7:

🎭 Playwright reports for b1fcdc7:

@techanvil techanvil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @ankitrox, this is looking good. I've left some comments, please take a look.

* ## EXAMPLES
*
* wp google-site-kit auth disconnect --id=11
* wp google-site-kit auth disconnect --id=11.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The rule should be disabled for this comment, and this change reverted.

Comment thread includes/Core/CLI/Reset_CLI_Command.php Outdated
*
* wp google-site-kit reset
* wp google-site-kit reset --persistent
* wp google-site-kit reset --persistent.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The rule should be disabled for this comment, and this change reverted.

// which requires authorization, however script_loader_tag is also filtered
// to apply script_execution attributes for all users, so it must be excluded here.
// 'script_loader_tag',
// 'script_loader_tag',.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's remove this commented out line:

Suggested change
// 'script_loader_tag',.

$config['headers']['User-Agent'] = Google_Proxy::get_application_name();

/** This filter is documented in wp-includes/class-http.php */
/** This filter is documented in wp-includes/class-http.php.*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

The rule should preserve trailing whitespace for single-line docblocks, i.e. this should be fixed as follows:

Suggested change
/** This filter is documented in wp-includes/class-http.php.*/
/** This filter is documented in wp-includes/class-http.php. */

}

/** This filter is documented in wp-login.php */
/** This filter is documented in wp-login.php.*/

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As above:

Suggested change
/** This filter is documented in wp-login.php.*/
/** This filter is documented in wp-login.php. */

* @package Google\Site_Kit\Sniffs\Commenting
*/

namespace Google\Site_Kit\Sniffs\Commenting;

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

This should use the same namespace as RequireDocTagDescriptionSniff.

Suggested change
namespace Google\Site_Kit\Sniffs\Commenting;
namespace Google\Site_Kit\PHPCS\Sniffs\Commenting;

* @param string $content Token content.
* @return string Updated token content.
*/
private function append_full_stop_preserve_newline( $content ) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I think this method needs attention to preserve the whitespace I flagged in my comments above.

@ankitrox

Copy link
Copy Markdown
Collaborator Author

Thank you @techanvil for reviewing the PR. I have updated the rule to preserve the single whitespace in single line comment.
Over to you for another pass.

@techanvil techanvil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks @ankitrox, please see my comments.

*
* ## EXAMPLES
*
* phpcs:ignore PHPCS.Commenting.DocCommentFullStop.MissingFullStop -- Command examples should not include trailing punctuation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Can you move this phpcs:ignore line above the start of the /** block? It interferes with the doc readability where it is.

Comment thread includes/Core/CLI/Reset_CLI_Command.php Outdated
* ## EXAMPLES
*
* wp google-site-kit reset
* phpcs:ignore PHPCS.Commenting.DocCommentFullStop.MissingFullStop -- Command examples should not include trailing punctuation.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

As above, please move this above the /** start of the block.

@ankitrox

Copy link
Copy Markdown
Collaborator Author

Thanks @techanvil
Moved those phpcs rules on top as suggested.

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.

2 participants