Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
AssertLegacyTrait::assertResponse() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->statusCodeEquals() instead. See https://www.drupal.org/node/3129738
Proposed resolution
Replace usages of AssertLegacyTrait::assertResponse().
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#41 | 3139218-41-8.8.x.patch | 417.01 KB | sja112 |
#31 | 3139218-31-9.0.x-and-8.9.x.patch | 389.85 KB | sja112 |
#26 | interdiff-3139218-26-22.txt | 3.27 KB | sja112 |
#26 | 3139218-26-8.8.x.patch | 416.57 KB | sja112 |
#10 | 3139218-10.patch | 393.25 KB | mondrake |
Comments
Comment #2
mondrakeComment #3
mondrakeTest only patch only removes the depracation silencer. Patch changes the usages and adds a depreaction test in BrowserTestBase.
Comment #4
mondrakeComment #6
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commented@mondrake,
I have reviewed the patch and found one change which might lead to patch re-roll in the future if the other issue (#3131900) gets resolved before this.
Since another issue (Patch: #3131900.15) is already RTBC'd. You should avoid making changes in the method
assertPagePath
.Comment #7
mondrakeHi @sja112, it's normally not a problem to have patches on the same piece of code running in parallel. Actually, it's quite the normality. If #3131900: Refactor assertions that assign return values to variables goes in first, this will have to be rerolled; if this goes in first, #3131900: Refactor assertions that assign return values to variables will need a reroll. Postponing a patch on another is normally related to more complex situations to be addressed before continuing, but IMHO it's not the case here.
Failure in #3 is due to UncaughtExceptionTest having its own
assertResponse
method.Comment #8
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commented@mondrake,
I notice this same piece of change in other patch as well. So just pointed it out. Agreeing to your point here we need a re-roll any ways.
I will review the patch #7 after completion of testing job.
Thanks!
Comment #10
mondrakeComment #11
daffie CreditAttribution: daffie commentedAll occurrences of
$this->assertResponse(XXX);
have been replaced by$this->assertSession()->statusCodeEquals(XXX);
.The method was already deprecated in 8.2.
Test has been added for deprecation testing.
The supression of the deprecation message has been removed.
The method
Drupal\FunctionalTests\Bootstrap\UncaughtExceptionTest::assertResponse()
has been given a proper docblock instead of the @inheritdoc it had.All code changes look good to me.
For me it is RTBC.
Comment #12
jungleAdding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions
Comment #13
xjmComment #15
xjmThanks for this. I confirmed with
git diff --color-words
that, with one exception, the change is 1:1 replacements of the method call, and that the only grep occurrences ofassertResponse(
outside the legacy code are inUncaughtExceptionTest
.Even though
UncaughtExceptionTest
caused a problem here, we should have filed a separate issue to address that. Especially with a patch of this size, it should be an entirely scannable set of one exact pattern of change withgit diff --color-words
. See https://www.drupal.org/core/scope#context. I almost marked the issue NW for this.I think it would have been better to file an issue for
UncaughtExceptionTest
first to actually rename its methods to avoid this confusion. As it is, let's do that as a followup.Let's make backport patches that replace the method calls but do not un-silence the deprecation. Thank you!
Comment #16
mondrake@xjm re #15, I am not sure about the followup for
UncaughtExceptionTest
. In fact thereassertResponse
is a helper to the test class itself that overrides the trait implementation. Other methods in this test class fall under the same concept, first and foremostdrupalGet
- they seem to override mink and use cUrl for the calls to the webserver. In other words they have little to do with this issue - which is about removing the deprecated assertResponse methods that we inherited from Simpletest, using WebAssert assertions instead. If we need to rename those methods than probably a general rethinking of that test is needed, but that would IMHO deserve a full fat issue on its own (i.e. to decouple from BrowserTestBase, probably). If not, than I do not see an issue to have a protected assertResponse method in a concrete test class, it would live its own life.Comment #17
xjm@mondrake My point is mostly that the added docs should not be included in this issue. :) So I would have filed an issue when I noticed the test fail, something like:
Edit: That doesn't affect the backport; backports are backports. Just a general practice that's increasingly important as we do more of this work.
Comment #18
mondrakeFiled #3142549: Document UncaughtExceptionTest overrides of AssertLegacyTrait methods and decide whether to rename them
Comment #19
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedOn this.
Comment #20
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedBackported patch for 8.8.x branch.
Comment #21
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedComment #22
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedRe-uploading patch caught one error while self-testing. Changes done in the wrong file.
Comment #25
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedComment #26
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedChanged patch to fix failed tests.
SimpleTestBrowserTest extends WebTestBase
which have its ownassertResponse
method.Comment #27
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedFiled #3142906: simpletest WebTestBase overrides of AssertLegacyTrait methods and decide whether to rename them
Comment #28
xjmThanks @sja112. Given the size of this patch, I would recommend backporting it one branch at a time. The version here is just to show which branches allow the change. It does not mean creating a patch that only works for 8.8.x when it has only been committed to 9.1.x. You can test the patch against a different branch than the version selector by selecting the correct version next to the upload button when you submit the patch. Edit: Also, you should just try to apply the patch to the relevant branches locally before uploading. :)
NW to start with a working 9.0.x patch.
Comment #29
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedComment #30
xjmOh and adding the reminder to the title that it's a backport.
Comment #31
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedPatch for D9.0 and D8.9
Comment #32
ketikagrover CreditAttribution: ketikagrover commentedComment #33
ketikagrover CreditAttribution: ketikagrover commented1. As per #17 need follow up issue has been created in #18 for uncaughtExceptionTest::assertResponse()
2. Backport patches created for D8.8, D8.9, and D9.0
3. Checked all three branches for any occurrence of $this->assertResponse() and all are replaced by $this->assertSession()->statusCodeEquals()
All code changes look good to me.
For me it is RTBC.
Comment #34
ketikagrover CreditAttribution: ketikagrover commentedComment #35
ketikagrover CreditAttribution: ketikagrover commentedComment #38
xjmThanks @sja112 and @ketikagrover. I reviewed locally with
git diff --color-words
(which was very hypnotic; I almost dozed off a couple times!) and also checked 9.0.x for other usages:That's the same test again (with the followup) plus the legacy method.
I cherry-picked it to 8.9.x as well. 8.9.x has a number of other matches:
At least some of those are SimpleTests which should not be changed, but not all of them. That said, the main goal of backporting this is to keep the branches in sync so that other cherry-picks are successful. Therefore, there's not too much value in fixing additional cases that are D8-only, so I pushed the 8.9.x cherry-pick to avoid anything going stale there.
I'll review 8.8.x. in a bit. Thanks!
Comment #39
xjm#26 does not apply anymore, so NW for that to be rerolled.
Comment #40
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedComment #41
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedRe-rolled patch for the D8.8 branch.
Comment #42
ketikagrover CreditAttribution: ketikagrover commented1. Patch re-rolled for D8.8 branch
2. One change I observed in this patch that some inline comments are added below is the example,
After debugging I noticed that changes are already present D8.9 and above branches.
as we are keeping all the branches the same and its good to have the appropriate information with the code this change is valid in my view.
Patch looks good to me.
Thanks!
Comment #43
xjmThanks @ketikagrover for describing how you reviewed this; that made me realize something that was affecting the 8.8.x backport. Example:
This is not being fixed correctly as it was in #3132964: assertResponse() does not actually support a $message parameter, so stop passing one.
I think what we should do is work on a backport of that patch first, and get that committed to 8.8.x. Then, we can reroll @sja112's patch on top of that. It should mostly be a clean rebase since @sja112's fixes have followed the same patterns, but a few hunks might need to be fixed manually. Then, this patch will be easier to review and commit as well. Thanks!
Comment #44
xjmNote that the final commit freeze for 8.8.x is on June 2 at 1200 UTC, so if it gets too close to that, there's a chance we might not be able to commit the backport. That said, the review is easier when all the comment changes are happening in the separate issue, so that should help.
Comment #45
xjmComment #47
mondrakeI suppose this is no longer committable to D8.8.