Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
All methods should have a scope. This is part of the existing coding standards - see https://www.drupal.org/docs/develop/coding-standards/object-oriented-cod...
Proposed resolution
Add the Squiz.Scope.MethodScope.Missing rule
Make all methods missing scope public as this is the default and therefore no API change.
I ran the following replaces:
^ static function
with public static function
^ function
with public function
^ abstract function
with abstract public function
Scripted version:
find ./core -name "*.php*" -exec sed -i 's/^ function/ public function/g' {} \;
find ./core -name "*.php*" -exec sed -i 's/^ static function/ public static function/g' {} \;
find ./core -name "*.php*" -exec sed -i 's/^ abstract function/ abstract public function/g' {} \;
curl https://www.drupal.org/files/issues/fix-post-script.patch | git apply
Remaining tasks
User interface changes
None
API changes
None
Data model changes
None
Comment | File | Size | Author |
---|---|---|---|
#54 | 2854529-8_4-43_patch_test_with_PHP_5_6___MySQL_5_5___Drupal_org.png | 111.96 KB | xjm |
#43 | 2854529-8.4-43.patch | 918.2 KB | alexpott |
#43 | 2854529-8.3-43.patch | 917.42 KB | alexpott |
#43 | fix-post-script.patch | 11.07 KB | alexpott |
#36 | 2854529-2-36.patch | 922.47 KB | alexpott |
Comments
Comment #2
alexpottComment #4
alexpottUghs I dislike the faking of procedural code in unit tests. It's just not a unit test anymore.
Comment #5
alexpottComment #6
Chi CreditAttribution: Chi commentedComment #7
alexpott@Chi thanks for relating that issue - let's proceed here because this is coming with a coding standard and automated checking - I will created everyone on that issue here and mark it as a duplicate.
Comment #15
Anonymous (not verified) CreditAttribution: Anonymous commentedalexpott credited vaplas.
Comment #16
alexpottCrediting everyone from #2600836: Make protected Simpletest test methods public for consistency
Comment #17
Chi CreditAttribution: Chi commented@alexpott that issue is not only about missing method scopes but also about wrong scopes (test methods should not be protected).
Comment #18
alexpott@Chi well let's just deal with the protected ones there then. See new comment on that issue.
Comment #20
alexpottFixing the final fails.
Comment #21
dawehnerNice! Thank you alex. My rudimentary ag/grep skills give a +1 here.
Comment #22
alexpottI'll backport this to 8.3.x once it lands in 8.4.x - it doesn't apply.
Comment #23
alexpottComment #24
Mixologic+1 testbots saw the new rule and ran it and phpcs didnt find any issues:
Comment #25
alexpottDiscussed with @xjm. We're going to schedule this for the RC phase to minimise disruption. We can roll 8.3.x and 8.4.x patches then.
Comment #26
alexpottAnd therefore postponed.
Comment #27
Chi CreditAttribution: Chi commentedFormally it is not a part of Drupal coding standard because Squiz.Scope.MethodScope is not listed in Drupal ruleset.xml.
Comment #28
alexpott@Chi interesting - can you file an issue to add it?
Comment #29
alexpottComment #30
Chi CreditAttribution: Chi commentedHere it is: #2854646: Add a rule to verifiy that class methods have scope modifiers.
Comment #31
alexpottComment #32
Chi CreditAttribution: Chi commentedWe 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.
Comment #33
alexpottComment #34
klausiThis change is not correct, we should use Drupal.Scope.MethodScope which has a built in fixer.
Comment #35
klausiJust 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.
Comment #36
alexpottHere's a new patch for 8.4.x - with a test only so people can see the fails on testbot. Fixes #34. No interdiff cuase reroll.
Comment #37
alexpottComment #38
alexpottAdding issue credit to @klausi and @Mixologic for their reviews.
Comment #40
alexpottComment #41
alexpottHmmm ok this will eventually go into the rc in 8.3.x but we need to test against and do 8.4.x first... need to remember to set the version on patch upload.
Comment #42
alexpottThe test-only patch failed with loads of messages - as expected... just need the patch to run against 8.4.x.
Comment #43
alexpottHere's all the patches for 8.3 and 8.4 post the array change.
Comment #44
klausiI 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.
Comment #45
alexpott@klausi but the verification of the patch doesn't need phpcbf - as long as the patch doesn't fail the rules when applied and the fails the rules when the new rule is added then is that not verified? It is not exactly unheard of fixers to even introduce coding standards regressions. For example (not a phpcbf fixer as far as I know but still) - #2857822: Fix coding standards issues introduced mostly by array syntax conversion
Comment #46
klausiYep, 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 :)
Comment #47
xjmThe trailing commas are fixed now, so this can proceed.
Comment #48
xjmThe title of this issue is fun to read aloud.
Comment #49
MixologicIm 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...
Comment #50
xjm!!! Holy smokes!
omg it even makes a patch!
Comment #51
xjmOkay 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.
Comment #52
Anonymous (not verified) CreditAttribution: Anonymous commentedI was guided by the instruction of #51.
COOL!
Comment #53
xjmThe other issue to fix the
protected
test methods was committed on its own, so resetting issue credit back to the contributors for this issue.Comment #54
xjmOh look, they're also reported on the d.o integration:
Comment #55
Anonymous (not verified) CreditAttribution: Anonymous commented#53 fair!
#54 fine! But with css
cursor: pointer;
it will be absolutely fine!)So.. we waiting #2857714: Upgrade Coder to 8.2.11 before RTBC, right (#44)?
Comment #56
xjmI 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. :)
Comment #57
xjm#2857714: Upgrade Coder to 8.2.11 is in 8.3.x now as well, so the branches correctly share the same version again.
Comment #58
alexpottBoth the patches in #43 still apply and still fix all the method visibility errors in core. I've sent them both for a re-test.
Comment #59
klausiPatches 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.
Comment #60
Anonymous (not verified) CreditAttribution: Anonymous commentedI also checked 8.4.x and 8.3.x via PhpStorm Inspect Code with rule
"Access modifiers shall be defined"
.Before patch: 1943 problems 8.4.x (and 1942 problems with 8.3.x).
After patch: 1 problem (/core/lib/Drupal/Core/Archiver/ArchiveTar.php::loadExtension, 279 line)
Should we care about ArchiveTar?
Comment #61
klausi@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.
Comment #62
Anonymous (not verified) CreditAttribution: Anonymous commented@klausi thank you for provide with this moment!
#58 re-test still clean!
Comment #63
catchCommitted/pushed to 8.4.x and 8.3.x, thanks!
Comment #65
xjmFixing issue credit to remove credit for myself; I did not contribute anything actually relevant to this issue. :P