Follow-up to #2707371: Fix several errors in the 'Drupal.Commenting.DocComment' coding standard

Problem/Motivation

Let's fix drupal/coder's sniffs in core.

We'll work on PSR2.Classes.PropertyDeclaration.ScopeMissing, which requires that we declare scope for class properties and methods. Since this means replacing var where we find it, we also end up fixing PSR2.Classes.PropertyDeclaration.VarUsed.

Proposed resolution

Add drupal/coder and the phpcs package to your Drupal codebase:

# From Drupal root directory
$ composer require drupal/coder
$ ./vendor/bin/phpcs --config-set installed_paths DRUPALPATH/vendor/drupal/coder/coder_sniffer/

Modify the Drupal.Commenting.DocComment errors to not be excluded from within core/phpcs.xml.dist.

Run phpcbf from the core directory:

$ cd core
$ ../vendor/bin/phpcbf

This will fix some errors. Review what it did and fix problematic changes.

Run phpcs from the core directory in order to verify that no errors remain:

$ ../vendor/bin/phpcs -p

To review this issue

Install coder/phpcs as above.

Apply the patch.

Run phpcs from the core directory to verify that no errors remain.

$ cd core
$ ../vendor/bin/phpcs -p

Read the patch to check for errors.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#7 2706753_7.patch27.33 KBMile23
#4 interdiff.txt523 bytesMile23
#4 2706753_4.patch38.42 KBMile23
#2 2706753_2_component.patch38.43 KBMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: Fix 'PSR2.Classes.PropertyDeclaration' coding standard » Fix 'PSR2.Classes.PropertyDeclaration' coding standard for Drupal\Component
Status: Active » Needs review
FileSize
2706753_2_component.patch38.43 KB

Limited scope to Drupal\Component. We can file other issues for the rest of core, or just keep changing scope on this one piecemeal until done.

Just a reminder that PSR2 tells us to not use var, and also to not use underscores at the beginning of class property names.

Status: Needs review » Needs work

The last submitted patch, 2: 2706753_2_component.patch, failed testing.

Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Status: Needs work » Needs review
FileSize
2706753_4.patch38.42 KB
interdiff.txt523 bytes

DiffOp properties needed to be public.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Version: 8.1.x-dev » 8.2.x-dev
Status: Needs review » Needs work

this needs to include sniffer into phpcs.xml.dist

current state for all core

     31 PSR2.Classes.PropertyDeclaration.Underscore
     53 PSR2.Classes.PropertyDeclaration.VarUsed
     67 PSR2.Classes.PropertyDeclaration.ScopeMissing
pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Issue summary: View changes
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented
Title: Fix 'PSR2.Classes.PropertyDeclaration' coding standard for Drupal\Component » Fix 'PSR2.Classes.PropertyDeclaration.ScopeMissing/VarUsed' coding standard
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2706753_7.patch27.33 KB

Updating issue summary to revert back to correct instructions.

Re: #5: That will be a big patch that will take a year to review, so I'm limiting the scope to PSR2.Classes.PropertyDeclaration.ScopeMissing, which also ends up fixing VarUsed (since var is the same as public).

We can rescope later or file a new issue with Underscore, which will require more cognitive resources to patch and review.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner at Chapter Three commented
Status: Needs review » Reviewed & tested by the community 
+++ b/core/lib/Drupal/Core/Database/Driver/sqlite/Connection.php
@@ -41,7 +41,7 @@ class Connection extends DatabaseConnection {
    */
-  var $tableDropped = FALSE;
+  public $tableDropped = FALSE;

That one is certainly internal, but yeah I guess its not this issue to discuss it.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

I agree - this rule is good to implement and all we can do in this issue is make the properties public. Anything else is tricky and requires a lot of thinking.

Committed 7de3512 and pushed to 8.1.x and 8.2.x. Thanks!

  • alexpott committed c26745d on 8.2.x 
    Issue #2706753 by Mile23: Fix 'PSR2.Classes.PropertyDeclaration....
Mile23’s picture

Mile23
Primary language English
Location Seattle, WA
CreditAttribution: Mile23 as a volunteer commented

If the property was declared using var, then it was always public by design.

Status: Fixed » Closed (fixed)

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