Fix class comment doc blocks in tests for 'Drupal.Commenting.DocComment.ShortSingleLine'

Restore drupalci and now setting back to NR.

Tested the same way as before.

Commented out phpcs exclude line and ran the code-commit-check.sh and confirmed the files being addressed in this ticket are not throwing errors.

Change looks good to me then.

Went back and reread them and they look fine

Only one that stood out for maybe a second opinion is but the brief work I've done in Xss.php those sounds right.

 * @see \Drupal\Component\Utility\Xss::filter()
 * @see \Drupal\Component\Utility\UrlHelper::filterBadProtocol
 * @see \Drupal\Component\Utility\UrlHelper::stripDangerousProtocols

Can I ask in advance of reviewing this one that it also be converted to an MR? There will be suggestions. I can tell already by the second hunk. :)

@smustgrave, thanks for reading the changes.

@xjm, all of these siblings have been converted to MRs.

Setting back to Needs review.

Needs followup to look at \Drupal\KernelTests\Core\Common\XssUnitTest as mentioned in this comment

Opened 📌 Look into XssUnitTest Comments Active for the followup

That appeared to be the only open thread I could tell.

Hiding patches for clarity.

Added a couple more suggestions. "Test module" is not a helpful file docblock. One of the proposed docblocks also went against our coding standards by not having complete sentences.

Thanks!

Sorry, that comment was meant for the other issue.

I started from the end of the MR this time, because I confused this issue with another one. I guess there are at least three issues related to this rule.

Fixing attribution.

Needs a followup for this comment, https://git.drupalcode.org/project/drupal/-/merge_requests/3278#note_149676

All followups made

📌 Convert testSkipSecurityFilters() to a unit or kernel test Active
📌 Remove possible duplicate test XssUnitTest::testBadProtocolStripping Active
📌 Convert testJail() to use expectExceptions Needs review

This is ready for reviews.

The Needs Review Queue Bot → tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

Apart from a re-roll or rebase, this issue may need more work to address feedback in the issue or MR comments. To progress an issue, incorporate this feedback as part of the process of updating the issue. This helps other contributors to know what is outstanding.

Consult the Drupal Contributor Guide → to find step-by-step guides for working with issues.

Rebased and tests are running

Took another shot at this. 95 is a lot of changes and covering a wide range of topics. But looking at what's there, they make sense and tell me what the test does.

A few nitpicks but this is looking almost ready to go.

Is it possible to enable this rule for tests only in phpcs.xml.dist to avoid future regressions?

Rishabh Vishwakarma → made their first commit to this issue’s fork.

@Rishabh Vishwakarma, thanks for the interest in this work. Please make your commit message reflect the changes that you have made. The commit message at the bottom of an issue is for a maintainer to use when committing to a project. I also recommend that you use the suggestions feature in GitLab when working on coding standards issues that are using an MR.

Time for a review again.

Don't mind marking this.

These are not my best but reading through the changes they make sense to me.

Re #39 this is not quite possible at present; Drupal.Commenting.DocComment.ShortSingleLine does not distinguish between class comments and method comments, and while this issue solves the class comments in tests, there are still about 130 method comments that are longer than one line.

Re-reviewed the whole patch and I couldn't find anything else to nitpick at. We can catch any stragglers in a followup when we eventually enable the Coder rule.

Committed and pushed to 10.1.x and backported to 10.0.x and 9.5.x to make future test backports easier. Thanks everyone!

It needs quick-fix as CI testing failed for 10.1 branch - https://www.drupal.org/pift-ci-job/2635635 →

moreover it broke testing patches https://www.drupal.org/pift-ci-job/2635665 →

Reverted from all three branches - this is a conflict with the new @covers rule from phpunit-phpstan

Locally it passes after https://git.drupalcode.org/project/drupal/-/merge_requests/3577/diffs?co...

Hope bot will be green

Reviewed and looks good to me as well.

Re-committed to 10.1.x, 10.0.x and 9.5.x. Thanks again!

Automatically closed - issue fixed for 2 weeks with no activity.

quietone → closed merge request !3278