Follow-up from #3316274: Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest(). In that issue we added a new assertExpectedAjaxRequest() method, but didn't deprecate the old one.
Convert all calls to assertWaitOnAjaxRequest() to assertExpectedAjaxRequest().
Add @deprecated and @trigger_error(....... E_USER_DEPRECATED) to the old method for removal in Drupal 11.
See https://www.drupal.org/about/core/policies/core-change-policies/drupal-d... for the format.
assertWaitOnAjaxRequest() is defined and add the deprecation notice there as explained in the proposed resolution.| Comment | File | Size | Author |
|---|---|---|---|
| #2 | 3401619-2.patch | 209.32 KB | pradhumanjain2311 |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
changes
Comments
Comment #2
pradhumanjain2311 commentedReplace assertWaitOnAjaxRequest with assertExpectedAjaxRequest
and add the deprecation warning in assertWaitOnAjaxRequest method.
Please review.
Comment #3
needs-review-queue-bot commentedThe Needs Review Queue Bot tested this issue.
While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)
Comment #4
catchThe changes look mostly fine. This actually requires #3316274: Stabilize FunctionalJavascript testing AJAX: add ::assertExpectedAjaxRequest() to land first, I should have marked it postponed on that and doing so now.
For the new deprecation, it should be from 10.2.0 instead of 10.1.0
Comment #5
quietone commentedComment #6
ankitv18 commentedComment #7
ankitv18 commentedComment #8
nafisb commentedI'm Nafis at DrupalCon Atlanta. I plan on working on this issue for the rest of the day.
Comment #10
ankitv18 commentedComment #15
brandonlira commentedHi, I've created a new clean branch to address this issue.
The previous branch had too many unrelated commits and was outdated, causing multiple conflicts and making it hard to review.
This new branch includes only the intended change: replacing all usages of the deprecated assertWaitOnAjaxRequest() with assertExpectedAjaxRequest() across the core/ directory.
A new merge request has been opened from this clean branch.
Could someone please review and test this updated MR !12108?
Thank you!
Comment #16
brandonlira commentedComment #18
smustgrave commentedTicket talks about deprecating a method but don't see the deprecation
Comment #19
megachrizThe Drupal Contribution Mentoring team is triaging issues for DrupalCon Vienna 2025, and we are reserving this issue for Mentored Contribution during the event.
After October 17, this issue returns to being open to all. Thanks!
The next step is to add the deprecation notice: find the place where the method
assertWaitOnAjaxRequest()is defined and add the deprecation notice there as explained in the proposed resolution.Comment #20
rory downes commentedI am working on this in DrupalCon Vienna
Comment #21
rory downes commentedThe task to add the depreciation notice has already been done in the branch created by @Brandonlira in comment https://www.drupal.org/project/drupal/issues/3401619#comment-16105612 - 3401619-deprecate-assertwaitonajaxrequest-rebased.
The other branch (issue-3401619-ajax-method-cleanup) is replacing all usages of the old method in core with the new method.
I am happy that both branches look OK but have not yet tested them.
What is not clear to me is whether the old method is to be replaced in all instances or if the new method is only intended to replace the old when needed.
The CR of the original ticket https://www.drupal.org/node/3401201 talks about firing a deprecation notice in one circumstance:
If either method is called when there are zero additional ajax requests made, but one or more ajax requests had occurred already, a deprecation error will be fired.
I think that I understand the new method is resolving that with the new parameter.
The old method calls the new with NULL in the new parameter.
We need someone more involved in the functional javascript and specifically Ajax testing (presumably the relevant framework manager) to decide if the old method will be removed. If it is then the two branches need to be tested ready for merging.
For this reason I am setting this back to Needs review and tagging it with 'Needs framework manager review'.
Comment #22
catchWe should remove assertWaitOnAjaxRequest() in Drupal 13.0.0 probably.
If all the usages are converted here and tests pass, then adding the deprecation can also happen in this issue (not at computer so didn't review the MR to check current state).
Comment #23
xjmComment #27
xjmBased on #22 and reading the change record for assertExpectedAjaxRequest(), I think we should just deprecate
assertWaitOnAjaxRequest(), but put a little information from the CR in the deprecation message, and write a good CR for this issue based on that one.We can disregard the previous branches and merge requests. I've ensured they're all hidden for clarity.
Writing such a deprecation and adding it to the docblock for
assertWaitOnAjaxRequest()is a good first-time contributor task.The Drupal Contribution Mentoring team is triaging issues for DrupalCon Nara 2025, and we are reserving this issue for Mentored Contribution during the event.
After November 19, this issue returns to being open to all. Thanks!
Thanks everyone!
Comment #29
catchBumping this to major since it's still the cause of a lot of random test failures, most recently in #3628411: [ci] Share compiled scripts between PHPUnit processes with opcache.file_cache.