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.At the moment most of the test methods are public.
#!/usr/bin/env bash
echo -n 'Public methods: '
grep -r '^ *public function test[A-Z]' . | wc -l
echo -n 'Public methods (PHP4 style): '
grep -r '^ *function test[A-Z]' . | wc -l
echo -n 'Protected methods: '
grep -r '^ *protected function test[A-Z]' . | wc -l
Public methods: 4117
Public methods (PHP4 style): 1379
Protected methods: 9
| Comment | File | Size | Author |
|---|---|---|---|
| #43 | unify_php_visibility-2600836-43.patch | 6.21 KB | Chi |
Comments
Comment #2
dawehnerGiven that test only changes are 'rc eligible' you could totally do that in 8.0.x I think
Comment #3
Chi CreditAttribution: Chi commentedComment #4
joshi.rohit100Comment #5
joshi.rohit100Comment #6
Chi CreditAttribution: Chi commentedWhat about methods without specified visibility scope? I would fix them as well.
Comment #7
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedAdded Public Visibility to methods without specified visibility scope.
Comment #8
Chi CreditAttribution: Chi commentedThere is much more work here. I would suggest creating some shell script to automate the process.
Comment #9
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedUsed phpStorm replace feature to replace most of them. Seems patch has covered all hopefully.
Comment #11
Sagar Ramgade CreditAttribution: Sagar Ramgade as a volunteer commentedPatch re-rolled.
Comment #12
anil280988 CreditAttribution: anil280988 at Publicis Sapient for Publicis Sapient commentedComment #15
dawehnerIt is rc eligible, as its tests only
Comment #17
Chi CreditAttribution: Chi commentedComment #18
balagan CreditAttribution: balagan commentedI also used the replace feature of phpStorm to change the visibility.
Comment #19
balagan CreditAttribution: balagan commentedComment #21
balagan CreditAttribution: balagan commentedFixed the error in file ArgumentsResolverTest.php
Comment #22
balagan CreditAttribution: balagan commentedComment #24
balagan CreditAttribution: balagan commentedFixed EntityResolverManagerTest.php and FormTestBase.php files.
Comment #25
balagan CreditAttribution: balagan commentedComment #28
quietone CreditAttribution: quietone as a volunteer commentedPatch no longer applies cleanly.
Comment #29
minakshiPh CreditAttribution: minakshiPh at Iksula commentedComment #30
dawehnerCan we add a coder rule for that, so we can automate that fix?
Comment #31
minakshiPh CreditAttribution: minakshiPh at Iksula commentedRe-rolled the patch.
Kindly review.
Thanks!
Comment #33
minakshiPh CreditAttribution: minakshiPh at Iksula commentedRe-rolled the patch.
Kindly review.
Thanks!
Comment #34
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #36
Manuel Garcia CreditAttribution: Manuel Garcia as a volunteer and at Appnovation commentedComment #37
dawehnerWe should have a small little script which rewrites our code, this also frees up from heavy reviews.
Comment #38
Anonymous (not verified) CreditAttribution: Anonymous commentedI made a script. But it rewrites only functions starting with 'test[A-Z]' in files '.*Test.php'. I don't know, we need or not need replace all function in all files? Example in #33 patch we have changes like:
Any hints?
Comment #39
jofitz CreditAttribution: jofitz at ComputerMinds commentedReroll
Comment #41
alexpottClosing this in favour of a coding standards based fix #2854529: Fix Drupal.Scope.MethodScope - all methods should have scopes
Everyone who's contributed on this issue has been credited there.
Comment #42
alexpott@Chi points out that this is also dealing with protected methods. This issue I guess should become about that. But we should also add a rule to coder to check for protected test* methods otherwise we can't guarantee we won't regress.
Comment #43
Chi CreditAttribution: Chi commentedI have so far found only 10 protected method. So the patch is pretty simple.
I could not find a finished rule for this. Perhaps this would require developing a custom sniff. Here is the issue I created for Coder project: #2854588: Add a rule to disallow protected test methods. I propose we add the rule to prevent regression in a followup when/if it is ready.
Comment #44
alexpott@Chi generally we do things the other way around - get the rule implemented in coder and then turn it on in core in an issue such as this one. Yes it takes more time but we've had many many coding standards cleanups that have regressed over time so experience tells us this is the only way to make last change.
Comment #45
Chi CreditAttribution: Chi commentedOk, let's postpone this then.
Comment #46
klausiPHPUnit fails anyway if a test method is declared as protected. Tried an example:
I think we don't care enough about Simpletest because we will deprecate it. I think our resources are better spent elsewhere than to write coding standard rules for a deprecated system.
So fixing the 10 Simpletest protected instances manually sounds perfectly fine to me.
Comment #47
klausiComment #48
xjm#46 makes sense to me, so committing based on that. SimpleTest is still a ways from being deprecated, but it is essentially EOL, so I can see not adding specific rules to maintain for it for D8. That said, it probably does also apply to D7, so it is still worth considering.
Thanks @minakshiPh for trying to help out here. :) Note that this issue will not grant you issue credit because your rerolls did not apply and so weren't. To get credit in the future, a good strategy is to double-check that the patch applies before you upload it. If you run into issues when creating patches, core mentoring can probably help also. Good luck with your future contributions!
Going to commit this, although I might revert if @alexpott still has concerns based on #44 / #46. Thanks! Committed to 8.4.x and backported to 8.3.x as an RC-eligible coding standards fix.
Comment #49
dawehner@xjm
Did you forgot to push?
Comment #50
klausi@dawehner: I see the commit in both branches, so this should be fine. http://cgit.drupalcode.org/drupal/commit/?id=47ec7f7d4f0146e1de578457028...