Problem/Motivation

AssertLegacyTrait::assertEscaped() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->assertEscaped() instead. See https://www.drupal.org/node/3129738

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mondrake created an issue. See original summary.

jungle’s picture

Issue tags: +Deprecated assertions

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

mondrake’s picture

Status: Active » Postponed

Some of the current calls to assert(No)Escaped in functional tests still have a message passed in, even if AssertLegacyTrait::assert(No)Escaped() do not take that in. We need to remove the messages from the calls and inline them as comments where appropriate before doing the conversion.

mondrake’s picture

mondrake’s picture

Assigned: Unassigned » mondrake

On this.

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Active » Needs review
FileSize
60.49 KB

Removed usages, removed depraction silencers, added a deprecation test.

Status: Needs review » Needs work

The last submitted patch, 6: 3139433-6.patch, failed testing. View results

mondrake’s picture

Assigned: Unassigned » mondrake

On it

mondrake’s picture

Assigned: mondrake » Unassigned
Status: Needs work » Needs review
FileSize
56.25 KB
5.45 KB
Hardik_Patel_12’s picture

Patch failed to apply , kindly review a new patch.

jungle’s picture

Status: Needs review » Needs work

Thanks @Hardik_Patel_12!

+++ b/core/modules/block/tests/src/Functional/BlockTest.php
@@ -283,9 +283,9 @@ public function testThemeName() {
-    $this->assertEscaped('<"Cat" & \'Mouse\'>');
+    $this->assertSession()->assertEscaped('<"Cat" & \'Mouse\'>');
...
-    $this->assertEscaped('Demonstrate block regions (<"Cat" & \'Mouse\'>)');
+    $this->assertSession()->assertEscaped('Demonstrate block regions (<"Cat" & \'Mouse\'>)');

+++ b/core/modules/block/tests/src/Functional/BlockXssTest.php
@@ -125,14 +125,14 @@ protected function doViewTest() {
+    $this->assertSession()->assertNoEscaped('<script>alert("view1");</script>:');
+    $this->assertSession()->assertEscaped('<script>alert("view2");</script>:');
...
+    $this->assertSession()->assertEscaped('<script>alert("view1");</script>');
...
+    $this->assertSession()->assertEscaped('<script>alert("view2");</script>: Fish & chips');

+++ b/core/modules/help/tests/src/Functional/HelpTest.php
@@ -141,16 +141,16 @@ protected function verifyHelp($response = 200) {
         foreach ($admin_tasks as $task) {
...
+          $this->assertSession()->assertNoEscaped('&amp;');
+          $this->assertSession()->assertNoEscaped('&lt;');
...
+          $this->assertSession()->assertNoEscaped('<');
...
+        $this->assertSession()->assertNoEscaped('&amp;');
+        $this->assertSession()->assertNoEscaped('&lt;');
...
+        $this->assertSession()->assertNoEscaped('<');

It‘s worthy to do micro-optimizations for changes which similar to the above. See the code blew from \Drupal\FunctionalTests\AssertLegacyTrait::assertSession().

 /**
   * Returns WebAssert object.
   *
   * @param string $name
   *   (optional) Name of the session. Defaults to the active session.
   *
   * @return \Drupal\Tests\WebAssert
   *   A new web-assert option for asserting the presence of elements with.
   */
  abstract public function assertSession($name = NULL);

we should avoid calling $this->assertSession() multiple times in a test. Each call returns a new instance, it's not good. Instead, saving it as a local variable, for example, $assert_session = $this->assertSession(); and using $assert_session for next one(s).

nitesh624’s picture

Assigned: Unassigned » nitesh624
jungle’s picture

Status: Needs work » Needs review

Setting back to NR, it's not worthy to do such micro-optimizations or undecided. See discussions in the parent issue for more.

mohrerao’s picture

Rerolled as patch failed to apply

nitesh624’s picture

Assigned: nitesh624 » Unassigned

Unassigning as I see others progressing on this.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
55.98 KB
3.31 KB

Rerolled, no more calls present in test code

dww’s picture

Status: Reviewed & tested by the community » Needs review

As with #3139408-14: Replace usages of AssertLegacyTrait::assert(No)Field, that is deprecated, what's up with:

core/tests/Drupal/KernelTests/AssertContentTrait.php defining non-deprecated versions of assert(No)?Escaped()?

That seems highly problematic. Not sure where that's being addressed, but I don't think this is RTBC without reckoning with that.

Sorry,
-Derek

mondrake’s picture

Status: Needs review » Reviewed & tested by the community
dww’s picture

Thanks for clarifying! +1 to RTBC then. I read through the patch and it's just the fix to remove the deprecation from the skip list, the new @legacy deprecation test, and the results of a script. ;)

Thanks,
-Derek

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 7628e38 and pushed to 9.1.x. Thanks!

  • catch committed 7628e38 on 9.1.x
    Issue #3139433 by mondrake, Hardik_Patel_12, mohrerao, jungle: Replace...

Status: Fixed » Closed (fixed)

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