Problem/Motivation

assertAttributeInstanceOf() is deprecated and will be removed in PHPUnit 9.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#11 3126969-11.patch4.35 KBmondrake
#11 interdiff_7-11.txt4.75 KBmondrake
#7 interdiff_6-7.txt1.6 KBmondrake
#7 3126969-7.patch5.51 KBmondrake
#6 interdiff_5-6.txt1.02 KBmondrake
#6 3126969-6.patch5.51 KBmondrake
#5 3126969-5.patch5.49 KBmondrake
#2 3126969-2.patch2.59 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
Status: Active » Needs review
FileSize
3126969-2.patch2.59 KB

IMO we can just drop those assertions that are just checking implementation details (the injection of services)

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Agree, this is not testing anything useful.

alexpott’s picture

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

Well this is testing the setter injection in \Drupal\Core\Entity\EntityTypeManager::createHandlerInstance().

I think we could make the $moduleHandler and$stringTranslation properties public in the test classes and test them.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Needs review
FileSize
3126969-5.patch5.49 KB

Different approach, with getters in the test classes.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
FileSize
3126969-6.patch5.51 KB
interdiff_5-6.txt1.02 KB
mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
FileSize
3126969-7.patch5.51 KB
interdiff_6-7.txt1.6 KB
daffie’s picture

daffie CreditAttribution: daffie commented
Status: Needs review » Reviewed & tested by the community

All instances of assertAttributeInstanceOf() have been replaced.
The creation of the class TestEntityHandlerBase has been done to test testGetHandler().
The classes TestEntityForm and TestRouteProvider have 2 extra method for testing.
The removal of suppressing of the warning for assertAttributeInstanceOf() makes sure that all instances have been removed.
All code changes look good to me.
For me it is RTBC.

alexpott’s picture

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

So the reason I suggested making the params public rather than adding methods is for the following reasons
1. We closer to the original tests without adding functionality.
2. The documentation can be 

/**
 * {@inheritdoc}
 */

3. We don't have to ponder why you've overridden \Drupal\Core\StringTranslation\StringTranslationTrait::getStringTranslation() and not \Drupal\Core\Entity\EntityHandlerBase::moduleHandler()
4. And if the upstream signatures change we do't have to worry about anything.

I should have documented this before.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented

Mmm yea. I missed that you can extend classes and change visibility of properties.

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake as a volunteer commented
Status: Needs work » Needs review
FileSize
interdiff_7-11.txt4.75 KB
3126969-11.patch4.35 KB

Here we go.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

Looks good.

  • alexpott committed de981ef on 9.1.x 
    Issue #3126969 by mondrake, longwave, alexpott, daffie: Replace usage of...

  • alexpott committed de686a9 on 9.0.x 
    Issue #3126969 by mondrake, longwave, alexpott, daffie: Replace usage of...
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Version: 9.1.x-dev » 9.0.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed de981ef81e to 9.1.x and de686a9327 to 9.0.x. Thanks!

alexpott’s picture

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

Status: Fixed » Closed (fixed)

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