Problem/Motivation
The file core/tests/Drupal/Tests/UiHelperTrait.php, method \Drupal\Tests\UiHelperTrait::drupalPostForm still has the following comment:
* @return string
* (deprecated) The response content after submit form. It is necessary for
* backwards compatibility and will be removed before Drupal 9.0. You should
* just use the webAssert object for your assertions.
Steps to reproduce
None (view the file) :-).
Proposed resolution
- Deprecate drupalPostForm
- Write a CR to inform developers to use drupalGet and submitForm instead
- Silence the deprecation and open follow ups to do the proper conversions
Remaining tasks
Implement the proposed resolution.
User interface changes
None.
API changes
None at this time.
Data model changes
None.
Release notes snippet
The return value of method \Drupal\Tests\UiHelperTrait::drupalPostForm was slated for deprecation in 9.0, however, this never happened. Therefore, the version number for deprecation has been bumped to 10.0
| Comment | File | Size | Author |
|---|---|---|---|
| #20 | 3166543-20.patch | 2.6 KB | mondrake |
| #15 | 3166543-15-test-only.patch | 1.02 KB | mondrake |
| #15 | 3166543-15.patch | 2.24 KB | mondrake |
Comments
Comment #2
partyka commented@longwave also deserves credit for a conversation in slack.
Comment #3
partyka commentedComment #4
longwaveCan we tag the @todo as something like "@todo Remove this before drupal:10.0.0" as that matches other @todos and the "drupal:10.0.0" format is something at least I was looking for when helping fix up the deprecations in the Drupal 9 cycle.
Comment #5
partyka commentedHere's the updated message.
Comment #6
partyka commentedComment #7
partyka commentedThis patch is ready for review!
Comment #8
partyka commentedComment #9
longwaveMaybe we need an actual postponed issue tagged for D10 to do this removal? Not sure, marking RTBC in the meantime as the changes here improve things, even though there is still no guarantee we will actually do it this time.
Comment #10
mondrakeIMHO we should just deprecate
drupalPostFormin favor ofsubmitForm- you simply cannot deprecate a return type, and we will be just postponing the problem here.drupalPostFormin its current form is a just wrapper ofsubmitFormwith castSafeStrings and some init additions.We may add the deprecation to the deprecation silencing list in the listener and do conversions in follow ups.
Comment #11
partyka commented@mondrake -- one of the things that @longwave and I discussed in slack was simply creating a new method. For the sake of argument, let's call this `drupalSubmitForm`, and it's identical to `drupalPostForm` minus the return and any logic associated with the return. However, there was a concern that this might be a large refactor due to the the number of times it's used.
This is the first I've heard of a `deprecation silencing list` -- do you mind sharing where this list is located? Thanks!
Comment #12
mondrake@partyka I don't think we should be concerned about deprecations that would imply large refactorings - yes it will take a while and possibly multiple issues to complete, but deprecation silencing is there exactly to allow for that.
For exampe, ATM there are 2947 usages of
drupalPostForm, and 3828 ofassertEqualwhich is currently already deprecated and silenced.Silencing occurs by doing the proper deprecation with @deprecated, @see and @trigger_error in the deprecated method/function, then adding the exact deprecation message in the array returned by
getSkippedDeprecationsin the file core/tests/Drupal/Tests/Listeners/DeprecationListenerTrait.php.Finally - IMHO we do not need a new method. We should try to refactor in a way that just using
drupalGetandsubmitFormwe can achieve the same results. Out of the 2947 above, 1060 are calls todrupalPostForm(NULL, ...which means that the drupalGet is not called. Also, to me it's weird and counterintuitive that a method called 'postForm' performs a GET before the POST... separating the GET vs the POST would make tests more readable.Comment #13
larowlanYes I agree, I was going to say that would be a new issue - but looking at the patch here - I think we could repurpose this one - thoughts?
Comment #14
mondrakeYes, let’s repurpose. Updated issue title and summary.
Comment #15
mondrakeLet's see this.
Comment #17
mondrakeConverting calls to a NULL $path seems quite straightforward, so filed #3168375: Convert calls to drupalPostForm(NULL, ...) to submitForm and made a patch for that.
Comment #18
larowlanComment #19
mondrakeComment #20
mondrakeDraft CR @ https://www.drupal.org/node/3168858, patch adjusted.
Comment #21
lendudeCR looks good and the deprecation is in line with the other test deprecations. Do we need follow ups already filed for cleaning up all cases or is the fact that this is in
\Drupal\Tests\Listeners\DeprecationListenerTrait::getSkippedDeprecationsenough reminder that we need to clean this up in core at some point?Comment #22
mondrake#21 there's a couple follow-ups already, listed in the CR. Maybe we need (another) meta to keep track?
Any suggestion on how to split the task welcome.
Comment #24
catchCommitted 4a3abc4 and pushed to 9.1.x. Thanks!
Comment #25
mondrakePublished CR