Problem/Motivation

Following the !placeholder replacement in PHP code, we also should perform the same replacement for JavaScript.
This keeps placeholder usage in PHP and JavaScript consistent, which is good for DX.

Scripts use:

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

Proposed resolution

  • Replace !placeholder by @placeholder for both URLs and non-URLs (this issue)
  • Decide if we need a :placeholder for URLs. According to discussion with alexpott in IRC there is currently no use case for this. (separate issue?)
  • #2570101: Remove !placeholder support from Drupal.formatString
  • Manually test for double escaping

Remaining tasks

t.b.d.

User interface changes

none

API changes

t.b.d.

Data model changes

none

Files: 

Comments

Sutharsan created an issue. See original summary.

alexpott’s picture

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.

Sutharsan’s picture

Status: Active » Needs review
FileSize
4.48 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,244 pass(es). View

Replacing all !placeholders in JS by @placeholder, both non-url and url. I can not oversee the consequences for URLs yet, but we need to start somewhere.

Sutharsan’s picture

Issue summary: View changes
Sutharsan’s picture

Issue summary: View changes
dawehner’s picture

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

Sutharsan’s picture

The way Drupal.Ajax is used in core, the CustomMessage is always empty. But technically, yes it may contain HTML. As far as I know we don't have a JS tool for double escape.

dawehner’s picture

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

effulgentsia’s picture

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.

xjm’s picture

pwolanin’s picture

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

nod_’s picture

Status: Needs review » Closed (won't fix)

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

nod_’s picture

Title: Replace !placeholder with @placeholder in JavaScript » Replace !placeholder with @placeholder where needed in JavaScript
Status: Closed (won't fix) » Needs work

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.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Chernous_dn’s picture

Status: Needs work » Needs review
FileSize
5.67 KB

Use

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

Create patch.

Chernous_dn’s picture

The last submitted patch, 15: replace_placeholder-2570093-15.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 16: replace_placeholder-2570093-16.patch, failed testing.

Chernous_dn’s picture

Chernous_dn’s picture

Status: Needs work » Needs review

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.