Replace !placeholder with @placeholder where needed in JavaScript

I'm not 100% certain that can or need to remove the !placholder in javascript. The thing is we have nothing equivalent of auto escaping and safeness.

+++ b/core/misc/ajax.js
@@ -140,9 +140,9 @@
 
-    customMessage = customMessage ? ("\n" + Drupal.t("CustomMessage: !customMessage", {'!customMessage': customMessage})) : "";
+    customMessage = customMessage ? ("\n" + Drupal.t("CustomMessage: @customMessage", {'@customMessage': customMessage})) : "";

Does that mean that customMessage can never contain HTML in the first place at all? Not sure whether this is a right assumption, I could totally imagine that its passed through t(). Do we have a tool in JS to ensure something is not double escaped?

Maybe we should directly get http://phpjs.org/functions/htmlspecialchars/ and use that.

I agree with #2. What makes it possible to remove '!' in PHP is that '@' can conditionally escape based on the safeness of the input. In JS, '@' escapes always, so we need '!' if the value already has (hopefully safe) HTML, just like in Drupal 7.

So, maybe we need to close this issue if the JS api can't be the same as PHP?

I agree with #2 and #9. Closing related issue as well.

So there are definitely places where we should properly use @ instead of !, reopening for those. We don't have to get rid of !placeholder but we can fix some strings.

Use

egrep -r '\.t\(.*![a-zA-Z]' core
egrep -r '\.formatPlural\(.*![a-zA-Z]' core
egrep -r '\.formatString\(.*![a-zA-Z]' core

Create patch.

Update patch.

Update patch again.

Just tested the current patch and it needs a reroll. Please create a new patch. Thanks.

The ES6 change and some other major changes basically require doing this one from scratch.

Quick overview of the files that potentially need changing.

$ find . -name *.js|xargs grep -l 'Drupal.t(.*!'
./core/misc/ajax.es6.js
./core/misc/ajax.js
./core/modules/ckeditor/js/ckeditor.es6.js
./core/modules/ckeditor/js/ckeditor.js
./core/modules/locale/tests/locale_test.es6.js
./core/modules/locale/tests/locale_test.js
./core/modules/quickedit/js/models/EntityModel.es6.js
./core/modules/quickedit/js/models/EntityModel.js
./core/modules/quickedit/js/util.es6.js
./core/modules/quickedit/js/util.js
./core/modules/quickedit/js/views/EntityToolbarView.js
./core/modules/system/js/system.modules.js
./core/modules/tour/js/tour.es6.js
./core/modules/tour/js/tour.js

The above list doesn't include multi-line calls, but the converter turns those into single-lines so the compiled JS file will be matched even if the ES6 one isn't. See system.modules.js.

It looks like the whole thing basically is the same as from the old patch, with one additional one in locale_test.js.

The bad line is the !total here.

@@ -255,7 +255,7 @@
           .find('li')
           // Rebuild the progress data.
           .each(function (index) {
-            const progress = Drupal.t('!tour_item of !total', { '!tour_item': index + 1, '!total': total });
+            const progress = Drupal.t('@tour_item of !total', { '@tour_item': index + 1, '@total': total });
             $(this).find('.tour-progress').text(progress);

On phone now; will fix later.

At the airport; my final contribution to the DrupalCon sprint. :)

Forgot to rebuild the JS of course.

...and forgot to also look for formatPlural (one more occurrence) and formatString (nothing found).

Reroll for 8.5.x because of JS const codestyle.

Still valid, needs a re-roll.

Re-rolled against 10.x. Please review

Will provide the patch tomorrow

Review require

The above patch LGTM, Moving to RTBC unless we are missing any other files.

1. Patch applies cleanly.
2. Tests pass.
3. Title and issue summary are clear.
4. Change is simple and addresses the issue from the issue summary.
5. Not expected that tests are needed.
6. Issue metadata looks okay.
7. Manually tested and patch works as expected per above.

@sahil.goyal, this does not apply to 10.1.x, so the "Needs reroll" tag was correct.

@aarti zikre, when you supply new patches for an issue, please pay attention to the current status of the issue, and include both an issue comment explaining your intentions and an interdiff for any changes you made to the patch. For example, in #45, a new 10.0.x patch was already supplied, yet you commented saying you would create patch without making note of what, if anything, needed to be fixed with the previous patch. Thanks!

Rerolled the patch against 10.1.x.

Changes in the new patch:

• Drupal 10 doesn't need to have the es6 code transpiled, so we can just copy the contents from .es6.js into .js file from the D9 patch and remove the .es6.js.
• Compared the patch in #45 and #54, it included a change in core/modules/locale/tests/src/Functional/LocaleJavascriptTranslationTest.php file. Removed that file in the new patch, as this issue focuses on only .js files (as mentioned in the issue title).

Thanks!