Change @dataprovider to static in CommentLinkBuilderTest

1. Checked code changes and they are (mostly) correct Mock -> Prophecy conversions.
2. Pattern matches the one agreed in #3365331: [PHPUnit 10] Provide a static alternative to ConfigMapperManagerTest::providerTestHasTranslatable
3. Green TestBot

Checking 1. for only these two tests was already quite a strain on my mind, so I can agree with splitting this up in to multiple/many issues.

Love it!

RTBC for this one, core committers/other big brains will chime in when this is on their RTBC-radar.

However in the already committed #3365331: [PHPUnit 10] Provide a static alternative to ConfigMapperManagerTest::providerTestHasTranslatable we have a

--- a/core/modules/config_translation/tests/src/Unit/ConfigMapperManagerTest.php
+++ b/core/modules/config_translation/tests/src/Unit/ConfigMapperManagerTest.php
@@ -169,20 +169,16 @@ protected function getElement(array $definition) {
    * @return \Drupal\Core\Config\Schema\Mapping
    *   A nested schema element, containing the passed-in elements.
    */
-  protected function getNestedElement(array $elements) {
+  protected static function getNestedElement(array $elements) {
     // ConfigMapperManager::findTranslatable() checks for
     // \Drupal\Core\TypedData\TraversableTypedDataInterface, but mocking that
     // directly does not work, because we need to implement \IteratorAggregate
     // in order for getIterator() to be called. Therefore we need to mock
     // \Drupal\Core\Config\Schema\ArrayElement, but that is abstract, so we
     // need to mock one of the subclasses of it.
-    $nested_element = $this->getMockBuilder('Drupal\Core\Config\Schema\Mapping')
-      ->disableOriginalConstructor()
-      ->getMock();
-    $nested_element->expects($this->once())
-      ->method('getIterator')
-      ->willReturn(new \ArrayIterator($elements));
-    return $nested_element;
+    $nested_element = (new Prophet())->prophesize(Mapping::class);
+    $nested_element->getIterator()->shouldBeCalledTimes(1)->willReturn(new \ArrayIterator($elements));
+    return $nested_element->reveal();
   }

I think/suppose/guess we have to revisit that after agreeing and commiting this, because of the $nested_element->getIterator()->shouldBeCalledTimes(1) and do the TearDown-Tango there as well?

Ah, didn't even think of that one, adding it as related.

Also, I would like to claim "Data Provider Prophets of Doom" as the name for my next metal band...

I'm not convinced we are doing the right thing here. I feel that by making data providers static, the PHPUnit authors are implying that data providers should be fast and not really be doing any work - and to me this includes creating mocks. If we need custom mocks per test combination, I think we should be providing the values required to create the mock into the test method, but not the mock itself?

In two of the cases here we could just adjust the arguments to the test method and then create the mock inside the test, I think that would be easier to follow than adding a new Drupalism to tests.

So would next steps be for this issue?

There was conversation between @catch, @mondrake, and @longwave on #contribute in slack. I was just a fly on the wall as this is above my head.

@mondrake proposed

I think the options here are:
1. we do this issue, which introduces a new API, allow checking call count expectations, etc - Symfony has done something similar.
2. we just drop all the calls to count expectation methods in dataproviders that use prophecy --> but in that case we need to correct other conversions and possibly find a way to prevent them from being called (PHPStan rule?)
3. we drop all the usage of Prophecy and createMock (that are not available statically) and convert all dataproviders to only use createStub - to be investigated if possible and require adjusting any conversion done already
4. ??

@catch

If upstream isn't helping we might want to do #1 and then open follow-ups to explore 2-4, since #1 would allow for conversions without massive refactoring of each individual test.

@longwave

a possible option 4 is check what other projects with large PHPUnit suites have done or are planning to do about this, if they also ran into it - unsure if there are any though

So tagging for follow ups but marking as this may be the best way forward (from my understanding), based on upstream phpunit.

If I paraphrased wrong sorry!

I'm triaging RTBC issues. I read the IS and the comments.

@smustgrave, thanks for recording the conversation from Slack. I do think it is better for all if summaries are in the Issue Summary. Having a link to the Slack conversation would help as well, which is https://drupal.slack.com/archives/C1BMUQ9U6/p1694529624846449

I read that thread and I am not convinced that there is agreement to any of the 4 options.

I am setting this back to Needs Review for make sure we agree on the direction. I will also mention this issue in committer slack.

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

What was the outcome of #15 - did we do research into how other projects are handling this.

I'm in the same camp as @longwave in #12 - I think we should be passing values in the data provider and create the mocks in the ::testSomething methods

Or failing that another option is to return a Closure that is bound to the class. In client projects we do that with Drupal Testing Traits because Drupal APIs aren't yet available in data providers.

public function someDataProvider(): array {
  return [
   'some node thing' => [fn(array $values) => $this->createSomeNode($values)],
  ],
}

/**
 * @dataProvider someDataProvider
*/
public function testSomething(callable $factory): void {
  $user = $this->createUser();
  $node = $factory(['uid' => $user]);
  // ...
}

I _think_ that would work here

Not sure an option was agreed upon. Moving to NW so the issue summary can be updated to include what the decided outcome should be.

Changing to Needs review because this needs an answer on the approach.

@larowlan support @longwave in passing values in the data provider and create the mocks in the ::testSomething methods. See #12.

Tried an alternative approach for CommentLinkBuilderTest in MR!6661.

MailHandlerTest is a bit more messy but I think we could clean it up to make the test more readable at the same time.

The Needs Review Queue Bot tested this issue. It fails the Drupal core commit checks. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

Committed and pushed e021637080 to 11.x and 9ad8446eb0 to 10.3.x. Thanks!