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

Comments

partyka created an issue. See original summary.

partyka’s picture

StatusFileSize
new993 bytes

@longwave also deserves credit for a conversation in slack.

partyka’s picture

Issue summary: View changes
longwave’s picture

Can 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.

partyka’s picture

StatusFileSize
new934 bytes
new997 bytes

Here's the updated message.

partyka’s picture

Issue tags: +Bug Smash Initiative
partyka’s picture

Status: Active » Needs review

This patch is ready for review!

partyka’s picture

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Maybe 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.

mondrake’s picture

Status: Reviewed & tested by the community » Needs review

IMHO we should just deprecate drupalPostForm in favor of submitForm - you simply cannot deprecate a return type, and we will be just postponing the problem here.
drupalPostForm in its current form is a just wrapper of submitForm with castSafeStrings and some init additions.
We may add the deprecation to the deprecation silencing list in the listener and do conversions in follow ups.

partyka’s picture

@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!

mondrake’s picture

@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 of assertEqual which 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 getSkippedDeprecations in 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 drupalGet and submitForm we can achieve the same results. Out of the 2947 above, 1060 are calls to drupalPostForm(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.

larowlan’s picture

IMHO we should just deprecate drupalPostForm in favor of submitForm

Yes 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?

mondrake’s picture

Title: Return value of `\Drupal\Tests\UiHelperTrait::drupalPostForm` still deprecated » Deprecate `\Drupal\Tests\UiHelperTrait::drupalPostForm`, keep deprecation silenced
Issue summary: View changes
Status: Needs review » Active
Issue tags: +Deprecated assertions

Yes, let’s repurpose. Updated issue title and summary.

mondrake’s picture

Status: Active » Needs review
Issue tags: +Needs change record
StatusFileSize
new2.24 KB
new1.02 KB

Let's see this.

Status: Needs review » Needs work

The last submitted patch, 15: 3166543-15-test-only.patch, failed testing. View results

mondrake’s picture

Converting calls to a NULL $path seems quite straightforward, so filed #3168375: Convert calls to drupalPostForm(NULL, ...) to submitForm and made a patch for that.

larowlan’s picture

Category: Bug report » Task
mondrake’s picture

Title: Deprecate `\Drupal\Tests\UiHelperTrait::drupalPostForm`, keep deprecation silenced » Deprecate UiHelperTrait::drupalPostForm, keep deprecation silenced
mondrake’s picture

Status: Needs work » Needs review
Issue tags: -Needs change record
StatusFileSize
new2.6 KB

Draft CR @ https://www.drupal.org/node/3168858, patch adjusted.

lendude’s picture

Status: Needs review » Reviewed & tested by the community

CR 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::getSkippedDeprecations enough reminder that we need to clean this up in core at some point?

mondrake’s picture

#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.

  • catch committed 4a3abc4 on 9.1.x
    Issue #3166543 by mondrake, partyka, longwave, larowlan, Lendude:...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed 4a3abc4 and pushed to 9.1.x. Thanks!

mondrake’s picture

Published CR

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.