InvocationMocker::withConsecutive() is deprecated in PHPUnit 9.6 and removed from PHPUnit 10

Also see https://github.com/sebastianbergmann/phpunit/issues/4564#issuecomment-12... upstream.

In PHPUnit 9.6, using withConsecutive() will yield a deprecation error, https://github.com/sebastianbergmann/phpunit/issues/5035

47 usages in core

Replaced mostly all places with willReturnOnConsecutiveCalls() but according to https://github.com/sebastianbergmann/phpunit/issues/4026 there's no direct replacement with reference to https://thephp.cc/articles/do-not-mock-what-you-do-not-own

It still needs work for remaining places but let's see what tests will tell

revert useless hunk

Another approach in https://stackoverflow.com/questions/75389000/replace-phpunit-method-with...

Still some instances but seems on the right track.

Addressed the files mentioned in #14

The last submitted patch, 15: 3306554-15.patch, failed testing. View results →

Patch in #15 is wrong, @Rishabh Vishwakarma did you read my comments that this places needs other approach?

I think #12 is good and can be committed as a first hunk of 'easy' replacements. We need a followup to sort out the tougher ones.

Committed and pushed to 10.1.x and backported to 10.0.x and 9.5.x as a test-only fix to keep things in sync. Thanks!

Filed 📌 InvocationMocker::withConsecutive() is deprecated in PHPUnit 9.6 and removed from PHPUnit 10 - remaining replacements Closed: won't fix follow up.

Discussed this again with @mondrake in Slack. We now think that just replacing withConsecutive with willReturnOnConsecutiveCalls is not really the right thing to do - these two are not equivalent, and it just happens that here the return values probably don't matter so the tests happen to pass, but we have not made the correct change.

Therefore, reopening for revert once the current code freeze is over.

@mondrake reminded me that I had forgotten to revert these, this is done now.

The best solution I've seen around is Symfony's https://github.com/symfony/symfony/commit/431b6ef44c57a45c36146dd440a5f7...

e.g. something like

+++ b/core/modules/migrate/tests/src/Unit/MigrateSqlIdMapEnsureTablesTest.php
@@ -126,7 +126,7 @@ public function testEnsureTablesNotExist() {
     $schema->expects($this->exactly(2))
       ->method('createTable')
-      ->withConsecutive(
+     ->willReturnCallback(function (...$args) {
+               static $series = [
+                  ['migrate_map_sql_idmap_test', $map_table_schema],
+                  ['migrate_message_sql_idmap_test', $table_schema],
+               ];
+
+                $expectedArgs = array_shift($series);
+                $this->assertSame($expectedArgs, $args);
+            })
       );

(untested, illustrative)

Won't fixed the child, we need to work here I think.

Thinking about adding a generic callback method to manage this. On it.

It's not done yet, but I'd love to get a pit stop review to see if we all agree it's moving in the right direction.

Previous attempt hit the news, https://github.com/sebastianbergmann/phpunit/issues/5252#issuecomment-14......

As other users of PHPUnit are also calling for this functionality - or at least being able to test the same method with different parameters, even if the order is unimportant - then I think we should wait for this upstream pull request to be closed first: https://github.com/sebastianbergmann/phpunit/pull/5203

If it comes to it, the other option here is convert these mocks to use Prophecy instead, which already allows mocking the same method with different arguments. As an example MigrateSqlIdMapEnsureTablesTest would become

    $schema = $this->prophesize('Drupal\Core\Database\Schema');
    $schema->tableExists('migrate_map_sql_idmap_test')->willReturn(TRUE);
    $schema->fieldExists('migrate_map_sql_idmap_test', 'rollback_action')->willReturn(FALSE);
    $schema->fieldExists('migrate_map_sql_idmap_test', 'hash')->willReturn(FALSE);
    $schema->fieldExists('migrate_map_sql_idmap_test', 'source_ids_hash')->willReturn(FALSE);
    $schema->addField('migrate_map_sql_idmap_test', 'rollback_action', [
      'type' => 'int',
      'size' => 'tiny',
      'unsigned' => TRUE,
      'not null' => TRUE,
      'default' => 0,
      'description' => 'Flag indicating what to do for this item on rollback',
    ])->shouldBeCalledOnce();
    $schema->addField('migrate_map_sql_idmap_test', 'hash', [
      'type' => 'varchar',
      'length' => '64',
      'not null' => FALSE,
      'description' => 'Hash of source row data, for detecting changes',
    ])->shouldBeCalledOnce();
    $schema->addField('migrate_map_sql_idmap_test', 'source_ids_hash', [
      'type' => 'varchar',
      'length' => '64',
      'not null' => TRUE,
      'description' => 'Hash of source ids. Used as primary key',
    ])->shouldBeCalledOnce();
    $this->runEnsureTablesTest($schema->reveal());

Is #37 also checking the consecutiveness of the calls i.e. ensuring the sequence? In the example the sequence is probably not relevant, but in other cases it will be.

No, Prophecy does not support call order: https://github.com/phpspec/prophecy/issues/130

I haven't reviewed our usage in detail but I suspect that in many/most cases the order is not actually important.

Thinking again, even if upstream https://github.com/sebastianbergmann/phpunit/pull/5203 gets in, it will only be in a 10.1+ release, not backported. So we would necessarily have to keep current status while on 9, and force a change as part of the adoption of 10.x. Unlikely that we could have code supporting both at the same time.

The MR would allow that, though. We could do it now and it will work now and in 10.x. So I still think it's worth doing.

Maybe we could identify (help needed!) which are the conversion where the sequence is not strictly necessary, and in that case use ::willReturnMap() or Prophecy.

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened → , as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule → and the Allowed changes during the Drupal core release cycle → .

I reviewed this PR without reading the comments before (my bad!), as I was trying to write a test for 📌 Dispatch events for changing content moderation states Needs work with an eye on this one.
I ended up with writing a constraint. Is not the most readable test code in the world, but I think it's good enough given we don't know where that will end up. The best past is that as far as I know that approach would be compatible for 9.5.x and 10.1.x and beyond.

Thoughts? https://www.drupal.org/project/drupal/issues/2873287#comment-15026320 📌 Dispatch events for changing content moderation states Needs work

No longer applies to 11.x

Posted in #testing but not sure the best way to test this one, if any pointers available.

Rebased.

#43 not really something that can be tested manually, needs a review on whether the automated tests provided by MockTraitTest are sufficient.

From what I can tell it seems fine.

Testing passing I take as a positive sign.

Maybe the committers have another thought?

Rebased.

There is an unresolved thread with a question about the use of assertSame(). I am setting back to NR.

Rebased and replaced assertEquals with assertSame as adviced by @penyaskito.

Can't resolve thread, since I'm not the starter of this MR.

Looking at #44/45, this needs a green test result and Core Committers review, so back to RTBC, where this was before @quietone correctly pointed out the open thread on the MR.

I don’t think #49 is effective. assertSame checks for identity, which can be good for scalars but not for arrays (hence the failures in the test), and absolutely no-no for objects since in this case the one passed to the function will by definition be different than the one that will be tested.

For this purpose I specifically reused Symfony approach to allow passing in constraints that can be effectively used whenever we do not expect equality (for instance greater than, logical nots, etc)

Reversed replacing assertEquals with assertSame.

Need to understand why we now have test failures.

Rebased, removed PHPStan baseline.

Wish we could use null to typehint closures like

      ->willReturnCallback(function (mixed ...$args) use (&$callSequence): null {
        return $this->consecutiveCallsCallback($callSequence, $args);
      })

but that only works for PHP 8.2+

Like it, dropped a few minor nits and questions in the MR, mostly showing my ignorance :)
Back to NW for admin reasons, but I think we're either there or at the very least almost.

Thanks @mondrake, all clear now, RTBC.

rebased

@mondrake what do you think about the suggested solution using yield at https://github.com/sebastianbergmann/phpunit/issues/4026#issuecomment-14... ?

#61 it may work, yes. But why do you think that'd be better? Just DX? Functionally the solutions here and there (there are other ones proposed on the issue) seem equivalent i.e. providing an inplace replacement as close as possible.

The plus here is IMHO we also have tests.

This method removal really caused a lot of frustration. I wonder shall we look for a radical alternative?

DX is slightly simpler and easier to convert for anyone that used it in contrib/custom tests, if we provide a trait similar to that and a simple CR.

I do think a lot of these uses don't strictly need to be consecutive, but the issue is really that you can't mock methods that take different arguments to return different results.

Let's try then.

Yes, DX definitely improved. NR to get early feedback, but needs work to change all the conversions and maybe some more context messages in the trait to help when failures happen.

However, I am concerned that using a static method means that we cannot use it with more than one mock at a time??

@longwave in the last commit I stepped away from using static methods to keep track of the expected arguments so we can now use the solution also for multiple mocks in the same test.

Now we have to decide whether we want to keep the latest syntax

     $this->messenger->expects($this->exactly(4))
       ->method('addError')
-      ->withConsecutive(
+      ->with(...self::consecutiveCalls($this->messenger, 'addError',
         ['no title given', FALSE],
         ['element is invisible', FALSE],
         ['this missing element is invalid', FALSE],
         ['3 errors have been found: <ul-comma-list-mock><li-mock>Test 1</li-mock><li-mock>Test 2 &amp; a half</li-mock><li-mock>Test 3</li-mock></ul-comma-list-mock>', FALSE],
-      );
+      ));

passing the mock object and the method name to the helper (that is used to reflect the method parameters and provide more control and context), or just stick to a simpler

     $this->messenger->expects($this->exactly(4))
       ->method('addError')
-      ->withConsecutive(
+      ->with(...self::consecutiveCalls(
         ['no title given', FALSE],
         ['element is invisible', FALSE],
         ['this missing element is invalid', FALSE],
         ['3 errors have been found: <ul-comma-list-mock><li-mock>Test 1</li-mock><li-mock>Test 2 &amp; a half</li-mock><li-mock>Test 3</li-mock></ul-comma-list-mock>', FALSE],
-      );
+      ));

that is simpler DX but less control.

Despite having started the first option, personally I'm more inclined towards the second ATM.

I prefer the simpler DX of the second one, unless there is a case where the reflection is required in order to discover additional arguments that aren't passed in the arrays? But perhaps we just shouldn't support that, as Sebastian Bergmann is trying to discourage use of this I think we should too - I feel this should only be for converting existing methods if at all possible, and we should try to find alternative approaches for new tests.

NW to complete conversions and add a test for the latest approach. I'm on it.

But perhaps we just shouldn't support that, as Sebastian Bergmann is trying to discourage use of this I think we should too - I feel this should only be for converting existing methods if at all possible, and we should try to find alternative approaches for new tests.

Do we need a CR in this case, if we are not suggesting to use the method?

For review again.

Going purely off the issue summary of replacing "withConsecutive" I applied the MR searched for withConsecutive and only one hit in a comment in MockTrait.php.

All phpstan lines have been removed too.

So from that standpoint believe it's good for 10.2

"But perhaps we just shouldn't support that, as Sebastian Bergmann is trying to discourage use of this I think we should too - I feel this should only be for converting existing methods if at all possible, and we should try to find alternative approaches for new tests."

I agree with this sentiment. This sort of approach is pretty brittle and I'd discourage it too. I expect there are various approaches like mocking less, using prophecy to tie the returns to specific arguments, or using data providers to deal with variations on duplicate code paths.

I also wouldn't hold this up refactors all of these though and all the individual reviews associated with that. Each one likely has their own unique problem and requires complicated understanding of the related system. This is a elegant stopgap.

So my only questions are:

1. Do we need to address this: "Do we need a CR in this case, if we are not suggesting to use the method?"
2. Should we create follow ups for removal/updates?

I think we should write a change record, but the change record should probably try to discourage use of this new method and instead suggest that people find alternative methods. This can still be useful for contrib or custom tests that are trying to solve their deprecations before we upgrade to PHPUnit 10, if we can't solve all cases immediately then I doubt they can either.

We should perhaps open followups to try and remove some of these calls, but as previously stated in this issue each case needs detailed inspection and there is no one size fits all solution, so perhaps we just start with a meta where we spin off issues for individual tests or groups of tests.

Added draft CR, https://www.drupal.org/node/3405915 →

Created that followup meta to track replacing calls. 📌 [META] Convert use of InvocationMocker::withConsecutive() Active

The CR mentions the need to rethink tests and does a good job explaining the stop gap. I think that's enough. Its going to be a lot easier to provide specific examples of how a developer might think about replacing this logic after core replaces its uses. So providing that with a later deprecation of MockTrait::consecutiveCalls makes more sense IMHO. We can provide some links between the CR's at that point to make them more findable as well.

All that to say, agree this is RTBC.

I'm triaging RTBC issues → . I read the IS, the comments, the change record and the MR. I didn't find any unanswered questions. And we have a follow up to remove this 'stop gap'.

I updated the proposed resolution in the issue summary with the approach agreed to here. I also updated the CR to add emphasis not using this for new tests.

Leaving at RTBC.

I'm not convinced we should be adding code that we know we're going to deprecate - why don't we do 📌 [META] Convert use of InvocationMocker::withConsecutive() Active and be done?

This was a step towards that for both us and contrib, to wean us off the method and continue unblocking PHPUnit 10. It doesn't appear trivial to refactor each case so this helper means we (and contrib) can use this as a stopgap.

On the other hand it doesn't look like the Symfony PHPUnit bridge upgrade is going anywhere fast and so we could.postpone this and keep it on hold, in case we need it later, but try to migrate away in the mean time?

The direction is now different, but rebased anyway in case of need.

📌 [META] Convert use of InvocationMocker::withConsecutive() Active is very close, so we won't need this for core, but do we want to consider a shim for contrib? Moving back to review because things have changed since this was RTBCed and I'm not sure what the answer is.

-1 to provide a shim for contrib if core does not need it - if contrib really needs this it'd better if they implement it themselves copying from this issue.

So is this closed won't fix?

Yes I think so, although 'outdated' seems slightly more appropriate since we determined that core doesn't need the shim in the meantime.

There's change record draft for contrib
So maintainers can comment or reopen for common shim

Maybe we could add a section to the (newly rescoped) CR https://www.drupal.org/node/3365413 →

++ good idea

Done #87.

quietone → closed merge request !3795