Problem/Motivation

For the conversion to PHPUnit as part of #2807237: PHPUnit initiative the $modules property must be protected since it is sometimes used as protected on old Simpletests. Otherwise PHPUnit will throw fatal errors and the converted test case with the protected $modules property cannot be executed.

Proposed resolution

Make $modules protected on BrowserTestBase and KernelTestBase.

Use something like

find . -path './.git' -prune -o -not \( -name '*.js' \) -type f -path '*/tests/*' -exec sed -i -e 's/public static $modules/protected static $modules/g' {} \;

Remaining tasks

Patch.

CommentFileSizeAuthor
#8 2814035-8.patch912 bytesdawehner
#7 simpletest-modules-protected-2814035-7.patch328.17 KBklausi
#2 simpletest-modules-public-2814035-2.patch7.51 KBklausi
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

klausi created an issue. See original summary.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Issue summary: View changes
Status: Active » Needs review
Issue tags: +find and replace
FileSize
simpletest-modules-public-2814035-2.patch7.51 KB

Patch.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Do you mind explaining why this actually has to be the case? Which external code is this using as public by default?

When BTB would declare the property as protected there shouldn't be an issue.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented

We cannot change BrowserTestBase because we have released the property there as "public static $modules" in Drupal 8.2.0. If we change it to protected then all contrib tests will break that have defined it as public on their test classes.

Yes, the correct way for this property would be protected, but it also does not matter really much. So I think we should rather not break compatibility.

The vast majority of core uses the property as public (on WTB, KTB, BTB), there are only these few instances we need to change to make it consistent.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Well, marking something more open than before is not an issue, see the following two code examples:

protected extends public: https://3v4l.org/NFKjN
public extends protected: https://3v4l.org/m7DpI

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Title: $modules property must be public on all test classes » Make $modules property protected on BrowserTestBase and KernelTestBase
Issue summary: View changes
Status: Needs review » Needs work

Ah, good to know!

Then let's change the property to protected on BrowserTestBase and KernelTestBase. We should also update all usages in test classes using those if that is easily possible.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
simpletest-modules-protected-2814035-7.patch328.17 KB

And here is the auto-generated patch with

find . -path './.git' -prune -o -not \( -name '*.js' \) -type f -path '*/tests/*' -exec sed -i -e 's/public static $modules/protected static $modules/g' {} \;
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented
FileSize
2814035-8.patch912 bytes

Honestly the minimal fix would have been to make the property on BrowserTestBase protected and do all the other conversions in its own issue.

This is done in that patch.
Converting all the instances doesn't even fix the problem of the issue aka. causing issues for automated conversions :)

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented
Status: Needs review » Reviewed & tested by the community

Sure, I can agree to that :-D

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Committed b33af7a and pushed to 8.3.x. Thanks!

  • catch committed b33af7a on 8.3.x 
    Issue #2814035 by klausi, dawehner: Make $modules property protected on...
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Let's still try to change all existing ones?

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi at jobiqo - job board technology commented

Yes, please open an issue for that because it is confusing.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer and at Chapter Three commented

Oh yeah, absolutely! #2822382: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase

Status: Fixed » Closed (fixed)

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

idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Related issues: +#2814035: Make $modules property protected on BrowserTestBase and KernelTestBase
idebr’s picture

idebr CreditAttribution: idebr at ezCompany commented
Related issues: -#2814035: Make $modules property protected on BrowserTestBase and KernelTestBase +#2822382: Make every $modules property protected on classes extending BrowserTestBase and KernelTestBase