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
Ditto.
Some WebAsserts methods are used to convert to PHPUnit earlier Simpletest tests. The original methods carried $message and $group arguments that were used to print information on the Simpletest Test Results UI - but no longer are relevant in PHPUnit.
Proposed resolution
In order to prevent cruft, deprecate passing more arguments than the signature allows, so that in D10 these deprecation can be converted to exception throwing.
Remaining tasks
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#28 | interdiff_26-28.txt | 20.79 KB | mondrake |
#28 | 3160405-28.patch | 77.17 KB | mondrake |
Comments
Comment #2
mondrakeComment #4
mondrakeComment #5
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI Will work on this.
Comment #6
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have added a patch which might fix failed tests of patch #2.
Comment #7
mondrakethank you, in these cases the content of the string removed should be added as a comment, since it is telling somenthing more about the reason the response does or does not contain something.
We also need a change record and deprecation tests to demonstrate that methods called with a $message are yielding a deprecation error.
Comment #8
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedI have added string as a comment as suggested in comment #7.
It still needs work for remaining things of comment #7.
Comment #9
mondrakeFixing one test, and adding the same check also to methods inherited from Mink's base WebAssert class.
Comment #10
mondrakeComment #11
mondrakeComment #12
mondrakeClosing #3143398: WebAssert methods do not have a $message argument like AssertLegacyTrait methods as duplicate of this - please give @cburshcka credit for his work on the other issue
Comment #13
jungleThis is great, thanks @mondrake for filing this.
$dynamic_skipped_deprecations
in\Drupal\Tests\Listeners\DeprecationListenerTrait::isDeprecationSkipped()
Should be
$container = NULL
Comment #14
mondrakeThanks @jungle!
#13.1 - actually we should not silence, but remove cruft
#13.2 - done, thanks
Changed from task to bug and tagged 'Bug Smash Initiative' as suggested on Slack.
Comment #15
mondrake#3133355: Introduce PHPUnit-compliant WebAssert::responseHeaderExists() method, and its negative introduces a
WebAssertTest
class that would be useful to then add the deprecation tests to. That's been in RTBC for a while, so let's see if that goes in soon rather than add here too.Comment #16
jungleThanks @mondrake!
You are RIGHT!
Should we add
@legacy
tests testing deprecated messages?Looks like
t()
call could be removed.CR needed/CR URL to be updated, setting back to NW for this at least.
Comment #17
mondrakeThanks @jungle
#16.3 - done
#16.4 - done
Tests still outstanding. Thinking whether to wait for #15, or write a new unit test that mocks a response - 52 functional tests just for deprecation are a performance hit.
Comment #19
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedChangin status to needs review as patch #17 passed the tests.
Comment #20
longwave#3133355: Introduce PHPUnit-compliant WebAssert::responseHeaderExists() method, and its negative landed, so we can add to WebAssertTest, but also not sure about adding 52 new functional tests just to cover these deprecations.
Comment #21
mondrakeJust a reroll.
Comment #22
mondrakeRe. #17 and #20, here adding a unit test that mocks the session, so tests can be run much faster, and 52 of them would not be a burden... but to write them. Just done a few methods, will need work for the rest.
Comment #23
mondrakeComment #24
mondrakeSome progress. The complicated part is to mock the DOM elements for methods that do xpath queries. I haven't come up with a solution yet.
Comment #25
mondrakeComment #26
mondrakeFound a way, some progress but more to go.
Comment #27
mondrakeOn this.
Comment #28
mondrakeCompleted adding tests, ready for review.
Comment #29
longwaveNice work on the unit tests for this! I reviewed all the changes and don't see any issues here.
Great that we are fixing these, this was surely an incorrect use of t() anyway.
Comment #31
larowlanThis is some top-shelf work folks - I love that we both fix it and prevent it happening further
Committed 050b91b and pushed to 9.1.x. Thanks!
Published the change record.
Comment #32
TR CreditAttribution: TR commentedWould you please list in the CR which WebAssert methods are affected by this, so a search of the change records for a specific deprecated method will reveal this change and so we have guidance on what we need to change to be D9.1-compatible? Yeah, I guess I could figure it out by myself on a case-by-case basis when this change affects one of my projects, but it would be much easier if there were a list so I could just grep to see if this is even an issue for my projects and so I can fix all cases at the same time.
Comment #33
mondrake#32 updated the CR with the list of affected methods.