Consider whether Infrastructure\Service\Translation\Translator should add quotes around placeholders

[effulgentsia]

Consider the following code:

$this->t(
    'The source and destination files cannot be the same at %path',
    $this->p(['%path' => '/some/path']),
)

When Drupal's translator is used, then when this message is displayed in the Drupal admin UI to an administrator whose preferred language is English, the message will be:

The source and destination files cannot be the same at <em class="placeholder">/some/path</em>

In other words, since Drupal is a web CMS, Drupal's translator returns HTML, with placeholder values formatted with <em> tags.

Currently, Composer Stager's Translator just inserts placeholder values with no formatting. In other words, it outputs:

The source and destination files cannot be the same at /some/path

Composer Stager should not assume an HTML display of its messages, so it should not follow Drupal's example of wrapping placeholder values with <em> tags, but... perhaps it should wrap them with quotes, so as to output:

The source and destination files cannot be the same at "/some/path"

That would mean prior to Infrastructure\Service\Translation\Translator calling return $this->symfonyTranslatorProxy->trans(...), it would modify $parameters to add quotes around all of them. More importantly, it would mean Composer Stager's Translator having some opinionated implementation, and not just be an un-opinionated wrapper/proxy around Symfony\Contracts\Translation\TranslatorTrait.

Is it appropriate and beneficial for Composer Stager's Translator to add the opinionated decision of adding quotes around placeholder values? Personally, I think yes, and that it would both improve the corresponding exception messages, and clarify that the scope of a translator includes not only translating to the desired language but also deciding how to format placeholder values.

[effulgentsia]

The counterexample is that in Domain\Factory\Translation\TranslatableAwareTrait the example in the docs is:

Hello, %first_name %last_name.

In this case we would not want quotes (or separate <em> tags) around each placeholder value. In Drupal the way we handle this is to use a different prefix, so in this case the translatable string would be:

Hello, @first_name @last_name.

In other words, in Drupal, % means to wrap in <em> tags and @ means not to. However, that's a Drupal convention and not a Symfony convention, so I don't think we should add such semantics to Composer Stager. However, I think that within Composer Stager we don't actually have any use case for not wanting each placeholder value emphasized on its own (whether via <em> tags or with quotes). In other words, I think we can stick with only % placeholders, and change the example in TranslatableAwareTrait to one that would make sense if each placeholder got output with quotes around it.

[TravisCarden]

@effulgentsia, since this involves translatable UI strings, I think it would be a BC breaking change if made, right? I'm tagging it a stable blocker.

[TravisCarden]

@tedbow, you agreed to look through the Automatic Updates module to see if there's anywhere it's currently displaying Composer Stager string placeholders surrounded by <em> tags in such a way that would result in poor UX if Composer Stager added quotes around them. The background question is whether Composer Stager itself should quote parameters for things like paths, command names, and error messages from caught exceptions. I'll give you a representative example that could materialize:

composer-stager/src/Internal/Process/Service/Process.php

Lines 101 to 111 in 5055aa2

	try {
	$callbackAdapter = new OutputCallbackAdapter($callback);
	return $this->symfonyProcess->run($callbackAdapter);
	} catch (Throwable $e) {
	throw new RuntimeException($this->t(
	'Failed to run process: %details',
	$this->p(['%details' => $e->getMessage()]),
	$this->d()->exceptions(),
	), 0, $e);
	}

So Failed to run process: %details is the string--no quotes (the current pattern in Composer Stager). Here's an actual message that produces in my testing--which is also what the Automatic Updates module will display if it's not wrapping the placeholder:

Failed to run process: The command "'asdf'" failed.

Exit Code: 127(Command not found)

Working directory: /Users/traviscarden/Projects/php-tuf/composer-stager

Output:
================


Error Output:
================
sh: line 0: exec: asdf: not found

Therefore, wrapping the %details placeholder with <em>s should result in this:

Failed to run process: <em>The command "'asdf'" failed.

Exit Code: 127(Command not found)

Working directory: /Users/traviscarden/Projects/php-tuf/composer-stager

Output:
================


Error Output:
================
sh: line 0: exec: asdf: not found</em>

And if we wrap it on both ends--with double quotes in Composer Stager and <em> tags in Automatic Updates, you would end up with this:

Failed to run process: "<em>The command "'asdf'" failed.

Exit Code: 127(Command not found)

Working directory: /Users/traviscarden/Projects/php-tuf/composer-stager

Output:
================


Error Output:
================
sh: line 0: exec: asdf: not found</em>"

So the questions are:

1. Is Automatic Updates already wrapping any Composer Stager parameters in such a way that this scenario could materialize?
2. If so, does it result in poor UX--or is it otherwise undesirable?
3. If the answer to either of those is "no", is there any relevant precedent in Drupal core?