[PHPUnit 10] Provide a static alternative to ConfigMapperManagerTest::providerTestHasTranslatable

Created on 7 June 2023, over 1 year ago
Updated 22 June 2023, over 1 year ago

Problem/Motivation

PHPUnit 10 deprecates use of non-static @dataProvider methods. This means that @dataProvider methods can no longer make calls to non-static methods. Any non-static call ($this->someMethod()) needs to be replaced by a call to a static alternative (static::someOtherMethod()), that would have to be implemented if not available.

In this issue, focus on replacing ConfigMapperManagerTest::providerTestHasTranslatable()

Proposed resolution

I cannot find an easy way to use PHPUnit mocks in data providers when they're static. Here I suggest to convers usage of a PHPUnit mocks to PHPSpec prophecies instead.

Let's see how this goes and then work on others.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

📌 Task
Status

Fixed

Version

11.0 🔥

Component
PHPUnit  →

Last updated about 6 hours ago

Created by

🇮🇹Italy mondrake 🇮🇹

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

Comments & Activities

  • Issue created by @mondrake
  • Status changed to Needs review over 1 year ago
  • last update over 1 year ago
    29,420 pass
  • 🇮🇹Italy mondrake 🇮🇹
  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    Woo, assuming since the tests were green your approach worked? I feel there could be a lot of these though. Should they be broken up across a few tickets? Might be easier to review and get in.

  • Status changed to Needs review over 1 year ago
  • 🇮🇹Italy mondrake 🇮🇹

    @smustgrave why NW then? What work is needed on this issue?

    If this approach (replacing PHPUnit mocks with prophecies where possible) is endorsed, then this may become a recommended way forward that we could summarize in https://www.drupal.org/node/3365413 → . But that should happen after commit IMHO.

    We need that endorsment and hopefully scope guidance on how to address the rest from a committer. Scope discussion in this type of things tend to take longer than doing the conversions themselves...

    BTW, a list of dataprovider methods that need the conversion can be seen in the latest test runs of the MR in 📌 [PHPUnit 10] @dataProvider methods must be declared static and public Fixed .

  • 🇳🇱Netherlands spokje

    Would the tag Needs framework manager review help here?

    If we have a blessing on the approach taken here, we can then either commit this one as the first of many others. or (which I see incoming) do multiple/all of them in one/multiple issues.

  • 🇺🇸United States smustgrave

    That was my mistake didn’t mean to change the status

  • 🇮🇹Italy mondrake 🇮🇹

    No worries

  • Status changed to RTBC over 1 year ago
  • 🇺🇸United States smustgrave

    Marking this to get in front of committer eyes.

  • last update over 1 year ago
    29,506 pass
  • Status changed to Fixed over 1 year ago
  • 🇬🇧United Kingdom catch
    +++ b/core/modules/config_translation/tests/src/Unit/ConfigMapperManagerTest.php
    @@ -151,13 +153,11 @@ public function providerTestHasTranslatable() {
        */
    -  protected function getElement(array $definition) {
    +  protected static function getElement(array $definition) {
         $data_definition = new DataDefinition($definition);
    -    $element = $this->createMock('Drupal\Core\TypedData\TypedDataInterface');
    -    $element->expects($this->any())
    -      ->method('getDataDefinition')
    -      ->willReturn($data_definition);
    -    return $element;
    +    $element = (new Prophet())->prophesize(TypedDataInterface::class);
    +    $element->getDataDefinition()->willReturn($data_definition);
    +    return $element->reveal();
       }
     
       /**
    @@ -169,20 +169,16 @@ protected function getElement(array $definition) {
    
    @@ -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();
       }
     
     }
    

    For me this diff looks a lot cleaner than the original, so seems like a straightforward change.

    Agreed we should document the pattern somewhere but shouldn't hold up the conversion.

    • catch → committed 0ed2aa1a on 11.x
      Issue #3365331 by mondrake: [PHPUnit 10] Provide a static alternative to...
  • Automatically closed - issue fixed for 2 weeks with no activity.

Production build 0.71.5 2024