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::assertFieldById() is deprecated in drupal:8.2.0 and is removed from drupal:10.0.0. Use $this->assertSession()->fieldExists() or $this->assertSession()->buttonExists() or $this->assertSession()->fieldValueEquals() 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 |
---|---|---|---|
#77 | interdiff_3139407_73-77.txt | 788 bytes | ankithashetty |
#77 | 3139407-77.patch | 51.16 KB | ankithashetty |
Comments
Comment #2
mondrakeComment #3
jungleAdding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions
Comment #4
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #5
munish.kumar CreditAttribution: munish.kumar as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #7
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #8
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #9
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #11
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #13
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #15
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedHI please review this patch.
Fix failed test cases.
Comment #16
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #17
daffie CreditAttribution: daffie commentedWhen I do a code base search for
$this->assertFieldById(
and$this->assertNoFieldById(
I still get results. More code changes are needed.Comment #18
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #19
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commented@daffie Thanks
RE #17 has been done can you please review this patch.
Comment #21
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #22
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #24
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedWorking on failed tests
Comment #25
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedNeed to update test case.
Comment #26
shaktikComment #27
shaktikComment #28
shaktikComment #30
shaktikComment #31
pavnish CreditAttribution: pavnish at Srijan | A Material+ Company for Drupal India Association commentedComment #32
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedDid a quick review and found major issues on previous patches.
Comment #33
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed PHP lint failure.
Comment #35
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed Failing tests.
Comment #36
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 commentedPatch failed to apply , kindly follow a new patch.
Comment #38
shaktikHi @Hardik_Patel_12,
Could you please add interdiff its drupal ci say the patch is pass and I am also not able to apply the #35 attached file failer report.
Thanks,
Shakti.
Comment #39
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedComment #40
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedPlease review the following patch.
Thanks
Comment #42
hash6 CreditAttribution: hash6 at QED42 for Drupal India Association commentedComment #43
shaktikHi @hash6,
Just reroll patch from #35
Comment #44
rahulrasgon CreditAttribution: rahulrasgon at QED42 commented@shaktik after applying #43 when I do a code base search for $this->assertFieldById( and $this->assertNoFieldById( I still get results. More code changes are needed.
File:
core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php
still get results with $this->assertFieldById( and $this->assertNoFieldById( .Comment #45
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedWorking on failed test cases on #40.
Comment #46
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedPlease review the patch.
Thanks
Comment #48
durgeshs CreditAttribution: durgeshs as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #49
durgeshs CreditAttribution: durgeshs as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedPlease review this patch.
Thanks,
Comment #50
mondrakeNeed a deprecation test for the deprecated methods, and the deprecation silencers removed from the
DeprecationListenerTrait
.All these seem to be checking for the field value too. not just for the field existence:
Comment #51
durgeshs CreditAttribution: durgeshs as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #52
durgeshs CreditAttribution: durgeshs as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedHi @mondrake
I have made changes as per #50, kindly review it.
Comment #53
jungle#50 hasn't been fully addressed.
Comment #54
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRemoving deprecation silencers removed from the DeprecationListenerTrait.
Comment #55
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedThe failure is in testFieldAssertsForButton from core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php.
Updated testFieldAssertsForButton method from core/tests/Drupal/FunctionalTests/BrowserTestBaseTest.php with new assertions.
Comment #56
jungleThanks @mohrerao!
Comment #57
junglePostpone on #3139406: Replace usages of AssertLegacyTrait::assert(No)FieldByName, that is deprecated, Some usages in BrowserTestBase could be removed due to the replacement is the same, and overlapped.
Comment #58
mondrakeRerolled and added deprecation test.
Comment #60
mondrakeFixes.
Comment #61
longwavefieldValueEquals() will throw an exception anyway if the field doesn't exist.
Same
Same
Same
Same
Same
We don't need the second argument to fieldExists() here.
We don't need to check existence separately.
We don't need the second argument.
Same
We don't need to check existence separately.
The default value of '' passed to assertFieldById() also checks the value is an empty string, not just existence.
Same
We don't need to check for existence separately.
Same
The second argument is unnecessary.
Where did 'name[0][value]' come from?
Shouldn't this be fieldNotExists()?
We don't need a second argument to fieldExists()
We don't need a second argument to buttonExists()
Same
Missing the third assertion here
We don't need the second argument again
Same
Comment #62
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedWorking on addressing comment #61.
Comment #63
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have tried to address points 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23 and 24 of comment #61.
Comment #66
shetpooja04 CreditAttribution: shetpooja04 at QED42 commentedI have addressed 1 to 6 points suggested in comment #61.
Please review
Comment #67
shetpooja04 CreditAttribution: shetpooja04 at QED42 commentedRerolled the patch in #66
Comment #69
anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commentedComment #70
paulocsHello,
Patch #70 looks good to me. I didn't find no AssertLegacyTrait::assert(No)FieldById any more.
Moving to RTBC.
Cheers, Paulo.
Comment #71
catchLooks good but needs a re-roll.
Comment #72
paulocsComment #73
paulocsPatch re-rolled.
Comment #74
longwaveComment #75
catchOne more issue:
this is failing cspell due to 'dont', probably need to change to i-do-not-exist and I do not exist or I don't exist.
Comment #76
catchComment #77
ankithashettyUpdated the patch in #73 with the minor corrections mentioned in #75... Kindly review.
Thanks!
Comment #78
mondrake#77 addresses #75. RTBC
Is there any plan to have cspell checks as part of DrupalCI test runs?
Comment #80
catchCommitted 71813a8 and pushed to 9.1.x. Thanks!