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

Created on 10 April 2023, 11 months ago
Updated 28 February 2024, 3 days ago

Problem/Motivation

In PHPUnit 10, data provider methods that are not declared static yield deprecation errors in PHPUnit runs like

There were 2 PHPUnit deprecations:

1) Drupal\Tests\Core\Entity\EntityFieldManagerTest::testGetBaseFieldDefinitionsTranslatableEntityTypeDefaultLangcode
Data Provider method Drupal\Tests\Core\Entity\EntityFieldManagerTest::providerTestGetBaseFieldDefinitionsTranslatableEntityTypeDefaultLangcode() is not static

/var/www/html/core/tests/Drupal/Tests/Core/Entity/EntityFieldManagerTest.php:345

2) Drupal\Tests\Core\Entity\EntityFieldManagerTest::testGetBaseFieldDefinitionsTranslatableEntityTypeLangcode
Data Provider method Drupal\Tests\Core\Entity\EntityFieldManagerTest::providerTestGetBaseFieldDefinitionsTranslatableEntityTypeLangcode() is not static

/var/www/html/core/tests/Drupal/Tests/Core/Entity/EntityFieldManagerTest.php:382

Proposed resolution

Declare all data provider methods as static.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

๐Ÿ“Œ Task
Status

Fixed

Version

11.0 ๐Ÿ”ฅ

Component
PHPUnitย  โ†’

Last updated less than a minute ago

Created by

๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Merge Requests

Comments & Activities

Not all content is available!

It's likely this issue predates : some issue and comment data are missing.

  • Issue created by @mondrake
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands Spokje

    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.

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    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.

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands Spokje

    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 :)

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    #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 10 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)
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    MR!3804 implements the DrupalListener check.

  • Assigned to PrabuEla
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia PrabuEla chennai
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia PrabuEla chennai
  • Status changed to Postponed 11 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

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

  • Status changed to Active 11 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    blocker got committed.

  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    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.

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

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

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    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.

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    Some manual tweaks

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

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

  • Issue was unassigned.
  • ๐Ÿ‡ฎ๐Ÿ‡ณIndia PrabuEla chennai
  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands Spokje

    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?

  • Status changed to Postponed 11 months ago
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    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...

  • ๐Ÿ‡ณ๐Ÿ‡ฑNetherlands Spokje

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

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    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.

  • last update 10 months ago
    8,203 pass, 745 fail
  • last update 10 months ago
    Custom Commands Failed
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
  • Open on Drupal.org โ†’
    Environment: PHP 8.2 & MySQL 8
    last update 9 months ago
    Not currently mergeable.
  • last update 9 months ago
    Custom Commands Failed
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
  • last update 8 months ago
    Custom Commands Failed
  • Pipeline finished with Failed
    25 days ago
    Total: 181s
    #89132
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

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

  • Pipeline finished with Failed
    22 days ago
    Total: 189s
    #91693
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

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

  • Status changed to Needs review 22 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    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.

  • Pipeline finished with Success
    22 days ago
    Total: 743s
    #91696
  • Status changed to RTBC 22 days ago
  • ๐Ÿ‡บ๐Ÿ‡ธUnited States smustgrave

    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.

  • Pipeline finished with Success
    19 days ago
    Total: 512s
    #93401
  • Pipeline finished with Success
    18 days ago
    Total: 593s
    #93737
  • ๐Ÿ‡ซ๐Ÿ‡ทFrance andypost

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

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    #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 Postponed 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.

  • ๐Ÿ‡ซ๐Ÿ‡ทFrance andypost

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

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    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.

  • Status changed to Needs review 18 days ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    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.

  • Status changed to RTBC 18 days ago
  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    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.

    • longwave โ†’ committed 654b1f20 on 11.x
      Issue #3353210 by mondrake, PrabuEla, Spokje: [PHPUnit 10] @dataProvider...
  • Status changed to Fixed 17 days ago
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    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!

  • ๐Ÿ‡ง๐Ÿ‡ชBelgium Wim Leers Ghent ๐Ÿ‡ง๐Ÿ‡ช๐Ÿ‡ช๐Ÿ‡บ

    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();
    }
    
  • ๐Ÿ‡ฌ๐Ÿ‡งUnited Kingdom longwave UK

    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.

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น

    Slightly edited the CR, hopefully for good :)

  • ๐Ÿ‡ฎ๐Ÿ‡นItaly mondrake ๐Ÿ‡ฎ๐Ÿ‡น
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build https://api.contrib.social 0.61.6-2-g546bc20