Properly deprecate methods in Drupal\FunctionalTests\AssertLegacyTrait and Drupal\KernelTests\AssertLegacyTrait, while still keeping deprecations silenced

Discussed in Slack, there are also a number of missing trigger_error() calls in these deprecations, so we should add these here if possible.

Nice!

I don't think these are properly "active" deprecations until trigger_error() is added. I suspect that the reason some of them don't have trigger_error() yet is that core still uses these deprecated methods in non-legacy tests...

then we have to enforce trigger_error() according to deprecation policy

Great idea! While there's not a lot of them

We also are supposed to hold off on adding new deprecations until 9.1.x so this might have to be postponed until then.

As 9.1.x development is underway I think we can look at this again.

Can we expand the scope of this to cover \Drupal\KernelTests\AssertLegacyTrait as well? The attached patch adds @deprecated notices for that class as well.

Actually let's use the correct syntax, $this instead of self::

Not sure what to do about verbose().

Parenting. IMHO:

1) this should be the first sub-issue for #3027952: [Plan] Remove the usage of deprecated methods in tests
2) we should make the proper deprecations here, adding the @see to reference CRs, the @trigger_error for the deprecation, and a silencer for the trigger in the DeprecationListener. Later issues will tackle removing the silencers.

Added @trigger_error and dummy @see - I can't actually find change records for any of this, so adding the tag for that too.

Drafted change record, https://www.drupal.org/node/3129738

Working on deprecation listener.

Thanks for adding the change record, I updated it to include a table of replacement methods based on the deprecation notices.

A handful of the previous deprecation notices did have links, some to change records and some to issues, but I think it's easier for developers if we put all the information in one place now.

#23 edited the CR to update about the recommendation to drop calls to pass.

And yes, one stop shop for all this is helpful :)

This looks good to go to me, but I don't think I can RTBC as I worked on it.

I will be bold here, RTBC

It’s not a complicated patch by iteself, no conditional execution, just adding the proper deprecation to methods that needed that long, silencing them while in to be issues the usages (tons of them!) will be finally removed in an orderly manner.

Updated IS with current proposal

Needs code style fixes

Only changes to CS, Coder is now checking the format of deprecations.

Related could affect this one, rtbc++

We're definitely not adding new deprecations to Drupal 8 anymore (we didn't anyway, but definitely not at almost-RC). So untagging RM review. Thanks!

1. +++ b/core/tests/Drupal/FunctionalTests/Core/Test/AssertLegacyTraitDeprecatedTest.php
   @@ -27,7 +27,7 @@ class AssertLegacyTraitDeprecatedTest extends BrowserTestBase {
   -   * @expectedDeprecation AssertLegacyTrait::getAllOptions() is deprecated in drupal:8.5.0 and is removed from drupal:10.0.0. Use $element->findAll('xpath', 'option') instead. See http://drupal.org/node/2735045
   +   * @expectedDeprecation AssertLegacyTrait::getAllOptions() is deprecated in drupal:8.5.0 and is removed from drupal:10.0.0. Use $element->findAll('xpath', 'option') instead. See https://www.drupal.org/node/3129738
   

   The diff here and in several other places is just changing the link from the issue to the CR, which is correct. However, that sort of seems like a separate issue scope?

2. +++ b/core/tests/Drupal/KernelTests/AssertLegacyTrait.php
   @@ -83,10 +99,14 @@ protected function assertIdenticalObject($actual, $expected, $message = '') {
   +   * @deprecated in drupal:8.0.0 and is removed from drupal:10.0.0. PHPUnit
   +   *   fails a test as soon as a test assertion fails, do not call this method
   +   *   and/or refactor the test so it is not needed.
   

   The double-negatives here are confusing. I'd suggest:

   PHPUnit fails as test as soon as a test assertion fails, so there is usually no need to call this method. If a test's logic relies on this method, refactor the test.

   (And the same for the @trigger_error(), of course.)

Thanks!

#35:

1. IMHO that'd be overkill. I have seen going back and forth with deprecations of this in D8, I'd suggest to make it clear we need to remove those methods by D10, now. This will unleash a lot of work, at least one issue for each of the deprecations, so better have it as soon as we can in 9.1.x
2. Done.

Thanks!

I suggest to postpone this on #3114617: Properly deprecate methods in Drupal\FunctionalTests\AssertLegacyTrait and Drupal\KernelTests\AssertLegacyTrait, while still keeping deprecations silenced and on completion of #3128573: [meta] Replace assertions with more appropriate ones, since the sub-issues of #3128573: [meta] Replace assertions with more appropriate ones overlap with the changes here.

Sorry, post to wrong issue in #37.

Review points addressed, back to RTBC.

#35.1 was not addressed. I'm suggesting to split this patch into two, separating out the different pattern of updating just that one link to a separate issue, which will make it easier to review, commit, and easier get into 9.1.x as you suggest. See https://www.drupal.org/core/scope#context and https://www.drupal.org/core/scope#size for more information. Thanks!

@xjm,

Addressing #35.1. I have updated the patch to remove

+++ b/core/tests/Drupal/FunctionalTests/Core/Test/AssertLegacyTraitDeprecatedTest.php
@@ -27,7 +27,7 @@ class AssertLegacyTraitDeprecatedTest extends BrowserTestBase {
-   * @expectedDeprecation AssertLegacyTrait::getAllOptions() is deprecated in drupal:8.5.0 and is removed from drupal:10.0.0. Use $element->findAll('xpath', 'option') instead. See http://drupal.org/node/2735045
+   * @expectedDeprecation AssertLegacyTrait::getAllOptions() is deprecated in drupal:8.5.0 and is removed from drupal:10.0.0. Use $element->findAll('xpath', 'option') instead. See https://www.drupal.org/node/3129738

and other related changes from the patch.

I will create a separate issue for this change.

Created separate issue to track the remaining part of the scope of this issue.

Change the link in @trigger_error and other related palces from the issue to the CR

Personally I think this was OK to do in the same issue as then it made everything nice and consistent, but as this change has now been made let's get this in and fix the followup issue soon.

@sja112 Thanks for working on this and creating the followup.

Needs a reroll. Thanks!

@xjm,

I have re-rolled the patch.

Thanks.

On it

Failed test not working because of
AssertLegacyTrait::assertHeader() is deprecated in drupal:8.3.0 and is removed from drupal:10.0.0. Use $this->assertSession()->responseHeaderEquals() instead. See https://www.drupal.org/node/3129738

Somehow changing it to drupal:8.2.0 from drupal:8.3.0 in AssertLegacyTrait::assertHeader() --> @trigger_error its working.

+++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
@@ -741,10 +844,13 @@ protected function assertNoCacheTag($cache_tag) {
+    @trigger_error('AssertLegacyTrait::assertHeader() is deprecated in drupal:8.3.0 and is removed from drupal:10.0.0. Use $this->assertSession()->responseHeaderEquals() instead. See https://www.drupal.org/node/3129738', E_USER_DEPRECATED);

+++ b/core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
@@ -166,6 +166,52 @@ public static function getSkippedDeprecations() {
+      'AssertLegacyTrait::assertHeader() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->responseHeaderEquals() instead. See https://www.drupal.org/node/3129738',

The two versions should be identical. changed 8.3.0 to inline with 8.2.0 in DeprecationListenerTrait

Reroll looks good, back to RTBC.

Reviewing this one again.

All right, I tried to read over each deprecation carefully to make sure that the method of names and deprecation's matched. A couple review notes:

1. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -221,12 +243,15 @@ protected function assertResponse($code) {
   +    @trigger_error('AssertLegacyTrait::assertFieldByName() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->fieldExists() or $this->assertSession()->buttonExists() or $this->assertSession()->fieldValueEquals() instead. See https://www.drupal.org/node/3129738', E_USER_DEPRECATED);
   
   @@ -241,12 +266,15 @@ protected function assertFieldByName($name, $value = NULL) {
   +    @trigger_error('AssertLegacyTrait::assertNoFieldByName() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->fieldNotExists() or $this->assertSession()->buttonNotExists() or $this->assertSession()->fieldValueNotEquals() instead. See https://www.drupal.org/node/3129738', E_USER_DEPRECATED);
   
   @@ -263,12 +291,15 @@ protected function assertNoFieldByName($name, $value = '') {
   +    @trigger_error('AssertLegacyTrait::assertFieldById() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->fieldExists() or $this->assertSession()->buttonExists() or $this->assertSession()->fieldValueEquals() instead. See https://www.drupal.org/node/3129738', E_USER_DEPRECATED);
   

   This is minor, but it's in enough places (including the test) that I don't feel comfortable fixing it on commit. We don't typically join lists of more than two elements in English with or. It should have commas: "A, B, or C."

   It looks like the existing deprecation blocks for the same methods already have this problem, though, so not in scope to fix it here.

2. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -694,7 +794,7 @@ protected function assertPattern($pattern) {
       *
   -   * Deprecated in drupal:8.4.0 and is removed from drupal:10.0.0. Use
   +   * @deprecated in drupal:8.4.0 and is removed from drupal:10.0.0. Use
       *   $this->assertSession()->responseNotMatches() instead.
   
   @@ -723,7 +826,7 @@ protected function assertCacheTag($expected_cache_tag) {
       * @param string $cache_tag
       *   The cache tag to check.
       *
   -   * Deprecated in drupal:8.4.0 and is removed from drupal:10.0.0. Use
   +   * @deprecated in drupal:8.4.0 and is removed from drupal:10.0.0. Use
       *   $this->assertSession()->responseHeaderNotContains() instead.
   

   These don't have a @trigger_error() in the patch. I confirmed in each case that they're already present in the codebase.

3. +++ b/core/tests/Drupal/FunctionalTests/AssertLegacyTrait.php
   @@ -808,7 +920,7 @@ protected function constructFieldXpath($attribute, $value) {
   -   * Deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use
   +   * @deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use
       *   $this->getSession()->getPage()->getContent() instead.
   
   @@ -827,7 +939,7 @@ protected function getRawContent() {
   -   * Deprecated in drupal:8.5.0 and is removed from drupal:10.0.0. Use
   +   * @deprecated in drupal:8.5.0 and is removed from drupal:10.0.0. Use
       *   $element->findAll('xpath', 'option') instead.
       *
   

   Same.

Committed to 9.1.x, and published the CR. Thanks!

Adding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions

Closed #2496109: Replace all usages to deprecated methods on KernelTestBase(NG) as a duplicate, adding credit.