[PHPUnit 10] @dataProvider methods must be declared static and public

Also, they all must be public https://github.com/sebastianbergmann/phpunit/blob/main/ChangeLog-10.0.md...

Hmm, I get 1124 (hopefully) relevant results for @dataProvider.

How to scope such a thing?

Are we OK with one big patch/MR that should pass with no deprecation errors as mentioned in the IS, using a tweaked composer so it will use PHPUnit 10.x? It is a straightforward replacement IMHO.

I think we should change without composer tweak. It should work now on PHPUnit 9.5 with public static; it will be just ready for the future.

Changes this big, if the scope is only the correction of visibility, I saw being done in beta release windows. Flagging for release manager input.

I think we should change without composer tweak. It should work now on PHPUnit 9.5 with public static; it will be just ready for the future.

Ah, I seemed to make myself not clear enough (per usual...)

I meant it should work now/on PHPUnit 9.5, but using PHPUnit 10.x in a "should_not_fail"-patch just to confirm we've got them all.

Anyway: Indeed beta-release window springs to mind :)

#6 no worries, it can also be said I'm dumb (per usual).

We can do that via the MR in 🌱 [meta] Support PHPUnit 10 in Drupal 11 Active .

Or,

1. tweaking DrupalListener to check the dataprovider methods' visibility and scope, like it's done today for the visibility of the $module property (should be easy, but not really in line with the intent to remove those checks), or
2. securing the visibility/scope check via PHPStan or PHPStan-phpunit (probably the best approach, but needs work upstream)

MR!3804 implements the DrupalListener check.

We need to wait for 📌 Fix PHPStan L1 errors "@dataProvider Foo related method not found." Fixed to be committed IMHO

blocker got committed.

Re #4/#5 if this can be done with a bulk find-and-replace or similar script (so it can be easily rerolled) then it can be committed in one go in a beta window - bonus points for providing the script so others can reroll it on demand.

If some data providers require separate refactoring (e.g. because they call non-static methods) then it might be best to split those out to a separate issue so they can be handled first.

#14 a possible starting point for a script is #3186661-2: Remove usage of drupalPostForm →

A script, not very sophisticated but it does what it should I think, and a patch with a diff after the script is executed.

Instructions: drop the script in the root, rename it to convert.php, execute it, and git diff the repo to produce the patch.

Some manual tweaks

With this patch, PHPStan should pinpoint only all the dataproviders that are making non-static calls.

To fix all this Undefined variable: $this because we're now in static-land, we _could_ do something like:

Pre:

  public function nonHtmlResponseProvider() {
    return [
[SNIP] 
      'A dummy that implements AttachmentsInterface' => [get_class($this->prophesize(AttachmentsInterface::class)->reveal())],
    ];
  }

Post:

  public function nonHtmlResponseProvider() {
    return [
[SNIP] 
      'A dummy that implements AttachmentsInterface' => [get_class((new BigPipeResponseAttachmentsProcessorTest())->prophesize(AttachmentsInterface::class)->reveal())],
    ];
  }

which does pass PHPStan checks.

Would that be the way to go here?

I think we need to open separate issues for each case and discover the golden pot at then end of each rainbow, there's no one-fits-all approach here I'm afraid. I am working on a first one for $this->randomMachineName(), 📌 [PHPUnit 10] Provide a static alternative to randomMachineName() and implement in data providers Fixed .

Postponing here till we have no $this...

All clear, thanks @mondrake.
(Also booh! to Yet Again no pot-o-gold at the end of the rainbow)

Re the specific case in #20, I think the overarching goal in PHPUnit by making the dataprovider static is to separate the concerns of test data provision from test data consumption, to avoid the risk that object-level common properties pollute either end of the testing cycle.

'Static' makes very clear that the object instance is not getting into the dataprovider.

If that's the case, then we should refrain from getting the test object back into the way.

For curiosity, let's see this that tries to get the prophecy directly from the factory.

Updated script to skip conversion of dataproviders that access instance properties ( $this->xxxx ). This MR will touch over 600 files.

Wow this is tough. Quite close but not there yet.

The script converts all the dataproviders that do not use calls to instance properties, that will have to be converted separately. The only exception is ConfigEntityValidationTestBase::providerInvalidMachineNameCharacters() that needs to be manually reverted to non-static, because if the method base class does not make calls to $this, the extending classes do.

So didn't look at all 674 files but skimmed a very good chunk of them and they all appear to be doing what the ticket/script describes and adding static to provider functions in tests.

Did verify only test files appear to be touched, tests are green.

Is there a way to prevent commiting new code with non-static methods?

#32 see #7.

There is a PHPStan rule checking for that, but only if PHPUnit 10 is already installed which is a bit pointless IMHO, https://github.com/phpstan/phpstan-phpunit/issues/178.

Using listeners I'm not very keen on since we're going to gut them.

Develop a custom PHPStan rule seems a bit outworldly since one exists already.

So IMHO the best thing to do now is to get these 1k in here, then watch and rebase 📌 Upgrade PHPUnit to 10, drop Symfony PHPUnit-bridge dependency Fixed where the deprecation of non-static dataprovider is reported/baselined. That will tell us if we readd some through the cracks before we bump PHPUnit to 10. Later, it will be PHPUnit 10 itself to tell us.

Thank you for elaboration! Sounds like solid plan, maybe rector's rule to have predictable conversion would be great addition)

Rector-phpunit already has some rules for this, https://github.com/rectorphp/rector-phpunit/blob/main/docs/rector_rules_... and https://github.com/rectorphp/rector-phpunit/blob/main/docs/rector_rules_...

I just found them once I had already made the script attached, and since it worked I saw no reason to change anything.

But that could be useful for contrib/custom conversions.

I was about to commit this and was scanning the diff for the last time when I had a thought: should we add array return types to each of these methods while we are here? Some already have return types, but most do not, and as we are touching the lines already...

If you disagree and think this is out of scope and we should do it separately, please set it back to RTBC.

I think it’s out of scope, in the sense that here we’re focusing in the thing needed for PHPUnit 10.

Adding return types is absolutely something to do, but then what about adding void to the test methods themselves, and using PHPStan array shapes docs to describe the array returned by the data providers, etc etc. A different story.

OK, the fact that data providers do not always return array convinced me that this is not the right place to be doing this, and we should handle it all in one go later.

Usually we would also handle this sort of bulk change in the beta phase, but as discussed in Slack it is better to land this now so we can clear the noise and figure out how to solve the issues with the remaining data providers. We should also commit this now before 10.3.x and 11.x diverge, this might become harder once we have other 11.x only changes.

Committed 654b1f2 and pushed to 11.x. Thanks!

Shouldn't this get a change record? Also to inform contrib modules extending core tests how they can do this in a way that does not break BC with 10.2.x and earlier?

Something like:

Core did:

-  public function urlProvider(): array {
+  public static function urlProvider(): array {

To make your contrib module extending this test compatible with >=10.3, make a matching change. But to retain compatibility with <=10.2, also add:

public function urlProvider(): array {
  return self::urlProvider();
}

Data providers can already be static, there is no need to add backward compatibility - we did nothing here to core except add static to method declarations, and everything still works on PHPUnit 9.

Added https://www.drupal.org/node/3421393 → - we can expand with more examples of how to refactor the more complex cases once we have figured that out ourselves.

Slightly edited the CR, hopefully for good :)

Filed 📌 Add appropriate return typehints to all @dataProvider methods Active and 📌 Add void return typehints to all test methods Active as follow ups.

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

mondrake → closed merge request !3804