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::assertLink() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->linkExists() 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 |
---|---|---|---|
#41 | 3139414-interdiff-40-41.txt | 1.25 KB | cburschka |
#41 | 3139414-41-9.0.x.patch | 3.33 KB | cburschka |
#31 | 3139414-31.patch | 120.19 KB | cburschka |
Comments
Comment #2
jungleAdding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions
Comment #3
jungleComment #5
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #6
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commented@jungle The $this->assertSession() is not a part of use Drupal\KernelTests\KernelTestBase;
This is the part of use Drupal\Tests\BrowserTestBase;
Please suggest what we can do?
Comment #7
mondrake#6 just revert from #3 all changes that were made to Kernel tests.
Comment #8
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commented@mondrake thanks for suggesting me
I am reverting the kernel test changes
Thanks
Comment #9
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedHi @mondrake
Changes has been done it would be a great help for me if you can review this patch.
Thanks
Pavnish
Comment #10
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #11
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commented1. All occurrences of
$this->assert(No)Link(XXX)
have been replaced by$this->assertSession()->linkExists(XXX)
except from Kernel tests,to be specific from
core/modules/comment/tests/src/Kernel/Views/CommentUserNameTest.php
andcore/modules/field/tests/src/Kernel/String/StringFormatterTest.php
core/tests/Drupal/Tests/Core/Utility/LinkGeneratorTest.php
contain its own$this->assertLink()
method.This is also not changed.
2. The test has been added for deprecation testing.
3. The suppression of the deprecation message has been removed.
All code changes look good to me.
For me it is RTBC.
Comment #12
xjmComment #13
xjmOops, looks like it needs a reroll already. Thanks!
Comment #14
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #15
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #16
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolling patch
Comment #17
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed php lint errors.
Comment #18
daffie CreditAttribution: daffie commentedThe patch needs to be rerolled again.
Comment #19
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedAdded re-roll of patch #17.
Comment #20
daffie CreditAttribution: daffie commentedThere a few thing that need to be done in the patch:
core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php
the 2 lines supressing the deprecationg messages for AssertLegacyTrait::assertLink() and AssertLegacyTrait::assertNoLink() need to be removed.$this->assertLink()
and$this->assertNoLink()
need to be replaced. There a few of those in the patch, only there are a lot more of them.Comment #21
cburschkaCareful - this applies to calls to
AssertLegacyTrait::assertLink()
, which are in functional tests. There is alsoAssertContentTrait::assertLink()
which is used in kernel tests and which isn't marked as deprecated. Fortunately, kernel tests are all in namespaces/filepaths containing "Kernel" and can be excluded that way.Replacing all occurrences in files whose paths don't contain "Kernel" of
->assertLink(
with->assertSession()->linkExists(
and->assertNoLink(
with->assertSession()->linkNotExists(
, and removing the two suppressions.RandomGeneratorTrait::randomStringValidate()
has an inline comment that still mentionsWebTestBase::assertLink()
, but that class doesn't even exist anymore, and it's not clear to me that the problem it mentions also exists inWebAssert::linkExists()
. Not sure what to do with that one.Comment #22
cburschkaMissed the LinkGeneratorTest special case that was mentioned further upthread. Those changes need to be reverted.
Comment #23
daffie CreditAttribution: daffie commentedCreated an issue for that one. See: #3144331: Update comment in Drupal\Tests\RandomGeneratorTrait::randomStringValidate().
Comment #24
daffie CreditAttribution: daffie commented@cburschka: You are right about that.
Most code changes are right, only there are a few that are wrong:
Comment #25
cburschkaOops, I messed up my replacement pattern there apparently :)
Comment #26
cburschkaComment #27
daffie CreditAttribution: daffie commentedThe deprecation message suppression for
AssertLegacyTrait::assert(No)Link()
have been removed.All occurrences of
$this->assert(No)Link()
have been replaced by$this->assertSession()->link(Not)Exists()
.All changes look good to me.
The testbot is happy.
For me it is RTBC.
Comment #28
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #29
alexpottNeeds a reroll. Unfortunately.
Comment #30
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #31
cburschkaBrushed up too close to a change in #3142752: AssertLegacyTrait::assert(No)Escaped() in functional tests still have a message passed in; merged.
Comment #33
jungleLooks like a random failure, re-queued
Comment #34
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedThe deprecation message suppression for AssertLegacyTrait::assert(No)Link() have been removed.
All occurrences of $this->assert(No)Link() have been replaced by $this->assertSession()->link(Not)Exists().
All changes look good to me.
For me it is RTBC.
Comment #35
alexpottCommitted 0f9cecb and pushed to 9.1.x. Thanks!
Backported 12fb3d8 and pushed to 9.0.x, and 8130d88 and pushed to 8.9.x. Thanks!
I did this by ignoring these conflicts:
error: patch failed: core/modules/tracker/tests/src/Functional/TrackerTest.php:208
error: core/modules/tracker/tests/src/Functional/TrackerTest.php: patch does not apply
error: patch failed: core/modules/user/tests/src/Functional/UserPasswordResetTest.php:109
error: core/modules/user/tests/src/Functional/UserPasswordResetTest.php: patch does not apply
error: patch failed: core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php:185
error: core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php: patch does not apply
So we're probably still using these methods in 9.0.x and 8.9.x but the branches are largely the same.
We can now prepare a backport patch for core/modules/tracker/tests/src/Functional/TrackerTest.php and core/modules/user/tests/src/Functional/UserPasswordResetTest.php
Comment #39
cburschkaThe block of skipped AssertLegacyTrait deprecations in DeprecationListenerTrait (of which this patch removes two lines) seems to be entirely absent in 9.0.x.
It appears that in 9.0.x the AssertLegacyTrait methods are "fake"-deprecated (no @deprecated and no @trigger_error), so I guess that's why we don't need to listen for these deprecations.
One thing I noticed was that a lot of these assertions have a $message argument which they didn't remove, even though the new method doesn't support it (see also #3143398: WebAssert methods do not have a $message argument like AssertLegacyTrait methods), so removing that may be a follow-up task.
Comment #40
cburschkaOops - I accidentally diffed against 9.1.x instead of 9.0.x and created a monster patch
Comment #41
cburschkaRemoving two assertTitle -> assertSession()->titleEquals changes that should not be in this patch.
Comment #42
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedVerified backport patch for
core/modules/tracker/tests/src/Functional/TrackerTest.php
andcore/modules/user/tests/src/Functional/UserPasswordResetTest.php
All occurrences of $this->assert(No)Link() have been replaced by $this->assertSession()->link(Not)Exists() in these two files.
All changes look good to me.
For me it is RTBC.
Comment #43
mondrakeFiled follow-up.
Comment #44
mondrakeNot sure what to do here - the patch went in up to 8.9.x, I doubt a 8.8.x backport is an option now. Comments from #39 onwards are OOS.
Tentatively marking fixed.
Comment #45
mondrakeAh, no, I did not read #35 to the end: we still need a backport for
Latest patches were going in a different direction.
Comment #46
mondrake...and, that is what #41 is actually doing :)
Sorry for the noise.
Comment #47
mondrakeComment #48
alexpottIn future can #40 be filed as a separate follow-up issue. Multiple commits on the same issue are not good AND we already have follow-up issues committed so this just confuses people.
Committed 82979c9 and pushed to 9.0.x and 8.9.x. Thanks!
Backported to 8.9.x as this is a test only fix.