Follow-up to #2571915: Replace remaining !placeholder for field widget stuff

Problem/Motivation

See #2566503: [meta] Replace remaining !placeholder for Non-URL HTML outputs only

ib/Drupal/Core/Form/form.api.php:    $message = t("!count items were processed.", array(
lib/Drupal/Core/Form/FormValidator.php:          $form_state->setError($elements, $this->t('!name field is required.', array('!name' => $elements['#title'])));
lib/Drupal/Core/Form/FormValidator.php:      $form_state->setError($elements, $this->t('!name cannot be longer than %max characters but is currently %length characters long.', array('!name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title'], '$max' => $elements['#maxlength'], '%length' => Unicode::strlen($elements['#value']))));

Proposed resolution

The one remaining question is do translators have a need to add HTML to a date format string?

Remaining tasks

Agree that removing HTML support makes sense.

User interface changes

None

API changes

Date format strings no longer support adding HTML using the \ escape character.

Data model changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because at the moment date formats support HTML but it is escaped
Issue priority Major because part of the critical to remove !placeholder
Disruption Disruptive for existing sites that are adding HTML to date formats. If HTML is required in a formatted date then the site should implement a custom field formatter to do this.
CommentFileSizeAuthor
#14 remove_placeholder_in-2571919-14.patch958 byteslegolasbo
#2 remove_html_support-2571919-2.patch2.57 KBnevergone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner created an issue. See original summary.

nevergone’s picture

nevergone CreditAttribution: nevergone commented
Status: Active » Needs review
FileSize
remove_html_support-2571919-2.patch2.57 KB
pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented

In #2568613: Remove HTML support from date formats and replace remaining !placeholder where format_date() and 'date.formatter' => format() are used ?

At the least, we are saying there that we don't escape HTML:
HTML is not escaped by the date formatter, it must be escaped later.

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented
Title: Remove HTML support from date formats and replace remaining !placeholder for fapi stuff » replace remaining !placeholder for fapi stuff in FormValidator.php and form.api.php
dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Title: replace remaining !placeholder for fapi stuff in FormValidator.php and form.api.php » Remove !placeholder in FAPI

Meh.

+++ b/core/lib/Drupal/Core/Form/form.api.php
@@ -110,8 +110,8 @@ function callback_batch_operation($MULTIPLE_PARAMS, &$context) {
-    $message = t("!count items were processed.", array(
-      '!count' => count($results),
+    $message = t("@count items were processed.", array(
+      '@count' => count($results),

Are we sure here, because its batch api?

pwolanin’s picture

pwolanin CreditAttribution: pwolanin as a volunteer and at Acquia commented

@dawehner - I'm not sure what you are questioning. The count will always be an int and @count won't affect it.

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner as a volunteer commented
Status: Needs review » Reviewed & tested by the community

Its the number this would be never double escaped.

stefan.r’s picture

stefan.r CreditAttribution: stefan.r commented

RTBC++, considering #title is always supposed to be wrapped in t()

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

And you are supposed to make sure batch api JS variables are safe - so this makes sense.

Committed beed345 and pushed to 8.0.x. Thanks!

  • alexpott committed beed345 on 8.0.x 
    Issue #2571919 by nevergone: Remove !placeholder in FAPI

Status: Fixed » Needs work

The last submitted patch, 2: remove_html_support-2571919-2.patch, failed testing.

nevergone’s picture

nevergone CreditAttribution: nevergone commented
Status: Needs work » Fixed

Already committed.
Closed.

Status: Fixed » Closed (fixed)

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

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
CreditAttribution: legolasbo as a volunteer commented
Priority: Critical » Major
Status: Closed (fixed) » Needs review
FileSize
remove_placeholder_in-2571919-14.patch958 bytes

We missed one in FormValidator::performRequiredValidation().

Attached patch adjusts the placeholder.

swentel’s picture

swentel CreditAttribution: swentel commented 
+++ b/core/lib/Drupal/Core/Form/FormValidator.php
@@ -346,7 +346,7 @@ protected function performRequiredValidation(&$elements, FormStateInterface &$fo
+            $this->logger->error('Illegal choice %choice in %name element.', array('%choice' => $v, '%name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title']));

Shouldn't this just be @ instead of % ?

heddn’s picture

heddn
Primary language English
Location Nicaragua
CreditAttribution: heddn commented

Instead of reopening, which messes with commit credit, etc. Can we open a new follow-up issue?

legolasbo’s picture

legolasbo
he/him
Primary language Dutch
Location Middelburg
CreditAttribution: legolasbo as a volunteer commented
Status: Needs review » Closed (fixed)

As suggested in #16 I created a follow up at #2637458: Remove !placeholder in FormValidator::performRequiredValidation()