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::assertFieldByName() 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 |
---|---|---|---|
#79 | rawdiff_78-79.txt | 2.47 KB | mondrake |
#79 | 3139406-79.patch | 165.26 KB | mondrake |
Comments
Comment #2
jungleAdding the "Deprecated assertions" tag to add this into the kanban board https://contribkanban.com/board/Deprecatedassertions
Comment #3
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedOn it
Comment #4
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedAdding a draft patch. Still needs some work.
Comment #5
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedPlease ignore the previous patch as i generated it against 8.8.x. Adding new patch for the changes.
Comment #6
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedApologies for all the noise. Please ignore 3139406-4.patch and 139406-5.patch.
Comment #8
ridhimaabrol24 CreditAttribution: ridhimaabrol24 at Srijan | A Material+ Company for Drupal India Association commentedComment #9
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@ridhima, Im working on fixing the failed tests.
Comment #10
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedFixed some of the failing tests. Still WIP
Comment #11
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #12
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedStill some of the tests are failing. Still WIP.
Comment #13
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedAdding patch for replacing AssertLegacyTrait::assertNoFieldByName.
Comment #14
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #15
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedNeed re-roll also some test case failure fix from #13.
Description related to update
0 consider as null value.
Original code is
$this->assertNoFieldByName('settings[handler_settings][sort][direction]');
Comment #17
Bunty Badgujar CreditAttribution: Bunty Badgujar at Srijan | A Material+ Company for Drupal India Association commentedThere is open issue in un Mink fieldValueEquals does not support multiple select
Do we need seperate issue for this ?
Comment #18
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedComment #19
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRerolled patch as the patch doesn't apply.
Comment #20
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedPlease review the patch.
Thanks
Comment #23
nitesh624Comment #24
nitesh624Unassigning as I am bit busy in some other issue
Comment #25
naresh_bavaskarComment #26
naresh_bavaskarComment #27
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@rahulrasgon, Please add interdiff after adding a patch. I reworked on p[atch in #19 and have fixed failing tests.
Comment #28
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedComment #29
rahulrasgon CreditAttribution: rahulrasgon at QED42 commentedAll occurrences of $this->assertFieldByName() and $this->assertNotFieldByName() have been replaced.
The deprecation message suppression has been removed.
Added the @expectedDeprecation to the docblock of
testFieldAssertsForTextfields
,testFieldAssertsForButton
andtestFieldAssertsForCheckbox
Please review the patch.
Thanks
Comment #31
narendra.rajwar27Comment #32
narendra.rajwar27Un-assigning, not able to find cause for test case failure.
:(
Comment #33
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedWorking on it.
Comment #34
Hardik_Patel_12 CreditAttribution: Hardik_Patel_12 at QED42 for Drupal India Association commentedKindly review a new patch.
Comment #36
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #37
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #38
mondrakeNeeds reroll, deprecation tests, and completing removal of usages. Also where the $message argument is adding non-obvious context to the test, it should be added as an inline comment instead of removed.
Comment #39
mondrakeOops sorry, the deprecation test is present already.
Comment #40
longwaveRerolled, fixed one more instance, fixed coding standards issues. The remaining instances are the legacy trait, legacy tests, or the identically-named method in the kernel test and trait.
Comment #42
jungleFixing the test failed in #40. Changes made to BrowserTestBaseTest, key changes: 1) removed unnecessary
try catch
statements. 2) replacement/cleanup.Comment #43
jungleForgot attaching patch, sorry.
Comment #44
mondrakewhy?
We should use fieldValueEquals here, no?
Also:
The $message argument is adding non-obvious context to the test, it should be added as an inline comment instead of just being removed.
same
same
same
same
same
same
same
same
same
same
same
same
same
same
same
same
same
same
same
same
same
same
Comment #45
longwaveSo reverting this fails with:
Behat\Mink\Exception\ExpectationException: The field "field_single[0][value]" value is "", but "0" expected.
Drilling into it, I think assertFieldByName() has a subtle bug as it only uses == for comparisons, and 0 == "". I guess the test field has another bug if 0 is not stored/output correctly, but I think that is out of scope to fix here.
Working on the rest.
Comment #46
longwaveAddressed most of #44. I disagree with #44.3, #44.4, #44.16b, #44.16c, #44.18 and #44.19 in that they have enough comments already. I also think #44.15 doesn't need comments either.
Regarding RowUITest I think the fieldExists check is useless as the drupalPostForm immediately afterwards also ensures the field exists, and really we don't care about the default value in this section of the test. I removed the line in question and added a subsequent comment instead.
Comment #47
longwaveComment #48
mondrakeUmm.. I think here we are removing too much - this will leave us only with positive tests that assertion works, but not with the negative tests that assertions end up in exceptions if they fail. Also, please let's avoid using a $assert_session variable and stick to $this->assertSession()->.. for each call. See #3027952-45: [Plan] Remove the usage of deprecated methods in tests and following. The rest is good for me now.
Comment #49
mondrakeRerolled and addressed #48.
Comment #51
longwaveShouldn't we just leave BrowserTestBaseTest alone except for tagging the tests as legacy and expecting deprecations? I don't think we need to test the underlying WebAssert methods, unless we don't trust Mink's own tests?
Comment #52
mondrake#51 yes we should rely on Mink's own tests, but:
(a) we have some additional methods implemented in Drupal\Tests\WebAssert that do not have an explicit coverage (for example, buttonExists), only implicit through testing of the legacy method. If we mark them @legacy they'll be removed in later stage and we'd be left out of coverage.
(b) there are some specific cases tested like
that need their own testing.
IMHO we should have one-to-one matching between the methods in WebAssert and tests in WebAssertTest (functional). I'm trying to add separate deprecation tests marked @legacy as we move along, and change the existing tests to the new WebAssert methods but keeping their structure. Once the merry-go-round of legacy AssertLegacyTrait usage removal is done, we could jump to #3159783: [meta] Add tests for the WebAssert class and properly move whatever is left and relevant in BrowserTestbaseTest to WebAssertTest.
That's my idea though - happy to see alternatives.
Fixing errors in #49.
Comment #54
mondrakeComment #56
mondrakeHopefully
Comment #57
mondrakeComment #58
paulocsComment #59
paulocsPatch re-rolled.
Comment #60
mondrakeComment #61
longwaveI reviewed the entire patch with
git diff --color-words
and it is looking almost there, I don't see any issues except one.I don't see the point of the second assertion. The underlying code calls
preg_quote()
on the value and so both assertions are ultimately checking for an empty string aspreg_quote(NULL)
is also''
.Comment #62
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedPlease review!
Comment #63
mondrake#61 that's exactly the point of the test (this is the same in the removed hunk), I think... so that if the underlying code is for any reason changed, still its facing will have to treat NULL and empty string as the same.
Comment #64
longwaveRe #63,
assertFieldByName()
was subtly different. The original code wasNULL (the default) doesn't check the value at all, it just checks that the field exists; from the
assertFieldByXPath()
docs:fieldValueEquals()
treats NULL differently - it explicitly checks the value is the empty string.As this behaviour is removed I think #62 is OK, but will wait for your thoughts before marking RTBC.
Comment #65
mondrakeI see. Very subtle. That’s fine with #62, then, otherwise we’d be testing what Mink is testing itself. In any case, IMHO per #52 we should eventually refactor that test anyway later.
Thanks a lot for detailed review.
Comment #66
longwaveGreat, let's get this in then!
Comment #67
mondrakeComment #68
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedComment #69
Suresh Prabhu Parkala CreditAttribution: Suresh Prabhu Parkala at Specbee commentedRe-rolled patch please review.
Comment #70
mondrake@Suresh it would help if you added a raw diff i.e. the diff between the 2 patches - that is not easy to read but it helps identifying what was changed.
Comment #72
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have tried to fixed failed tests and removed the needs reroll tag.
Looks like this: " $this->assertText('Name value: value changed by setValueForElement() in #element_validate', 'Form element value in $form_state was altered.'); " removed mistakenly instead of this " $this->assertFieldByName('name', '#value changed by #element_validate', 'Form element #value was altered.'); " in patch #69
So I have fixed the above.
Comment #74
mondrakeOn this.
Comment #75
mondrakeReroll from #62 that was the last RTBCed one. Two patch context changes due to #3159230: AssertLegacyTrait::assert(No)Raw() in functional tests still have a message passed in.
Comment #76
mondrakeComment #77
mondrakeBack to RTBC, thanks @Suresh @ravi.shankar for your work - unfortunately https://www.drupal.org/patch/reroll is not very helpful, I understand it can be difficult rerolling with merge conflicts - and it very much depends on your workflow and available tools. If I may, as a rule of thumb:
Comment #78
mondrakeReroll.
Comment #79
mondrakeReroll.
Comment #81
catchCommitted a14e48a and pushed to 9.1.x. Thanks!