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
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.
Comment | File | Size | Author |
---|---|---|---|
#19 | 3078501-19.patch | 847 bytes | sd9121 |
#16 | alertParamters.png | 13.26 KB | paulocs |
#15 | 3078501-15.patch | 1.03 KB | sd9121 |
#7 | interdiff_1-7.txt | 805 bytes | swatichouhan012 |
#7 | 3078501-7.patch | 896 bytes | swatichouhan012 |
Comments
Comment #2
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedComment #4
sd9121 CreditAttribution: sd9121 at QED42 commentedComment #6
ay13 CreditAttribution: ay13 commentedThe 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.
Comment #7
swatichouhan012 CreditAttribution: swatichouhan012 at Valuebound for Valuebound commentedHere is the new patch kindly review.
Comment #10
sd9121 CreditAttribution: sd9121 at QED42 commentedComment #11
sd9121 CreditAttribution: sd9121 at QED42 commentedPlease review the updated patch.
Thanks!
Comment #13
longwaveI don't think this fix is right, we don't want "undefined" appearing in alert boxes.
Comment #14
sd9121 CreditAttribution: sd9121 at QED42 commentedComment #15
sd9121 CreditAttribution: sd9121 at QED42 commentedComment #16
paulocsPatch #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.
Comment #17
larowlanReclassifying 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
Comment #18
sd9121 CreditAttribution: sd9121 at QED42 commentedComment #19
sd9121 CreditAttribution: sd9121 at QED42 commentedThanks 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!
Comment #20
longwaveThis 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.
Comment #21
larowlanadding review credits
Comment #23
larowlanCommitted 6e451a1 and pushed to 9.1.x. Thanks!
Adding JavaScript tag posthumously