Problem/Motivation

Followup from #3164721: More uses of public static $modules. We have existing patches in the queue that might still use public static $modules, but the property should now be protected instead. We can add a check to do this, as per @mondrake:

We might prevent regressions by adding a check of the visibility of the property in DrupalListener::startTest, and throwing in exception in case it’s public.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#7 3165588-7.patch707 byteslongwave
#7 3165588-7-test-only.patch3.31 KBlongwave
#2 3165588.patch998 byteslongwave
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

longwave created an issue. See original summary.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Active » Needs review
FileSize
3165588.patch998 bytes

First attempt at this. I tried to write a test but any exception must be thrown by the test method itself, exceptions thrown directly in the listener aren't caught. I also tried using warnings instead of exceptions but that didn't seem to work either. Maybe this is enough on its own?

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn at MTech, LLC commented
Status: Needs review » Reviewed & tested by the community

This looks good.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Added this issue to the CR https://www.drupal.org/node/2833984

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Reviewed & tested by the community » Needs work

Maybe let's just have a test-only patch with a test class failing to have a protected $modules property, to demonstrate that we actually get a failure.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Second thinking (sorry) - maybe we shoud throw a deprecation error, not an exception - I am afraid many contrib modules would start failing badly.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs work » Needs review
FileSize
3165588-7-test-only.patch3.31 KB
3165588-7.patch707 bytes

Addressed #5/#6. Agree that a deprecation error rather than hard fail is better, for now at least. Maybe we should revisit in Drupal 10.

Reverted #3164721: More uses of public static $modules as the test only patch.

The last submitted patch, 7: 3165588-7-test-only.patch, failed testing. View results

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Thanks @longwave

  • catch committed cb0ad5b on 9.1.x 
    Issue #3165588 by longwave, mondrake: Add a check to ensure $modules...
catch’s picture

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

Deprecation is a good idea.

Committed cb0ad5b and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented

This was only committed to 9.1.x but according to the second CR https://www.drupal.org/node/2909426 $modules should be protected from 9.0.0. So can we either
(a) backport the check to 9.0.0, or
(b) alter the CR to say that it only has to be protected from 9.1.0

It needs to be clear, because I am finalising the coding standards fixes for @trigger_error format on #3048495: Fix Drupal.Semantics.FunctionTriggerError coding standard and the message currently thrown has no versions at all. We can waste developer time if the message is not precise and we have to go hunting to find out what is actually is meant. Thanks.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented

@jonathan1055 I made this addition to the CR to address that question: https://www.drupal.org/node/2909426/revisions/view/12095831/12104390

This is definitely too disruptive to be backported to the production branch.

jonathan1055’s picture

jonathan1055 CreditAttribution: jonathan1055 as a volunteer commented

That's fine. Thanks for the clarification @xjm.