Problem/Motivation

This sniff relates to Drupal API documentation standards for classes and namespaces.

Hello this fixes the Drupal.Commenting.ClassComment.Missing standard listed in this parent, #2571965: [meta] Fix PHP coding standards in core, stage 1.

In the pull request two classes are skipped:
- one is being fixed via the https://www.drupal.org/project/drupal/issues/3105880
- and another one intentionally doesn't include the class doc block due to testing the empty plugin annotations (therefore the adjusted phpcs.xml.dist).

Steps to reproduce

Proposed resolution

Comply with Drupal API documentation standards for classes and namespaces and another relevant part of the Drupal coding standards.

Remaining tasks

This issue will enable the sniff and fix any remaining issues. The remaining issues will, at least, include core/lib.

https://git.drupalcode.org/project/drupal/-/merge_requests/1889 is on 11.x

Done

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 drupal-cs-class-doc-3105950-3-D8.patch112.68 KBpeterkokot
#2 phpcs.patch112.68 KBpeterkokot

Issue fork drupal-3105950

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

peterkokot created an issue. See original summary.

peterkokot’s picture

peterkokot commented
StatusFileSize
new phpcs.patch112.68 KB
peterkokot’s picture

peterkokot commented
Status: Active » Needs review
StatusFileSize
new drupal-cs-class-doc-3105950-3-D8.patch112.68 KB
peterkokot’s picture

peterkokot commented
Issue tags: +ContributionWeekend2020

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Spokje made their first commit to this issue’s fork.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Needs review » Needs work

Spokje opened merge request !517

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review
quietone’s picture

quietone commented

I have done a very brief review, only looking at migrate files and then RDF and Rest. I am struck by the fact that a well formed name doesn't really need a doc bloc, but it is what it is. From my brief look I wonder about when to use English words instead of the class name. For example, this * Interface for the migration executable. changes MigrateExecutable to migration executable but this * ResourceTestBase for RestResourceConfig entity. makes no such change.

What is the approach to this?

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Needs review » Needs work

@quietone in #11.

What is the approach to this?

My approach was too random indeed, trying to use full English words this time.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Active

Ok, I tried to use as much decent English as possible.

Beware: I'm not a native speaker, so I might have missed/messed up a few/lot.
I learned it from a book...

spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review
quietone’s picture

quietone commented
Issue summary: View changes

Update IS.

quietone’s picture

quietone commented
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @quietone!

- Resolved all but one thread.
- Merged latest 9.3.x into MR.
- Found and fixed some new offenders.

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned

#WhyDoIAlwaysForgetToDoThisInThePreviousPost

spokje’s picture

spokje
Primary language Dutch
commented
Assigned: Unassigned » spokje
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented
Assigned: spokje » Unassigned
Status: Needs work » Needs review

Not quite sure what happened here, but all is well now.

quietone’s picture

quietone commented
Status: Needs review » Needs work

@Spokje, thanks.

I think that is all I can do here. I do think this needs another set of eyes before RTBC.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks (Yet Again) @quietone.

Back to Needs Review for

I do think this needs another set of eyes before RTBC.

daffie’s picture

daffie commented
Status: Needs review » Needs work
spokje’s picture

spokje
Primary language Dutch
commented

Resolved 2 old threads and added a comment on the new one by @daffie (thanks for the review)

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

- Rebased MR on 9.4.x
- Merged latest commits on 9.4.x
- Fixed new offenders
- Resolved last open thread (thanks @daffie)

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Nice work - adding all these is quite tedious but it's good to finally get this one done.

Reviewed the full set of changes, added some suggestions where there are typos or I think we could improve wording, left a few other comments on places that didn't make sense to me when I read the sentence on its own.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Needs work » Needs review

Thanks @longwave for Yet Another Review!

Resolved (hopefully successful) all threads.

spokje’s picture

spokje
Primary language Dutch
commented
Version: 9.3.x-dev » 9.4.x-dev

MR is now against 9.4.x-dev

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Thanks! I've re-read the whole MR and have no further suggestions. I'm sure there's more nitpicking that can be done (and our existing class comments also probably need work in places), but quietone, daffie and I have all had a go at reviewing this so I think it's had plenty of eyes on it now and is ready for commit.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

We're going to need a version of this that applies to 10.0.x - sorry. Plus it needs to be rerolled against 9.4.x

ankithashetty made their first commit to this issue’s fork.

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented

Rebased the MR against 9.4.x branch, thanks!

ankithashetty opened merge request !1885

ankithashetty closed merge request !1885

ankithashetty opened merge request !1889

ankithashetty’s picture

ankithashetty
Location Karnataka, India
commented
Status: Needs work » Needs review

Here is an MR for 10.0.x branch.

Changes made to

  • core/modules/hal/src/LinkManager/LinkManager.php
  • core/modules/hal/src/LinkManager/RelationLinkManager.php
  • core/modules/hal/src/LinkManager/RelationLinkManagerInterface.php
  • core/modules/hal/src/LinkManager/TypeLinkManager.php
  • core/modules/hal/src/LinkManager/TypeLinkManagerInterface.php
  • core/modules/hal/tests/src/Functional/aggregator/FeedHalJsonTestBase.php
  • core/modules/hal/tests/src/Functional/aggregator/ItemHalJsonTestBase.php
  • core/modules/hal/tests/src/Functional/comment/CommentHalJsonTestBase.php
  • core/modules/system/tests/modules/render_deprecation/src/RenderDeprecationController.php
  • core/tests/Drupal/Tests/Core/DependencyInjection/Compiler/MimeTypePassTest.php
  • core/tests/Drupal/Tests/Core/Session/SessionManagerTest.php

files in 9.4.x MR is ignored in this new MR as they no longer exist in 10.0.x branch.

Thanks!

quietone’s picture

quietone commented
Parent issue: » #2571965: [meta] Fix PHP coding standards in core, stage 1

Add parent issue

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

MR needs to be updated for 10.1.x now.

If someone could chim in what else is needed for it to make it this round.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

quietone commented
Issue summary: View changes
quietone’s picture

quietone commented
Issue summary: View changes
Status: Needs work » Active
quietone’s picture

quietone commented
Issue summary: View changes
quietone’s picture

quietone commented
Issue summary: View changes
quietone’s picture

quietone commented
Title: Fix 'Drupal.Commenting.ClassComment.Missing' coding standard » [meta] Fix 'Drupal.Commenting.ClassComment.Missing' coding standard
quietone’s picture

quietone commented
Issue summary: View changes

Rebased MR1889 onto 11.x and updated phpcs.xml.dist. This will need another rebase when the child issues are fixed.

quietone’s picture

quietone commented
Issue summary: View changes
quietone’s picture

quietone commented
Issue summary: View changes
quietone’s picture

quietone commented
Status: Active » Needs review

Time for review.

borisson_’s picture

borisson_
Primary language Dutch
Location Mechelen, 🇧🇪
commented
Status: Needs review » Reviewed & tested by the community

These changes look good. It also has the change to phpcs.xml.dist to prevent regressions.

  • nod_ committed 96df9ddb on 11.x 
    Issue #3105950 by spokje, quietone, peterkokot, ankithashetty, longwave...

nod_ closed merge request !1889

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Reviewed & tested by the community » Fixed

Committed 96df9dd and pushed to 11.x. Thanks!

Status: Fixed » Closed (fixed)

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