Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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
Comment | File | Size | Author |
---|---|---|---|
#36 | interdiff-33-34-9.0.x.txt | 1.74 KB | Hardik_Patel_12 |
#36 | interdiff-33-34-8.9.x.txt | 2.95 KB | Hardik_Patel_12 |
#34 | 3139412-34-9.0.patch | 52.52 KB | Hardik_Patel_12 |
#34 | 3139412-34-8.9.patch | 54.01 KB | Hardik_Patel_12 |
#27 | 3139412-D89-27.patch | 50.69 KB | Bunty Badgujar |
Comments
Comment #2
jungleAdding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions
Comment #3
longwaveComment #4
longwaveOops, left some debug code in #2.
Comment #6
Bunty Badgujar CreditAttribution: Bunty Badgujar commentedPatch doesn't apply on current 9.1.x because one changes already added in [#3139439]
Comment #7
Bunty Badgujar CreditAttribution: Bunty Badgujar commentedComment #8
Bunty Badgujar CreditAttribution: Bunty Badgujar commentedTest case fixed and revert code suggest in #6
Comment #9
mondrakeThanks. It needs
1. a deprecation test to demonstrate that a call to
assertTitle
will lead to a depreaction error being triggered, and2. 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).
Comment #10
longwaveI added a deprecation test in #4, I don't know why it was removed in #8?
Comment #11
Bunty Badgujar CreditAttribution: Bunty Badgujar commented@mondrake and @longwave thanks for response. Deprecation test for demonstrate has been added as suggested #9 and #10.
Comment #12
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedComment #13
mondrakeTo 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.
Comment #14
xjmComment #15
xjmGood 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, beforetitleEquals()
runs. And many of the assertions in the patch already do just that.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 unnecessaryt()
fromassertTitle()
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?
Comment #16
mondrake#15 yes it does.
Filed #3143339: Clean up the arguments of calls to WebAssert::titleEquals() and AssertLegacyTrait::assertTitle().
Comment #17
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedYes, there is one place in #11. This need to be fix.
Previously in
core/tests/Drupal/FunctionalTests/AssertLegacyTrait::assertTitle
assertTitle($expected_title)
is doing string cast even if you provide string or markup object.So, Should we need to update
core/tests/Drupal/Tests/WebAssert::titleEquals
with string cast ?Comment #18
mondrake@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.
Comment #19
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedIssue #3143339: Cleanup the arguments of calls to WebAssert::titleEquals and AssertLegacyTrait::assertTitle fixed. So moving back to need work.
Comment #20
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedRe-roll #11 after cleanup done in #18.
Comment #21
mondrake#15-#16 addressed, so back to RTBC. This is now just string changes for the method calls + deprecation test + removal of the silencer.
Comment #22
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedComment #24
xjmThanks! 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.
Comment #25
xjmComment #26
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedComment #27
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedAdding patch for 8.8.x,8.9.x and 9.0.x
Comment #28
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedWrong patch uploaded for 8.8.x in #27.
Comment #29
longwaveI 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.
Comment #30
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedComment #32
xjmLooks 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!
Comment #33
longwaveRerolled, same patch works for me on 8.9.x and 9.0.x.
Comment #34
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedThere are still $this->assertTitle() call left , removing that calls. Kindly review the patch.
Comment #35
longwave@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.
Comment #36
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented@longwave , kindly review the interdiffs.
Comment #37
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commented@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.
Comment #39
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedKindly review the patch #33.
Comment #40
mondrakeMarking fixed on the basis of #3027952-54: [Plan] Remove the usage of deprecated methods in tests. Please open a separate issue for backport.
Comment #41
longwave