Problem/Motivation

The official coding standard has changed, so we should adjust the sniff. I haven't looked into what's actually necessary yet, just opening this as a placeholder. Drupal core never enabled this particular sniff so is unaffected (although it would help to enable it later if it matches the rules).

Coding standards issue #2140961: Allow constructor methods to omit docblocks
Change Record https://www.drupal.org/node/3365111

Proposed resolution

Modify sniff Drupal.Commenting.FunctionComment.Missing to not flag an error if there is no docblock for a __construct function.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#12 Screenshot 2023-11-10 at 16.34.29.png236.2 KBjonathan1055

Issue fork coder-3400560

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

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Title: Allow constructors to omit docblocks » Allow constructors to omit docblocks in default phpcs rules
Project: Coder » GitLab Templates
Version: 8.3.x-dev »
Component: Coder Sniffer » gitlab-ci

Moving this to gitlab templates in case it's possible via a phpcs configuration change in the default template rather than changing the actual sniff.

fjgarlin’s picture

fjgarlin commented

The file to change would be: https://git.drupalcode.org/project/gitlab_templates/-/blob/1.0.x/scripts...

Core uses: https://git.drupalcode.org/project/drupal/-/blob/11.x/core/phpcs.xml.dis...

I don’t know how far we should get in tweaking the default file, but maybe we can take some of the rules from core.

I don’t know what drupalci did, but it’d be worth checking.

Having said all that, if a module has a phpcs file, it will be respected.

jonathan1055’s picture

jonathan1055 commented

I know that Drupal CI is about to be replaced by Gitlab-CI but the coding standards ruleset is a core file, so this should surely be a Core issue, in the Core queue? I think the linked scripts/phpcs.xml.dist is the file for this project to use on its own files? We should not be modifying it to make changes for other projects.

fjgarlin’s picture

fjgarlin commented

The file is used as a fallback if a contrib module does not have one. See https://git.drupalcode.org/project/gitlab_templates/-/blob/1.0.x/include...

If drupalci uses for contrib the core file (doesn’t it contain core paths in it??) we could mirror the same here.

It’s still unclear to me where this issue appeared. The fixes here would affect all contrib, but not core.

More details and CI runs links would help to narrow down the scope and the possible fix.

jonathan1055’s picture

jonathan1055 commented
Project: GitLab Templates » Coder
Version: » 8.3.x-dev
Component: gitlab-ci » Coder Sniffer

It’s still unclear to me where this issue appeared. The fixes here would affect all contrib, but not core.

Yes indeed. I don't think the issue should have been moved to this queue. It's about relaxing a coder sniff due to the receent Coding Standards change - heres the Change Record (but oddly it is still in draft)

The sniff is Drupal.Commenting.FunctionComment.Missing and I have checked, there is no phpcs config option to ignore the sniff just for constructor methods (I think that was wishful thinking by @catch)

So this issue should go back to the Coder queue, to request a modification to the sniff.

catch’s picture

catch
he/him
Primary language English
commented

the coding standards ruleset is a core file, so this should surely be a Core issue, in the Core queue?

Core's file has this already:

  <rule ref="Drupal.Commenting.ClassComment">
    <exclude name="Drupal.Commenting.ClassComment.Missing"/>
  </rule>

The rule has never been enabled for core (like lots of rules), because it can take months to bring core up to a new coder sniff given the amount of 15 year old code in there. So from the point of view of core, this issue would only block enabling that rule. Contrib uses a different file as @fjgarlin points out.

jonathan1055’s picture

jonathan1055 commented

Yes that's right. I might have a quick look at the sniff to see what is involved. Is it just if the function name is __construct then we can ignore the rule?

catch’s picture

catch
he/him
Primary language English
commented

Is it just if the function name is __construct then we can ignore the rule?

Yes exactly. In the original issue we discussed doing it for all magic methods, but it's fine to hardcode it to __construct().

jonathan1055’s picture

jonathan1055 commented
Assigned: Unassigned » jonathan1055
Issue summary: View changes

What about __destruct ? Is that also covered by the relaxed rule, or is it just __construct ?

I see in the draft change record it says

docblocks for magic methods, such as __construct() can be omitted

catch’s picture

catch
he/him
Primary language English
commented

No it's only constructors, the change record was wrong. Updated and published it..

jonathan1055’s picture

jonathan1055 commented
StatusFileSize
new Screenshot 2023-11-10 at 16.34.29.png236.2 KB

@klausi, sorry I created an issue fork here, forgetting that development and MRs should be against https://github.com/pfrenssen/coder
I can't delete the branch, see attached. Can you or someone else do it?

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes

The PR is https://github.com/pfrenssen/coder/pull/215
First added test coverage to show that current sniff fails.

jonathan1055’s picture

jonathan1055 commented
Assigned: jonathan1055 » Unassigned
Status: Active » Needs review

https://github.com/pfrenssen/coder/pull/215/files is ready for review

jonathan1055’s picture

jonathan1055 commented
Title: Allow constructors to omit docblocks in default phpcs rules » Allow _construct() method to omit docblock
Issue summary: View changes

Added Change Record to the IS.

  • jonathan1055 authored 0f95abc3 on 8.3.x 
    feat(FunctionComment): Docblock is not required for __constuct() methods...
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs review » Fixed

Merged, thanks!

Status: Fixed » Closed (fixed)

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