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
CommentFileSizeAuthor
#43 unify_php_visibility-2600836-43.patch6.21 KBchi
#39 unify_php_visibility-2600836-39.patch645.2 KBjofitz
#38 unify_php_visibility.php_.txt764 bytesAnonymous (not verified)
#33 unify_php_visibility-2600836-33.patch646.61 KBminakshiPh
#31 unify_php_visibility-2600836-31.patch647.1 KBminakshiPh
#24 unify_php_visibility_2600836-24.patch661.06 KBbalagan
#21 unify_php_visibility_2600836-21.patch662.54 KBbalagan
#18 unify_php_visibility_2600836-18.patch663.05 KBbalagan
#11 drupal-unify_php_visibility_for_all_test_methods-2600836-11-D8.patch661.28 KBsagar ramgade
#9 drupal-Unify_PHP_visibility_for_all_test_methods-2600836-9-D8.patch661.28 KBsagar ramgade
#8 methods_without_scope.txt128.97 KBchi
#7 interdiff-2600836-5-7.txt537 bytesanil280988
#7 2600836-7.patch4.68 KBanil280988
#5 2600836-5.patch4.41 KBjoshi.rohit100

Comments

Chi created an issue. See original summary.

dawehner’s picture

dawehner
Primary language German
commented
Version: 8.1.x-dev » 8.0.x-dev

Given that test only changes are 'rc eligible' you could totally do that in 8.0.x I think

chi’s picture

chi commented
Issue tags: +Novice
joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented
Assigned: Unassigned » joshi.rohit100
joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
commented
Assigned: joshi.rohit100 » Unassigned
Status: Active » Needs review
StatusFileSize
new 2600836-5.patch4.41 KB
chi’s picture

chi commented
Status: Needs review » Needs work

What about methods without specified visibility scope? I would fix them as well.

anil280988’s picture

anil280988 commented
Status: Needs work » Needs review
StatusFileSize
new 2600836-7.patch4.68 KB
new interdiff-2600836-5-7.txt537 bytes

Added Public Visibility to methods without specified visibility scope.

chi’s picture

chi commented
Title: Unify PHP visibility for all test methods » Unify PHP visibility for all test methods
Status: Needs review » Needs work
StatusFileSize
new methods_without_scope.txt128.97 KB

There is much more work here. I would suggest creating some shell script to automate the process.

sagar ramgade’s picture

sagar ramgade commented
Status: Needs work » Needs review
StatusFileSize
new drupal-Unify_PHP_visibility_for_all_test_methods-2600836-9-D8.patch661.28 KB

Used phpStorm replace feature to replace most of them. Seems patch has covered all hopefully.

Status: Needs review » Needs work

The last submitted patch, 9: drupal-Unify_PHP_visibility_for_all_test_methods-2600836-9-D8.patch, failed testing.

sagar ramgade’s picture

sagar ramgade commented
StatusFileSize
new drupal-unify_php_visibility_for_all_test_methods-2600836-11-D8.patch661.28 KB

Patch re-rolled.

anil280988’s picture

anil280988 commented
Status: Needs work » Needs review

The last submitted patch, 9: drupal-Unify_PHP_visibility_for_all_test_methods-2600836-9-D8.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: drupal-unify_php_visibility_for_all_test_methods-2600836-11-D8.patch, failed testing.

dawehner’s picture

dawehner
Primary language German
commented
Issue tags: +rc eligible

It is rc eligible, as its tests only

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

chi’s picture

chi commented
Version: 8.1.x-dev » 8.2.x-dev
balagan’s picture

balagan commented
StatusFileSize
new unify_php_visibility_2600836-18.patch663.05 KB

I also used the replace feature of phpStorm to change the visibility.

balagan’s picture

balagan commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 18: unify_php_visibility_2600836-18.patch, failed testing.

balagan’s picture

balagan commented
StatusFileSize
new unify_php_visibility_2600836-21.patch662.54 KB

Fixed the error in file ArgumentsResolverTest.php

balagan’s picture

balagan commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: unify_php_visibility_2600836-21.patch, failed testing.

balagan’s picture

balagan commented
StatusFileSize
new unify_php_visibility_2600836-24.patch661.06 KB

Fixed EntityResolverManagerTest.php and FormTestBase.php files.

balagan’s picture

balagan commented
Status: Needs work » Needs review

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Needs review » Needs work

The last submitted patch, 24: unify_php_visibility_2600836-24.patch, failed testing.

quietone’s picture

quietone commented
Issue tags: +Needs reroll

Patch no longer applies cleanly.

minakshiPh’s picture

minakshiPh commented
Assigned: Unassigned » minakshiPh
dawehner’s picture

dawehner
Primary language German
commented

Can we add a coder rule for that, so we can automate that fix?

minakshiPh’s picture

minakshiPh commented
Assigned: minakshiPh » Unassigned
Status: Needs work » Needs review
StatusFileSize
new unify_php_visibility-2600836-31.patch647.1 KB

Re-rolled the patch.

Kindly review.
Thanks!

Status: Needs review » Needs work

The last submitted patch, 31: unify_php_visibility-2600836-31.patch, failed testing.

minakshiPh’s picture

minakshiPh commented
Status: Needs work » Needs review
StatusFileSize
new unify_php_visibility-2600836-33.patch646.61 KB

Re-rolled the patch.

Kindly review.
Thanks!

manuel garcia’s picture

manuel garcia commented
Issue tags: -Needs reroll

Status: Needs review » Needs work

The last submitted patch, 33: unify_php_visibility-2600836-33.patch, failed testing.

manuel garcia’s picture

manuel garcia commented
Issue tags: +Needs reroll
dawehner’s picture

dawehner
Primary language German
commented

We should have a small little script which rewrites our code, this also frees up from heavy reviews.

Anonymous’s picture

Anonymous (not verified) commented
StatusFileSize
new unify_php_visibility.php_.txt764 bytes

I 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:

-  function storeViewPreview($output) {
+  public function storeViewPreview($output) {

-  function isDirectory($path) {
+  public function isDirectory($path) {

-  function _testColor($theme, $test_values) {
+  public function _testColor($theme, $test_values) {

Any hints?

jofitz’s picture

jofitz
he/him
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new unify_php_visibility-2600836-39.patch645.2 KB

Reroll

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Related issues: +#2854529: Fix Drupal.Scope.MethodScope - all methods should have scopes

Closing 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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Needs review » Needs work

@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.

chi’s picture

chi commented
Status: Needs work » Needs review
StatusFileSize
new unify_php_visibility-2600836-43.patch6.21 KB

I have so far found only 10 protected method. So the patch is pretty simple.

But we should also add a rule to coder to check for protected test* methods otherwise we can't guarantee we won't regress.

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.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented

@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.

chi’s picture

chi commented
Status: Needs review » Postponed

Ok, let's postpone this then.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Postponed » Reviewed & tested by the community

PHPUnit fails anyway if a test method is declared as protected. Tried an example:

1) Warning
Test method "testGetDefinitions" in test class "Drupal\Tests\Component\Annotation\Plugin\Discovery\AnnotationBridgeDecoratorTest" is not public.

FAILURES!
Tests: 1, Assertions: 0, Failures: 1.

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.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Title: Unify PHP visibility for all test methods » Make protected Simpletest test methods public for consistency
xjm’s picture

xjm
she/her
Primary language English
commented
Version: 8.4.x-dev » 8.3.x-dev
Status: Reviewed & tested by the community » Fixed

#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.

dawehner’s picture

dawehner
Primary language German
commented

@xjm
Did you forgot to push?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

@dawehner: I see the commit in both branches, so this should be fine. http://cgit.drupalcode.org/drupal/commit/?id=47ec7f7d4f0146e1de578457028...

Status: Fixed » Closed (fixed)

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