Problem/Motivation

Symfony PHPUnit bridge 5 has added their own \Symfony\Bridge\PhpUnit\ExpectDeprecationTrait that we can use.

#3156998: Using @requires extension_name in PHPUnit unit tests fails if extension is not present has changed our ExpectDeprecationTrait to use the new Symfony trait but now we can deprecate our own trait in favour of Symfony's.

Currently postponed on #3156998: Using @requires extension_name in PHPUnit unit tests fails if extension is not present and fixing https://github.com/symfony/symfony/pull/37515 upstream.

Proposed resolution

Add @deprecation annotations, @trigger_error()'s, replace core usages, and add a deprecation test.

Remaining tasks

User interface changes

None

API changes

@todo

Data model changes

None

Release notes snippet

@todo

CommentFileSizeAuthor
#10 3157434-10.patch21.15 KBalexpott
#10 3-10-interdiff.txt769 bytesalexpott
#3 3157434-3.patch21.09 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Issue summary: View changes
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Postponed » Needs review
FileSize
3157434-3.patch21.09 KB

The blocker has landed and Symfony 5.1.3 has been released with the upstream fix.

alexpott’s picture

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

Less core code for the win! 16 files changed, 27 insertions, 167 deletions.

+++ b/composer/Metapackage/PinnedDevDependencies/composer.json
index 9a836b97e5..cea0a49e7a 100644
--- a/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldResourceTestBase.php

--- a/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldResourceTestBase.php
+++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldResourceTestBase.php

+++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldResourceTestBase.php
+++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldResourceTestBase.php
@@ -4,13 +4,10 @@

@@ -4,13 +4,10 @@
 
 use Drupal\entity_test\Entity\EntityTestMapField;
 use Drupal\Tests\rest\Functional\EntityResource\EntityResourceTestBase;
-use Drupal\Tests\Traits\ExpectDeprecationTrait;
 use Drupal\user\Entity\User;
 
 abstract class EntityTestMapFieldResourceTestBase extends EntityResourceTestBase {
 
-  use ExpectDeprecationTrait;
-

A lot of legacy removals forgot to remove the use of the trait. This is cleaning that up.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@alexpott as I see in composer.lock funding was removed, is it intensional? Iirc it was added in separate issue

alexpott’s picture

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

@andypost it's what is happening when I run composer update - I have no control over that

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

@alexpott thanks, I just facing it last months... looks like packagers removing this key faster then they been added it

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs review » Reviewed & tested by the community

Nice cleanup!

Nit:

+++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
@@ -2,17 +2,18 @@
+  use \Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;

don't we prefer to alias the base trait in the class imports list and then use the alias in the class body?

alexpott’s picture

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

@mondrake I did that because the trait has the same name. Saved an alias.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3-10-interdiff.txt769 bytes
3157434-10.patch21.15 KB

I think we've got issues trying to remove usages of FQDNs so let'z fix that.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff-10-11.txt937 bytes
3157434-11.patch21.54 KB

Rerolled the patch based on #10 and fixed test(s) in Drupal\Tests\Core\Site\SettingsTest

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented
Status: Needs review » Needs work 
+++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -3,9 +3,9 @@
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;

I don't think the intention is that we have to manually use this anywhere. It should be loaded for us, right?

catch’s picture

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

Needs a reroll again.

jungle’s picture

jungle
He/Him
Location Chongqing, China
CreditAttribution: jungle as a volunteer commented

Needs a reroll again.

#11 is still valid to me.

alexpott’s picture

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

#13 - #11 looks good to me.
#12 - we need to use the trait in classes that use the method at the moment. Given that Symfony has deprecated @expectedDeprecation annotations we'll probably end up adding it to the base classes once we deal with that deprecation.

alexpott’s picture

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

For some reason @jungle was not credited for the reroll. @mondrake also is credited for #8

dww’s picture

dww
we/he/they
CreditAttribution: dww as a volunteer commented

Careful re-review. TL;DR: RTBC +1! ;)

  1. +++ b/composer.lock
@@ -183,16 +183,6 @@
-            "funding": [

@@ -385,20 +375,6 @@
-            "funding": [

    This is a bummer per #5, but #6 explains there's nothing we can do about this kind of noise. Alas. 

  2. +++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestMapFieldResourceTestBase.php
@@ -4,13 +4,10 @@
-  use ExpectDeprecationTrait;

+++ b/core/modules/system/tests/modules/entity_test/tests/src/Functional/Rest/EntityTestResourceTestBase.php
@@ -5,13 +5,11 @@
-  use ExpectDeprecationTrait;

+++ b/core/tests/Drupal/KernelTests/Core/Plugin/Context/ContextAwarePluginBaseTest.php
@@ -23,8 +22,6 @@
-  use ExpectDeprecationTrait;

+++ b/core/tests/Drupal/Tests/Core/Entity/ContentEntityBaseUnitTest.php
@@ -24,8 +23,6 @@
-  use ExpectDeprecationTrait;

    I now understand that most of this patch is just cleaning up the unused trait from prior work (now that I read #4 carefully). That's part of why I was confused about SettingsTest. 

  3. +++ b/core/tests/Drupal/Tests/Core/Site/SettingsTest.php
@@ -3,9 +3,9 @@
+use Symfony\Bridge\PhpUnit\ExpectDeprecationTrait;

    Now I see. So we'll move this to our test framework base classes in a follow-up. Sounds good. 

  4. +++ b/core/tests/Drupal/Tests/ExpectDeprecationTest.php
@@ -9,12 +9,17 @@
+ * Do not remove this test when \Drupal\Tests\Traits\ExpectDeprecationTrait is

    Thanks for this comment! ;)

  5. +++ b/core/tests/Drupal/Tests/ExpectDeprecationTest.php
@@ -25,10 +30,12 @@ public function testExpectDeprecation() {
-    $this->addExpectedDeprecationMessage('Test isolated deprecation2');
+    $this->expectedDeprecations(['Test isolated deprecation2']);

+++ b/core/tests/Drupal/Tests/Traits/ExpectDeprecationTrait.php
@@ -33,29 +37,9 @@ protected function addExpectedDeprecationMessage($message) {
+    @trigger_error(__METHOD__ . '() is deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use \Symfony\Bridge\PhpUnit\ExpectDeprecationTrait::expectDeprecation() instead. See https://www.drupal.org/node/3161901', E_USER_DEPRECATED);

    At first this was a head-scratcher. Now I see it's because we're also deprecating expectedDeprecations() here, and we want to test that. The previous test was testing the same thing twice. Now the 2 calls are to each of our dynamic deprecation expect methods. 👍

  6. I love how much code this patch lets us remove! 🎉

Thanks,
-Derek

  • catch committed 574f861 on 9.1.x 
    Issue #3157434 by alexpott, jungle, dww, mondrake: Deprecate \Drupal\...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Status: Reviewed & tested by the community » Fixed

Not sure where I went wrong in #13...

Committed 574f861 and pushed to 9.1.x. Thanks!

Status: Fixed » Closed (fixed)

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented

Publish change record