In the tests that have been newly ported to PHPUnit tests by #3055728: Convert from Simpletests to PHPUnit tests, the visibility of methods in the test classes is inconsistent and wrong in many place. The attached patch fixes this.

There are three types of changes in this patch:

  1. setUp() should be protected
  2. test functions (defined as functions with names that start with "test") should be public
  3. utility functions (defined as functions with names that don't start with "test") should be protected
CommentFileSizeAuthor
visibility.patch20.61 KBtr

Comments

TR created an issue. See original summary.

Status: Needs review » Needs work

The last submitted patch, visibility.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

tr’s picture

tr commented
Status: Needs work » Needs review
adamps’s picture

adamps commented

Thanks @TR. Please can you post a link to the page that documents the requirements that you are fixing?

jcnventura’s picture

jcnventura
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

@AdamPS, the fact the tests still pass show that the changes are not harmful. Unfortunately, I don't think there are any requirements to have everything 'protected' except the test* methods. But there should be, and they should be here: https://www.drupal.org/project/coding_standards/issues/2057905

The changes being made here make a lot of sense, as only the test functions are called externally, and all the rest is handled by the testing system, and as such can be made protected.

I'm setting this to RTBC, as I've verified the tests still run, and it solves a lot of missing method visibility problems.

tr’s picture

tr commented

https://www.drupal.org/docs/develop/standards/object-oriented-code#visib...
says that all methods must declare a visibility. This patch does that.

There are currently 86 methods in all the Simplenews test classes. Of those 86, a full half - 43 methods - do not declare any visibility. So we must choose between public, protected, and private for these. This patch fixes those 43 methods plus corrects the visibility of 10 additional methods.

  1. In the case of setUp(), protected is the correct choice because setUp() is a method inherited from BrowserTestBase. See https://api.drupal.org/api/drupal/core%21tests%21Drupal%21Tests%21Browse...

    A choice of anything other than protected amounts to a change to the API of the parent class, and that should only be done deliberately, for a specific purpose, and for a specific need. There is no such purpose or need here, and the current lack of visibility or public visibility is just legacy from poor PHP-based programming practices inherited from Drupal 6.

  2. Test classes are weird objects to begin with, since there is this artificial requirement that the method names conform to certain naming patterns. But these specially-named functions must be public because they are invoked from the testing framework so they must be visible to that framework. public is the only visibility that will work here - any other visibility will fail to execute.
  3. The only debatable point is what visibility to choose for the non-test methods. But 40 years of OO theory and established practice makes this a very easy choice in all but the most fringe cases (which don't apply here ...): public is used for the public API of the class (the functionality the class exposes to the outside world), protected is used for internal implementation of the public API (i.e. helper or utility functions) since we don't want the outside world to use these functions but we do want subclasses to be able to customize this functionality, and private is used only for internal implementation (of either the public API or of the utility functions) when we are *sure* that subclasses won't need to override these private methods. Private should be used rarely and only in very special cases, none of which apply here.

  • AdamPS committed 05baa6b on 8.x-2.x authored by TR 
    Issue #3079894 by TR, jcnventura: Fix visibility of test methods
adamps’s picture

adamps commented
Category: Bug report » Task
Priority: Minor » Normal
Status: Reviewed & tested by the community » Fixed

@TR thanks for a very clear and logical explanation.

Status: Fixed » Closed (fixed)

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