Problem/Motivation

Discovered in #3063887: Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7.

Drupal\migrate_drupal\Tests\StubTestTrait implements

protected function createStub($entity_type_id)

This method has a name collision with a PHPUnit 8 method on PHPUnit\Framework\TestCase that has a different signature and a different purpose.

This will prevent future usage of PHPUnit 8.

Proposed resolution

  • Rename createStub to something else
  • Deprecate createStub
  • Add/adjust tests

Remaining tasks

API changes

\Drupal\migrate_drupal\Tests\StubTestTrait::createStub is now \Drupal\migrate_drupal\Tests\StubTestTrait:: createEntityStub

Release notes snippet

CommentFileSizeAuthor
#8 interdiff_5-8.txt1.13 KBmondrake
#8 3097822-8.patch1.74 KBmondrake
#5 interdiff_4-5.txt735 bytesmondrake
#5 3097822-5.patch2.44 KBmondrake
#4 3097822-4.patch1.72 KBmondrake
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Parent issue: » #3063887: Support PHPUnit 8 in Drupal 9, drop support for PHPUnit 7
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Issue tags: +PHPUnit 8
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Active » Needs review
FileSize
3097822-4.patch1.72 KB
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
FileSize
3097822-5.patch2.44 KB
interdiff_4-5.txt735 bytes

There's apparently only one call of ::createStub outside of the trait itself in core's test codebase.

The last submitted patch, 4: 3097822-4.patch, failed testing. View results

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at iO commented 
+++ b/core/modules/migrate_drupal/src/Tests/StubTestTrait.php
@@ -37,8 +37,27 @@ protected function performStubTest($entity_type_id) {
+   * @see https://www.drupal.org/project/drupal/issues/3097822
...
+    @trigger_error('StubTestTrait::createStub() is deprecated in drupal:8.9.0 and is removed from drupal:9.0.0. Use ::createEntityStub() instead. See https://www.drupal.org/project/drupal/issues/3097822', E_USER_DEPRECATED);

These should point to a CR not this issue.

It feels a little excessive to do this deprecation for what is essentially an internal method for the trait. Not even migrate_tools and migrate_plus are using this, wouldn't just a CR suffice here?

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Issue tags: +Needs change record
FileSize
3097822-8.patch1.74 KB
interdiff_5-8.txt1.13 KB

I like #7.

Lendude’s picture

Lendude
Primary language Dutch
Location Amsterdam
CreditAttribution: Lendude at iO commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs change record

Let's see if others agree.

Added CR, updated IS to match this direction.

CR : https://www.drupal.org/node/3105980

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented

I don't see a way around the internal API change, but also good that in practice no-one will notice so seems fine to go ahead here.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed a5876a27ac to 9.0.x and 32e999c49b to 8.9.x. Thanks!

Okay so let's commit this to 8.9.x - so just incase someone is relying on this we'll only break in a major version.

  • alexpott committed a5876a2 on 9.0.x 
    Issue #3097822 by mondrake, Lendude: Drupal\migrate_drupal\Tests\...

  • alexpott committed 32e999c on 8.9.x 
    Issue #3097822 by mondrake, Lendude: Drupal\migrate_drupal\Tests\...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Thunder commented

I've published the change record - I don't think we add to the release notes unless we discover that unexpectedly this is used a lot.

Status: Fixed » Closed (fixed)

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