Problem/Motivation

@trigger_error('The ' . get_class($test) . '::$modules property must be declared protected. See https://www.drupal.org/node/2909426', E_USER_DEPRECATED);

This could be enforced by phpcs or rector, the other option is throwing an exception here but there's no version information in the deprecation message so that'd probably have to change to $version_it_was_added to be enforced in 11.0.0

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3301205

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

catch’s picture

catch
he/him
Primary language English
commented
Parent issue: » #3301206: [meta] Fix incomplete/incorrect deprecations in Drupal core

mondrake made their first commit to this issue’s fork.

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented

@mglaman worked on a rector rule for this recently at https://github.com/palantirnet/drupal-rector/pull/210 and phpstan rule at https://github.com/mglaman/phpstan-drupal/issues/463

The deprecation still needs updating to have the expected version format and deprecate in Drupal 11.

Gábor Hojtsy opened merge request !2580

gábor hojtsy’s picture

gábor hojtsy
he/him
Primary language Hungarian
Location Hungary
commented
Status: Active » Needs review

Proposed an update based on the suggestion in the issue summary. The original CR at https://www.drupal.org/node/2909426 says this is going to be required from Drupal 9.0.0 so that needs updating if/when this lands.

quietone’s picture

quietone commented
Status: Needs review » Reviewed & tested by the community

I searched for change records and found the following.
#2833984: $modules property on BrowserTestBase and KernelTestBase is protected : Announced the change to protected in 8.2.x.
#2909426: $modules is now a protected property in tests : States "Starting with Drupal 9.0.0 this property should now always be protected. In 9.1.0-alpha1 and above, making the property public will cause a deprecation to be thrown."

So, yes the new deprecation message is correct.

mondrake’s picture

mondrake
Location 🇮🇹
commented

IMHO the point of this issue should be to remove completely the deprecation message and the implementation of the check in DrupalListener, not to change the string of the error.

However, there's no phpstan-drupal implementation yet (and that's the one that would be relevant here for core, since the PHPStan check is run at every test, not the rector one), so for me the actual status should be postponed on upstream.

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Replace the test class protected $modules deprecation error with a phpcs or rector rule » Replace the test class protected $modules deprecation error with a phpstan rule
Status: Reviewed & tested by the community » Postponed

I think I agree with #8. Thanks!

xjm’s picture

xjm
she/her
Primary language English
commented
Title: Replace the test class protected $modules deprecation error with a phpstan rule » Replace the test class protected $modules deprecation error with a phpstan-drupal rule
Issue tags: +Needs followup

Tagging "needs followup" for filing an issue against phpstan-drupal (even though it's actually a blocker rather than a followup).

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

mondrake’s picture

mondrake
Location 🇮🇹
commented

It's done upstream https://github.com/mglaman/phpstan-drupal/issues/463, we need a point relaease to pin to and then we can get rid of the check.

mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: Replace the test class protected $modules deprecation error with a phpstan-drupal rule » [upstream] Replace the test class protected $modules deprecation error with a phpstan-drupal rule
Issue tags: -Needs followup
mondrake’s picture

mondrake
Location 🇮🇹
commented
Title: [upstream] Replace the test class protected $modules deprecation error with a phpstan-drupal rule » [PP-upstream] Replace the test class protected $modules deprecation error with a phpstan-drupal rule
mondrake’s picture

mondrake
Location 🇮🇹
commented

mglaman/phpstan-drupal just released 1.2.1. Once that's updated in core, this issue will be feasible.

quietone’s picture

quietone commented

Added #3399419: Update mglaman/phpstan-drupal to 1.21 for 3301205

quietone’s picture

quietone commented
Status: Postponed » Active

#3399419: Update mglaman/phpstan-drupal to 1.21 for 3301205 was committed. This is no longer postponed.

quietone’s picture

quietone commented
Title: [PP-upstream] Replace the test class protected $modules deprecation error with a phpstan-drupal rule » Replace the test class protected $modules deprecation error with a phpstan-drupal rule

Spokje made their first commit to this issue’s fork.

Spokje opened merge request !5310

mondrake’s picture

mondrake
Location 🇮🇹
commented
Status: Active » Reviewed & tested by the community

MR 5310 is OK. It bumps the minimum phpstan-drupal release to the one that performs the check, hence the hash in composer.lock needs to change. Tests for the feature removed from the listener and added to phpstan-drupal are performed upsteram, so this is just a net removal.

Thanks

spokje’s picture

spokje
Primary language Dutch
commented

Damn @mondrake, that's very quick and _exactly_ the reasoning I was about to put into the IS! :)

Looking at \Drupal\Tests\Listeners\DrupalListener a bit closer, I feel we can replace (and then deprecate \Drupal\Tests\Listeners\DrupalComponentTestListenerTrait) with another phpstan-drupal rule. I feel core is doing work in that trait that is now coverable by PHPStan.

After that is done, I think DrupalListener is basically an "empty" wrapper around \Symfony\Bridge\PhpUnit\SymfonyTestsListener and might be up for removal?
But the latter is me treading into waters that are above my pay grade.

I'll open up an issue for DrupalComponentTestListenerTrait in here (postponed) and one in the mglaman/phpstan-drupal queue shortly.

spokje’s picture

spokje
Primary language Dutch
commented
Related issues: +#3400366: Remove DrupalComponentTestListenerTrait and replace with a PHPStan rule
mondrake’s picture

mondrake
Location 🇮🇹
commented

#23 cool.

The follow-up is great.

General point with listeners: they are deprecated and simply no longer there in PHPUnit 10.
So my approach is: any steps taken to thoroughly remove them is good; any step in the direction of refactoring them but keep them is a waste of time.

spokje’s picture

spokje
Primary language Dutch
commented

Thanks for the confirmation @mondrake, I try to keep up with all the PHPUnit/Symfony-bridge work you're doing, but it's hard to keep up with your good work.

mondrake’s picture

mondrake
Location 🇮🇹
commented

Thanks... honestly I'm not even sure where Symfony's phpunit-bridge is going, see e.g. https://github.com/sebastianbergmann/phpunit/issues/5532.

So, ATM we're pretty much sitting on the river's bank...

  • longwave committed e0ce28fa on 10.2.x 
    Issue #3301205 by Spokje, mondrake, Gábor Hojtsy, quietone, catch, xjm:...

  • longwave committed 9d64e97e on 11.x 
    Issue #3301205 by Spokje, mondrake, Gábor Hojtsy, quietone, catch, xjm:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

+1 to removing the listeners as it is a step towards PHPUnit 10 even if that seems to be getting no closer every time I look at it.

Committed and pushed to 11.x and 10.2.x, thanks!

Spokje closed merge request !5310

Status: Fixed » Closed (fixed)

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

quietone closed merge request !2580