Fix Drupal.Scope.MethodScope - all methods should have scopes

@alexpott that issue is not only about missing method scopes but also about wrong scopes (test methods should not be protected).

Nice! Thank you alex. My rudimentary ag/grep skills give a +1 here.

+1 testbots saw the new rule and ran it and phpcs didnt find any issues:

00:00:49.091 ----------------   Starting phpcs   ----------------
00:00:49.091 Directory created at /var/lib/drupalci/workspace/jenkins-drupal_patches-3016/ancillary/phpcs
00:00:49.092 PHPCS sniffing the project.
00:00:49.092 Checking for phpcs.xml(.dist) file.
00:00:49.092 Checking for PHPCS config file: /var/www/html/core/phpcs.xml*
00:00:49.093 test -e /var/www/html/core/phpcs.xml*
00:00:49.269 Using existing PHPCS config file.
00:00:49.270 Checking for phpcs tool in codebase.
00:00:49.270 test -e /var/www/html/vendor/squizlabs/php_codesniffer/scripts/phpcs
00:00:49.477 PHPCS config file modified, sniffing entire project.
00:00:49.478 Executing PHPCS.
00:00:49.478 cd /var/www/html/core && /var/www/html/vendor/squizlabs/php_codesniffer/scripts/phpcs --warning-severity=2 --report-full=/var/lib/drupalci/workdir/phpcs/codesniffer_results.txt --report-checkstyle=/var/lib/drupalci/workdir/phpcs/checkstyle.xml --report-diff=/var/lib/drupalci/workdir/phpcs/codesniffer_fixes.patch /var/www/html/core
00:03:40.342 cd /var/www/html/core && /var/www/html/vendor/squizlabs/php_codesniffer/scripts/phpcs -e  --warning-severity=2 --report-full=/var/lib/drupalci/workdir/phpcs/codesniffer_results.txt --report-checkstyle=/var/lib/drupalci/workdir/phpcs/checkstyle.xml --report-diff=/var/lib/drupalci/workdir/phpcs/codesniffer_fixes.patch /var/www/html/core > /var/lib/drupalci/workdir/phpcs/phpcs_sniffs.txt
00:03:40.629 Directory created at /var/lib/drupalci/workspace/jenkins-drupal_patches-3016/artifacts/phpcs
00:03:40.630 Adjusting paths in report file: /var/lib/drupalci/workspace/jenkins-drupal_patches-3016/ancillary/phpcs/checkstyle.xml
00:03:40.635 ---------------- Finished phpcs in 171.55637288094 ---------------- 

All methods should have a scope.

Formally it is not a part of Drupal coding standard because Squiz.Scope.MethodScope is not listed in Drupal ruleset.xml.

Here it is: #2854646: Add a rule to verifiy that class methods have scope modifiers.

We could check a scope for class properties in this issue as well since it is very related subject. Currently there is no errors of this type (thanks to #2706753: Fix 'PSR2.Classes.PropertyDeclaration.ScopeMissing/VarUsed' coding standard) but adding a rule for this (Squiz.Scope.MemberVarScope.Missing) would help us to prevent regression.

+++ b/core/phpcs.xml.dist
@@ -108,6 +108,7 @@
   <!-- Squiz sniffs -->
+  <rule ref="Squiz.Scope.MethodScope.Missing"/>
   <rule ref="Squiz.Strings.ConcatenationSpacing">

This change is not correct, we should use Drupal.Scope.MethodScope which has a built in fixer.

Just looked through our coding standards and realized that we don't define the order of method modifiers anywhere. Opened #2855980: Define order of object method modifiers as in PSR-12.

I can't verify this patch with our phpcbf version because we would need #2857714: Upgrade Coder to 8.2.11.

Will try with my global phpcbf with restricted sniffs and see if that matches.

Yep, let's make phpcs clean first with #2857822: Fix coding standards issues introduced mostly by array syntax conversion, the other phpcs fails are confusing.

I don't want to review your patch by hand - I want to confirm with phpcbf that the patch is complete and my fixers in Coder are as good as advertised :)

The trailing commas are fixed now, so this can proceed.

The title of this issue is fun to read aloud.

Im not sure how well advertised some of these new phpcs features are in drupalci, but you can see which sniffs actually ran, as well as the standard output from phpcs if you go into the artifacts dir, and go into the phpcs dir on the dispatcher:

https://dispatcher.drupalci.org/job/drupal_patches/4452/artifact/jenkins...

Im not sure how well advertised some of these new phpcs features are in drupalci, but you can see which sniffs actually ran, as well as the standard output from phpcs if you go into the artifacts dir, and go into the phpcs dir on the dispatcher:

!!! Holy smokes!

omg it even makes a patch!

Okay the way to navigate to it is Click on the test results bubble -> View results on dispatcher -> Click on Build artifacts -> Click on artifacts (end of the path) -> Click on phpcs -> results and a fixer patch are there.

So https://dispatcher.drupalci.org/job/drupal_patches/4529/artifact/jenkins... this one looks clean.

The other issue to fix the protected test methods was committed on its own, so resetting issue credit back to the contributors for this issue.

Oh look, they're also reported on the d.o integration:

I think so, yah. We just need someone to confirm the 8.3.x backport of the 8.2.11 patch because we don't want to have the branches in different states. It also sounds like klausi wants to do a review with 8.2.11 committed first to make sure the fixer is working as intended.

Once that's all in place and the patch is current, then we'll freeze commits briefly until it's committed. I don't think I've committed anything that would conflict with or violate the standard but we can double-check since it's like, you know, a megabyte patch. :)

#2857714: Upgrade Coder to 8.2.11 is in 8.3.x now as well, so the branches correctly share the same version again.

Patches looks good to me, verified with ../vendor/bin/phpcs that the output is empty.

I wanted to also test this with the Drupal.Methods.MethodDeclaration sniff, but that one would also change additional method definitions, so the patches were not comparable. See #2855980: Define order of object method modifiers as in PSR-12 where we push for that.

@vaplas: no, because ArchiveTar is third party code copied into Drupal and has a @codingStandardsIgnoreFile directive on top. It is exempt from our coding standards and should not be modified.

Committed/pushed to 8.4.x and 8.3.x, thanks!

Fixing issue credit to remove credit for myself; I did not contribute anything actually relevant to this issue. :P