Problem/Motivation

According to MDN, window.alert() has only 1 parameter.
Drupal.AjaxCommands.prototype.alert() is implemented below and the 2nd parameter has no affect on an alert dialog.

alert(ajax, response, status) {
  window.alert(response.text, response.title);
},

Proposed resolution

Like other window.alert() in /core/misc/ajax.es6.js, join title and text.

CommentFileSizeAuthor
#19 3078501-19.patch847 bytessd9121
#16 alertParamters.png13.26 KBpaulocs
#15 3078501-15.patch1.03 KBsd9121
#11 3078501-11.patch895 bytessd9121
#7 interdiff_1-7.txt805 bytesswatichouhan012
#7 3078501-7.patch896 bytesswatichouhan012
ajax-alert-fix.patch884 bytesTom Konda
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Tom Konda created an issue. See original summary.

ravi.shankar’s picture

ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commented
Status: Active » Needs review

Status: Needs review » Needs work

The last submitted patch, ajax-alert-fix.patch, failed testing. View results

sd9121’s picture

sd9121 CreditAttribution: sd9121 at QED42 commented
Assigned: Unassigned » sd9121

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.0-alpha1 will be released the week of October 14th, 2019, which means new developments and disruptive changes should now be targeted against the 8.9.x-dev branch. (Any changes to 8.9.x will also be committed to 9.0.x in preparation for Drupal 9’s release, but some changes like significant feature additions will be deferred to 9.1.x.). For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

ay13’s picture

ay13 CreditAttribution: ay13 commented

The test fails because it is expecting the same result from what is passed in, 'string' === 'string', but the patch joins response.text and response.title together.

Updating the test to take the text and title into consideration would pass the issue.

swatichouhan012’s picture

swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commented
Assigned: sd9121 » Unassigned
Status: Needs work » Needs review
FileSize
3078501-7.patch896 bytes
interdiff_1-7.txt805 bytes

Here is the new patch kindly review.

Status: Needs review » Needs work

The last submitted patch, 7: 3078501-7.patch, failed testing. View results

Version: 8.9.x-dev » 9.1.x-dev

Drupal 8.9.0-beta1 was released on March 20, 2020. 8.9.x is the final, long-term support (LTS) minor release of Drupal 8, which means new developments and disruptive changes should now be targeted against the 9.1.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

sd9121’s picture

sd9121 CreditAttribution: sd9121 at QED42 commented
Assigned: Unassigned » sd9121
sd9121’s picture

sd9121 CreditAttribution: sd9121 at QED42 commented
Assigned: sd9121 » Unassigned
Status: Needs work » Needs review
FileSize
3078501-11.patch895 bytes

Please review the updated patch.

Thanks!

Status: Needs review » Needs work

The last submitted patch, 11: 3078501-11.patch, failed testing. View results

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented 
1) Drupal\FunctionalJavascriptTests\Ajax\CommandsTest::testAjaxCommands
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'Alert'
+'undefined: Alert'

I don't think this fix is right, we don't want "undefined" appearing in alert boxes.

sd9121’s picture

sd9121 CreditAttribution: sd9121 at QED42 commented
Assigned: Unassigned » sd9121
sd9121’s picture

sd9121 CreditAttribution: sd9121 at QED42 commented
Assigned: sd9121 » Unassigned
Status: Needs work » Needs review
FileSize
3078501-15.patch1.03 KB
paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
CreditAttribution: paulocs at CI&T commented
Status: Needs review » Reviewed & tested by the community
FileSize
alertParamters.png13.26 KB

Patch #15 looks good to me. It is okay for the tests and now window.alert() has only one parameter as expected.
I also did not find other place in core that uses window.alert() with two parameters.
Set to RTBC.

Cheers, Paulo.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Category: Bug report » Task
Status: Reviewed & tested by the community » Needs work
Issue tags: +Bug Smash Initiative

Reclassifying this as a task, because we're doing cleanup here - there is no actual bug.

If I look at \Drupal\Core\Ajax\AlertCommand we only support 'text' and not title.

So I'm not sure we're ever going to have a title.

So I think we need to remove the premise that there is a response.title

sd9121’s picture

sd9121 CreditAttribution: sd9121 at QED42 commented
Assigned: Unassigned » sd9121
sd9121’s picture

sd9121 CreditAttribution: sd9121 at QED42 commented
Assigned: sd9121 » Unassigned
Status: Needs work » Needs review
FileSize
3078501-19.patch847 bytes

Thanks you @larowlan for reviewing my patch.

I have removed response.title from the alert box and only kept response .text value.

Please review this patch.

Thank you!

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Needs review » Reviewed & tested by the community

This is a nice simple fix now.

I looked back in git history and this code was introduced in the very first commit to ajax.js in #544418: Integrate CTools AJAX framework with Drupal to extend (and replace) existing ahah framework yet the response.title was never set there either, I guess this was inherited from CTools but never worked there either.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

adding review credits

  • larowlan committed 6e451a1 on 9.1.x 
    Issue #3078501 by sd9121, swatichouhan012, Tom Konda, larowlan, longwave...
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +JavaScript

Committed 6e451a1 and pushed to 9.1.x. Thanks!

Adding JavaScript tag posthumously

Status: Fixed » Closed (fixed)

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