Problem/Motivation

I think there's a typo in core/tests/PHPStan/Rules/TestClassClassMetadata.php

      if ($should_run_in_separate_process && !$has_run_in_separate_process_attribute) {
        $fails[] = RuleErrorBuilder::message("Test class {$class->getName()} must have attribute \PHPUnit\Framework\Attributes\RunInSeparateProcesses.")
          ->identifier('testClass.missingAttribute.RunInSeparateProcesses')
          ->line($node->getStartLine())
          ->build();
      }

That should be RunTestsInSeparateProcesses (missing the word "Tests").

Proposed resolution

Fix all instances of the typo "RunInSeparateProcesses".

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3626548

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

mcdruid created an issue. See original summary.

mcdruid’s picture

mcdruid commented
Issue summary: View changes

bt dev2 made their first commit to this issue’s fork.

bt dev2 opened merge request !17291

bt dev2’s picture

bt dev2 commented
Status: Active » Needs review

I have fixed the typo in \Drupal\PHPStan\Rules\TestClassClassMetadata::processNode().

Edit:

I also fixed the same typos in core/tests/PHPStan/tests/TestClassClassMetadataTest.php 

$this->analyse
[
  'Test class Drupal\Tests\Core\Foo\MissingAttributes must have attribute \PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses.',
  18,
],

[
  'Test class Drupal\Tests\Core\Foo\MissingRunTestsInSeparateProcesses must have attribute \PHPUnit\Framework\Attributes\RunTestsInSeparateProcesses.',
  25,
],

But I still see the typo here, should we fix this as well?
/core/tests/Drupal/Tests/Core/Test/BrowserTestBaseTest.php

/**
 * A class extending BrowserTestBase for testing purposes.
 *
 * @phpstan-ignore testClass.missingAttribute.Group, testClass.missingAttribute.RunInSeparateProcesses
 */
class BrowserTestBaseMockableClassTest extends BrowserTestBase {

}
dcam’s picture

dcam commented
Status: Needs review » Needs work

But I still see the typo here, should we fix this as well?

Yes, it isn't just appropriate to fix it, it is also the reason for the PHPStan linting failure.

bt dev2’s picture

bt dev2 commented
Status: Needs work » Needs review

Yes, I fixed a typo in the BrowserTestBaseMockableClassTest doc. PHPStan linting is passing now, along with tests.

dcam’s picture

dcam commented
Title: typo in \Drupal\PHPStan\Rules\TestClassClassMetadata::processNode » Fix RunInSeparateProcesses typos
Issue summary: View changes

Thank you @bd dev2.

I grepped Core for any additional instances of the string "RunInSeparateProcesses". None were found. This looks good to me.

dcam’s picture

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

I forgot to set the status to RTBC, but it's fine because I wanted to add more anyway.

The PHPStan error message changes are necessary because they reference the incorrect class name. But committers should consider the downstream impact of changing the rule identifier, testClass.missingAttribute.RunInSeparateProcesses. The contrib code search doesn't return any results for the bad "RunInSeparateProcesses" name, but there could be someone out there using it, for instance to ignore the rule. Their tests would suddenly start failing due to the change.

mcdruid’s picture

mcdruid commented

Thanks for working on this.

... consider the downstream impact of changing the rule identifier, testClass.missingAttribute.RunInSeparateProcesses

The incorrect version's been there for nearly a year since 8e1a45a4d687.

So I think you're right, but I'd vote for correctness over leaving the mistake in place.

If that causes some people a little work to update things, sorry.. but it'll be correct from now on.

Others may have a different opinion though; the name of the identifier's perhaps not that significant.

  • catch committed bb0afec1 on 11.x 
    fix: #3626548 Fix RunInSeparateProcesses typos

By: mcdruid
By: bt dev2...

  • catch committed dafb9ad5 on 12.0.x 
    fix: #3626548 Fix RunInSeparateProcesses typos

By: mcdruid
By: bt dev2...

  • catch committed fc11adb9 on main 
    fix: #3626548 Fix RunInSeparateProcesses typos

By: mcdruid
By: bt dev2...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.x-dev
Status: Reviewed & tested by the community » Fixed

I can't imagine anyone is relying on this outside this one test, so I've gone ahead and cherry-picked to 12.0.x and 11.5.x. I also don't think we should consider PHPStan rules to be 'API' in any sense at all. Generating a new baseline or updating a skip can happen for any number of reasons including phpstan updates themselves.

Committed/pushed to main and cherry-picked to 12.0.x and 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed 3abfd240 on 11.x 
    Revert "fix: #3626548 Fix RunInSeparateProcesses typos"

This reverts...
catch’s picture

catch
he/him
Primary language English
commented
Status: Fixed » Needs work

This broke 11.x, https://git.drupalcode.org/project/drupal/-/pipelines/989488 will need a backport MR (or we could decide not to backport it).

bt dev2’s picture

bt dev2 commented

I could fix the typo in core/tests/Drupal/Tests/Core/Test/WebDriverTestBaseTest.php and push it to the "3626548-11.x-typo-in-drupalphpstanrulestestclassclassmetadataprocessnode" branch, where @catch reverted the changes.

And then, should we create another MR targeting 11.x?

catch’s picture

catch
he/him
Primary language English
commented

A new MR targeting 11.x is good yes - then we can double check that phpstan continues to pass before committing it (which is what I should have asked for in the first place here).

bt dev2 opened merge request !17348

bt dev2’s picture

bt dev2 commented

I created MR !17348 for 11.x. All tests and PHPStan analysis are passing.

dcam’s picture

dcam commented
Status: Needs work » Reviewed & tested by the community

The backport looks good. There are no more instances of the typo string remaining in 11.x.