Problem/Motivation

AssertLegacyTrait::assertTitle() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->titleEquals() 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

longwave’s picture

Status: Active » Needs review
FileSize
58.79 KB
longwave’s picture

FileSize
58.19 KB

Oops, left some debug code in #2.

Status: Needs review » Needs work

The last submitted patch, 4: 3139412-3.patch, failed testing. View results

Bunty Badgujar’s picture

Patch doesn't apply on current 9.1.x because one changes already added in [#3139439]

+  /**
+   * Tests legacy assertHeader().
+   *
+   * @group legacy
+   * @expectedDeprecation 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
+   */
+  public function testAssertHeader() {
+    $account = $this->drupalCreateUser();
+    $this->drupalLogin($account);
+    $this->drupalGet('test-page');
+    $this->assertHeader('X-Drupal-Cache-Tags', 'http_response rendered');
+  }
+
Bunty Badgujar’s picture

Assigned: Unassigned » Bunty Badgujar
Bunty Badgujar’s picture

Assigned: Bunty Badgujar » Unassigned
Status: Needs work » Needs review
FileSize
24.09 KB
57.63 KB

Test case fixed and revert code suggest in #6

mondrake’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests, +Needs followup

Thanks. It needs

1. a deprecation test to demonstrate that a call to assertTitle will lead to a depreaction error being triggered, and
2. a followup to cleanup the arguments of titleEquals: change in 1st argument so that a string is passed directly (instead of casting to string a call to t() or a FormattableMarkup object, and no more arguments are passed in (2nd being a $message and 3rd being a $group).

longwave’s picture

I added a deprecation test in #4, I don't know why it was removed in #8?

Bunty Badgujar’s picture

Status: Needs work » Needs review
FileSize
738 bytes
58.48 KB

@mondrake and @longwave thanks for response. Deprecation test for demonstrate has been added as suggested #9 and #10.

Bunty Badgujar’s picture

mondrake’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

To me, this looks good to me now.

Just it's not clear for me at this point whether we can postpone #9.2, or else we should do that BEFORE this issue, in which case this issue would have to be postponed.

Raising the issue to the right eyes, so that we can get a piece of feedback that would inform other issues in this thread, too.

xjm’s picture

Title: Replace usages of AssertLegacyTrait::assertTitle, that is deprecated » Replace usages of deprecated AssertLegacyTrait::assertTitle()
xjm’s picture

Status: Reviewed & tested by the community » Needs review

Good questions on #9.2.

I've an observation about the string casts. There is one place in core already that does:
$this->assertSession()->titleEquals(t('Roles') . ' | Drupal')

I do see there are test failures related to markup objects in #4, but I think we are over-applying the string cast as a fix. Concatenating a markup object (including a t()) should automatically cast it to a string anyway, before titleEquals() runs. And many of the assertions in the patch already do just that.

+++ b/core/modules/block_content/tests/src/Functional/BlockContentTypeTest.php
@@ -121,7 +121,7 @@ public function testBlockContentTypeEditing() {
-    $this->assertTitle(new FormattableMarkup('Edit @type custom block type | Drupal', ['@type' => 'basic']));
+    $this->assertSession()->titleEquals((string) new FormattableMarkup('Edit @type custom block type | Drupal', ['@type' => 'basic']));

The above is the first assertion that actually seems to have failed in the test results from #4, but we are changing many more places before that. This one failed because there is no concatenation that automatically performs the string cast.

So I think the answer to whether #9.2 should be blocker or followup is a bit flexible, but I personally would prefer to remove the t() first in a separate issue. The scope of "Remove unnecessary t() from assertTitle() calls" would eliminate the need for the string casts almost completely, and then this issue would become pure regex/PHPStorm refactor/your tool of choice once that's in, so the two steps together shouldn't be that much more work. And I would prefer to get the right pattern out there in core first, rather than perpetuating the bad pattern with workarounds.

Does that work?

mondrake’s picture

Bunty Badgujar’s picture

Status: Postponed » Needs review

I do see there are test failures related to markup objects in #4, but I think we are over-applying the string cast as a fix. Concatenating a markup object (including a t()) should automatically cast it to a string anyway, before titleEquals() runs. And many of the assertions in the patch already do just that.

Yes, there is one place in #11. This need to be fix.

The above is the first assertion that actually seems to have failed in the test results from #4, but we are changing many more places before that. This one failed because there is no concatenation that automatically performs the string cast.

Previously in core/tests/Drupal/FunctionalTests/AssertLegacyTrait::assertTitle

assertTitle($expected_title) is doing string cast even if you provide string or markup object.

/**
   * Pass if the page title is the given string.
   *
   * @param string $expected_title
   *   The string the page title should be.
   *
   * @deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use
   *   $this->assertSession()->titleEquals() instead.
   *
   * @see https://www.drupal.org/node/3129738
   */
  protected function assertTitle($expected_title) {
    @trigger_error('AssertLegacyTrait::assertTitle() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->titleEquals() instead. See https://www.drupal.org/node/3129738', E_USER_DEPRECATED);
    // Cast MarkupInterface to string.
    $expected_title = (string) $expected_title;
    return $this->assertSession()->titleEquals($expected_title);
  }

So, Should we need to update core/tests/Drupal/Tests/WebAssert::titleEquals with string cast ?

mondrake’s picture

Status: Needs review » Postponed

@Bunty Badgujar let's do #3143339: Clean up the arguments of calls to WebAssert::titleEquals() and AssertLegacyTrait::assertTitle() first, then this can be rerolled and the discussion will no longer be needed.

Bunty Badgujar’s picture

Assigned: Unassigned » Bunty Badgujar
Status: Postponed » Needs work
Bunty Badgujar’s picture

Status: Needs work » Needs review
FileSize
53.35 KB

Re-roll #11 after cleanup done in #18.

mondrake’s picture

Status: Needs review » Reviewed & tested by the community

#15-#16 addressed, so back to RTBC. This is now just string changes for the method calls + deprecation test + removal of the silencer.

Bunty Badgujar’s picture

Assigned: Bunty Badgujar » Unassigned

  • xjm committed 8837525 on 9.1.x
    Issue #3139412 by Bunty Badgujar, longwave, mondrake, xjm: Replace...
xjm’s picture

Version: 9.1.x-dev » 8.8.x-dev

Thanks! Committed to 9.1.x.

PTBP for a backport for 9.0.x and earlier that does not un-silence the deprecation. Note that 8.8.x only has a few more days of non-security commits, so we might run out of time to backport it there.

xjm’s picture

Title: Replace usages of deprecated AssertLegacyTrait::assertTitle() » [backport] Replace usages of deprecated AssertLegacyTrait::assertTitle()
Status: Reviewed & tested by the community » Patch (to be ported)
Bunty Badgujar’s picture

Assigned: Unassigned » Bunty Badgujar
Bunty Badgujar’s picture

Adding patch for 8.8.x,8.9.x and 9.0.x

Bunty Badgujar’s picture

Wrong patch uploaded for 8.8.x in #27.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

I visually compared the four patches for the different branches. I didn't see any differences in the 9.0 and 8.9 patches so they are good to go.

8.8 is also good in this regard, but core committers might want to wait for both #3132964: assertResponse() does not actually support a $message parameter, so stop passing one and #3139218: Replace usages of AssertLegacyTrait::assertResponse(), which is deprecated to get in first - these touch some of the nearby lines and hence will need reroll. If those get in first then the 8.9 patch here should be fairly clean to drop straight into 8.8 as far as I can see.

Bunty Badgujar’s picture

Assigned: Bunty Badgujar » Unassigned

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

Bug reports should be targeted against the 8.9.x-dev branch from now on, and new development or disruptive changes should be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

xjm’s picture

Status: Reviewed & tested by the community » Needs work

Looks like we need a reroll of the 9.0.x and 8.9.x patches. (We don't need an 8.8.x patch anymore, since that branch is now in security support only.) Thanks!

longwave’s picture

Status: Needs work » Needs review
FileSize
50.63 KB

Rerolled, same patch works for me on 8.9.x and 9.0.x.

Hardik_Patel_12’s picture

There are still $this->assertTitle() call left , removing that calls. Kindly review the patch.

longwave’s picture

@Hardik_Patel_12 please provide interdiffs so we can see what you changed.

Also this is supposed to be a backport, we don't need to change all usages in 8.9.x and 9.0.x, just keep the branches in sync.

Hardik_Patel_12’s picture

@longwave , kindly review the interdiffs.

Hardik_Patel_12’s picture

@longwave , thanks for clearing my doubt on slack for #35 . In this case patch at #33 is valid and patch at #34 is doing some extra work in 9.0/8.9 so we can ignore this patch . Will keep in mind this in future while working on backport issues.

Status: Needs review » Needs work

The last submitted patch, 34: 3139412-34-9.0.patch, failed testing. View results

Hardik_Patel_12’s picture

Status: Needs work » Needs review

Kindly review the patch #33.

mondrake’s picture

Title: [backport] Replace usages of deprecated AssertLegacyTrait::assertTitle() » Replace usages of deprecated AssertLegacyTrait::assertTitle()
Status: Needs review » Fixed

Marking fixed on the basis of #3027952-54: [Plan] Remove the usage of deprecated methods in tests. Please open a separate issue for backport.

longwave’s picture

Version: 8.9.x-dev » 9.1.x-dev

Status: Fixed » Closed (fixed)

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