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

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
StatusFileSize
new60.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
StatusFileSize
new56.25 KB
new5.45 KB
hardik_patel_12’s picture

StatusFileSize
new56.26 KB

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

StatusFileSize
new56.26 KB

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
StatusFileSize
new55.98 KB
new3.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.