Problem/Motivation

This string should not be passed through the translation system. It should maybe be FormattableMarkup. Needs a follow-up filed to fix and discuss.

from alexpott in https://git.drupalcode.org/project/drupal/-/merge_requests/8393/diffs#no...

From core/lib/Drupal/Core/Form/ConfigFormBase.php,
$this->t(implode("\n", $transformed_message_parts));

$this->t(implode("\n", $transformed_message_parts)); shouldn't be use t(). Instead, it should be handled as FormattableMarkup() to ensure proper handling of content and prevent unintended translation.

Follow up from #3123067: Fix 'Drupal.Semantics.FunctionT.NotLiteralString' coding standard

Steps to reproduce

Proposed resolution

can use FormattableMarkup() instead of t(), see: core/includes/errors.inc

Remaining tasks

Investigate and decide on a course of action
Review

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3456149

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone commented
Related issues: +#3123067: Fix 'Drupal.Semantics.FunctionT.NotLiteralString' coding standard
quietone’s picture

quietone commented
Title: Fix transltion string in ConfigFormBase.php » Fix translation string in ConfigFormBase.php

pooja_sharma made their first commit to this issue’s fork.

pooja_sharma opened merge request !8542

pooja_sharma’s picture

pooja_sharma commented

I have observed in core files (for eg. core/includes/errors.inc) instead of using translation t() can be use FormattableMarkup() (even same mentioned in MR where this issue trace out) , So replaced the t() with FormattableMarkup().

Please review , moving NR.

pooja_sharma’s picture

pooja_sharma commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Can the issue summary be updated as well please.

Thanks

pooja_sharma’s picture

pooja_sharma commented
Issue summary: View changes

Added description , Please review &moving to NR

pooja_sharma’s picture

pooja_sharma commented
Issue summary: View changes
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Made a small tweak to the comment, pulling from the issue summary, but change seems good to me. Function being protected assuming will be non disruptive to existing forms.

pooja_sharma’s picture

pooja_sharma commented

Rebased MR with latest code, seems working fine.

pooja_sharma’s picture

pooja_sharma commented

MR conflicts are visible on MR, addressed those

Rebased MR with latest code, seems working fine.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Added a comment to the MR.

pooja_sharma’s picture

pooja_sharma commented

Applied the mentioned suggestions.

Please review, moving NR

pooja_sharma’s picture

pooja_sharma commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Left a comment on MR.

pooja_sharma’s picture

pooja_sharma commented
Status: Needs work » Needs review

Applied the mentioned suggestions.

pooja_sharma’s picture

pooja_sharma commented

Please review, moving NR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Don't have a preference over MarkupInterface or \Stringable but only one is used

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

One comment on the comment. Also I think @alexpott might have been asking for a return value of MarkupInterface|Stringable?

pooja_sharma’s picture

pooja_sharma commented

Applied suggestion: MarkupInterface|\Stringable for return type

Please review , moving NR

pooja_sharma’s picture

pooja_sharma commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

That's my mistake @pooja_sharma. Think you had that before and had you change it as I interpreted it differently.

alexpott’s picture

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

Only committing to 11.x since the change is very minor and results in an API widening.

Committed b014318 and pushed to 11.x. Thanks!

  • alexpott committed b0143182 on 11.x 
    Issue #3456149 by pooja_sharma, smustgrave, quietone, catch, alexpott:...

Status: Fixed » Closed (fixed)

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

gapple’s picture

gapple
he/they
Primary language English
commented

Ran into an issue with the the return type change, since I was developing a module using the latest version of core and copied the new return type hint.

When using the new return type in an override with a stable version of core <=11.0.5, the child class is returning a wider type than it's parent.
When using the old return type in an override with 11.x-dev, parent::formatMultipleViolationsMessage() returns a wider type than the child class.

For a module to maintain compatibility with older versions of core and not break if it calls the parent method which now returns Markup, it will need to change the value returned from its parent:

  protected function formatMultipleViolationsMessage(string $form_element_name, array $violations): TranslatableMarkup {
    $parent = parent::formatMultipleViolationsMessage($form_element_name, $violations);
    if ($parent instanceof TranslatableMarkup) {
      return $parent;
    }
    else {
      // phpcs:ignore Drupal.Semantics.FunctionT.NotLiteralString
      return $this->t((string) $parent);
    }
  }

Once the module drops support for older versions of core, it can update its typehint and remove the extra code.

\Drupal\update\UpdateSettingsForm also needs its return type updated.

gapple’s picture

gapple
he/they
Primary language English
commented

Opened #3481331: ConfigFormBase::formatMultipleViolationsMessage() return type change will cause error as a bug from this change.