Re-word "An illegal choice has been detected" message to remove legality suggestion

Tagging as novice task to review. This is a good issue to learn how to test that a Merge Request still applies and provide screenshots or other information about what effect the changes have.

Tagging for usability review, will slack them also

In the meantime can the MR please be updated for 10.1

Patch for Drupal version 10.1.x

Hi!

I removed the remaining ambiguous messages and I took the liberty of include a comma after the word "Please".

I attached an alternative patch with no commas as well.

Please, review.

Added patch consider #12

Hi, nicrodgers! Thanks for review.

In #11 I already added an alternative patch without the commas (no_commas-3265616-11.patch) and its interdiff with patch #9 (interdiff_no_commas_9-11.txt).

Please review.

ops, damn ADHD haha

extra full stops removed.

could someone please provide before/after screenshots and steps to reproduce in the IS?

Updated issue summary

Searching repo for "illegal choice" and there are 2 in FormValidator

Think we should log "invalid" also if that's the message we show.

Thanks, @smustgrave. I replaced the remaining 2 "illegal choice" recurrences.

Please review.

Yes, big +1 to this - I've seen users be surprised by the "illegal" message before, and "invalid" is clearer to me.

Thanks for making those changes!

This was still tagged for usability review, which should have blocked RTBC. However, simply replacing "illegal" with "invalid" is a definite improvement.

I reviewed this with git diff --color-words.

I also checked for outstanding uses of the word "illegal" with:

grep -ri "illegal" * | grep -v "node_modules" | grep -v "vendor"

There are lots, and they should probably all be fixed to use a different word. However, only this one seems directly related to the current FAPI error:

core/modules/menu_ui/menu_ui.module:  //   To avoid an 'illegal option' error after saving the form we have to load

That comment won't make any sense after this change, so we need to update it too as part of this issue.

For what it's worth, the "has been detected" part of the error message has always been confusing to me too. "Detected" how? It took me a very long time in 2006 or whatever to understand that this meant an invalid value was submitted as part of the form data. So I think the message should be changed to "An invalid value was submitted."

Then, there are two further problems with the message. One is the use of the word "Please" -- which we should never use in the UI according to our user interface text standards:

Do not use the word "please". This makes it sound as if the user is supposed to do a favor for someone.

Finally, the bit about contacting the site administrator also could be misleading. I wouldn't necessarily know who the site administrator is or how to contact them, and it could be my fault as a technical end user for submitting a custom POST request that had the wrong data types -- or, in the most likely scenario when I'm the developer writing a form, I am the site administrator and it still makes no sense.

So, I think the message should be "An invalid form value was submitted." No second sentence. But changing it to that extent would require UX review.

Marking NW at least for updating the Menu UI module inline comment, for the followups to alter other uses of "illegal" in the UI (and possibly deprecating API methods with the word as well), and finally for possibly changing it to "An invalid form value was submitted" and getting UX review of that.

Thanks everyone!

Addressed the requirements as per #24 (Updated the Menu UI module inline comment)

I am clarifying the "Steps to reproduce" in the issue summary.

The Novice tag was added in #4 for manual testing and screenshots. The Novice tasks have been done, so I am removing the tag.

TLDR of #24:

Change:

An illegal choice has been detected. Please contact the site administrator.

To:

An invalid form value was submitted.

Updating the remaining tasks.

After the UX review, we concluded the following:

The errors should be descriptive and actionable, but these errors don't fulfill that guidelines:
The first part "An illegal choice has been detected" is not enough descriptive and the second "Please contact the site administrator" is not actionable.

The "invalid" word could be also associated to people with disabilities, so we should find better options.

We found these errors only apply to some form element types like: checkboxes, tableselect and select. In addition, the code that is triggering those errors is a $form_state->setError() so it is possible to get more context about the issue to show a more descriptive message and it is already flagging the form element that failed.

Updated issue description with suggested error messages on remaining task

I just would like to point out that "invalid" when used as an adjective has a different pronunciation from "invalid" when used as a noun. The adjective is pronounced in-VAL-id, whereas the adjective is pronounced IN-va-lid (as a term used to refer to someone who is too sick to move). So they are different words, pronounced differently, despite the spelling. I certainly think we're not going to be able to eliminate the word "invalid" (as an adjective) from Drupal, so we should not attempt that as followup scope.

That said, I was considering asking for more debugging and left it aside until there had been UX review, so I am glad to see the UX review went in the same direction and I support the improved error messages in the IS. Let's get the patch updated for that behavior. Thanks!

Created a patch with the new approach, in this case an interdiff makes no sense because the code is going into another direction.

The followup issue has been filed at #3340978: [meta] Remove other uses of “illegal” from core -- thanks @schlaukopf!

Test fails look related:

1) Drupal\Tests\comment\Functional\CommentFieldsTest::testCommentFieldCreate
Behat\Mink\Exception\ResponseTextException: The text "The submitted value in Comment element is not allowed." was not found anywhere in the text of the current page.

/var/www/html/vendor/behat/mink/src/WebAssert.php:811
/var/www/html/vendor/behat/mink/src/WebAssert.php:262
/var/www/html/core/modules/comment/tests/src/Functional/CommentFieldsTest.php:168
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

ERRORS!
Tests: 5, Assertions: 107, Errors: 1.

1) Drupal\Tests\system\Functional\Form\ValidationTest::testCustomRequiredError
Undefined array key "#title"

/var/www/html/core/modules/system/tests/src/Functional/Form/ValidationTest.php:237
/var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728

ERRORS!
Tests: 5, Assertions: 82, Errors: 1.

FYI, form elements such as actions, form_id, form_build_id do not have #title

In the issue summary the proposed solution

Please contact the site administrator.

is included

But one of the remaining tasks is to remove that. So which is it?

Should the HTML be The submitted value <em class="placeholder">FORGERY</em> in <em class="placeholder">Checkboxes</em> element is not allowed.?

With e.g.

\Drupal::messenger()->addStatus(new FormattableMarkup('The submitted value %value in %element element is not allowed.', [
  '%value' => 'FORGERY', 
  '%element' => 'Checkboxes',
]));

Not sure I see the difference?

Hi smustgrave,

Do you mean the issue description is not clear or it looks like was not updated?

#36 the suggestion looks the same as what was implemented or am I wrong?

Hi, @smustgrave. They do not look the same.

The rendered HTML in the patch:

The submitted value <strong><em class="placeholder">FORGERY</em></strong> in <em class="placeholder">Checkboxes</em> element is not allowed

The submitted value "FORGERY" in Checkboxes element is not allowed.

The suggestion in IS is the above

It might be renderred into The submitted value "<em class="placeholder">FORGERY</em>" in <strong><em class="placeholder">Checkboxes</em></strong> element is not allowed

My suggestion:

The submitted value <em class="placeholder">FORGERY</em> in <em class="placeholder">Checkboxes</em> element is not allowed

%variable: Use when the replacement value is to be wrapped in <em> tags.

As %variable is in use, do we really need the extra strong markup?

I would say no, just my opinion though. But the issue summary proposal and patch should line up so moving back to NW for that.

Update the patch to follow the suggestion in IS

After viewing the code I think both of you are right, the text in the summary was not the same than the patch and I think the suggestion from @jungle is good, because is not common in Drupal to make them bold or use quotes in those messages.

I suggest to change the description to remove the quotes and the strong tag as follows.

The submitted value FORGERY in Checkboxes element is not allowed.

@jidrone did the UX review before, so I think his opinion is good. IS Updated. Attaching the new patch per #44

+1 to #46, thanks!

#46 addressed

1. +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormCheckboxesTest.php
   @@ -109,7 +109,7 @@ public function testExposedFormRenderCheckboxes() {
   -    $this->assertSession()->pageTextNotContains('The submitted value in type element is not allowed.');
   +    $this->assertSession()->pageTextNotContains('The submitted value in the Type element is not allowed.');
   

   Meanwhile, changed 'type' to 'Type'. the first letter of the element title should be in uppercase.

2. +++ b/core/modules/views/tests/src/Functional/Plugin/FilterTest.php
   @@ -173,7 +173,7 @@ public function testInOperatorSelectAllOptions() {
   -    $this->assertSession()->pageTextNotContains('The submitted value "page" in type element is not allowed.');
   +    $this->assertSession()->pageTextNotContains('The submitted value "page" in the Type element is not allowed.');
   

   Also here

Thanks @jungle for sticking with this one!

+++ b/core/tests/Drupal/Tests/Core/Form/FormValidatorTest.php
@@ -424,7 +425,7 @@ public function providerTestPerformRequiredValidation() {
-        'An illegal choice has been detected. Please contact the site administrator.',
+        new TranslatableMarkup('The submitted value %choice in the %name element is not allowed.', ['%name' => 'Test', '%choice' => 'baz'], [], $this->getStringTranslationStub()),

@@ -437,7 +438,7 @@ public function providerTestPerformRequiredValidation() {
+        new TranslatableMarkup('The submitted value %choice in the %name element is not allowed.', ['%name' => 'Test', '%choice' => 0], [], $this->getStringTranslationStub()),

@@ -450,7 +451,7 @@ public function providerTestPerformRequiredValidation() {
-        'An illegal choice has been detected. Please contact the site administrator.',
+        new TranslatableMarkup('The submitted value %choice in the %name element is not allowed.', ['%name' => 'Test', '%choice' => 'baz'], [], $this->getStringTranslationStub()),

In general, we should avoid using t() or any of its variants in tests unless those tests are specifically testing translation or multilingual functionality. And, indeed, line 462 of this test already has an example of how to test the expected placeholder markup (Basically, <em class="placeholder">value</em>).

I already made this change locally just to make sure it would work, so the attached updates the patch to fix it.

Forgot to also remove the use statement.

Nice to see this one. One of the most unhelpful error messages in all of Drupal, and surprised this issue is only a year old.

Tagging as a 10.1 string change.

Committed/pushed to 10.1.x, thanks!

+++ b/core/lib/Drupal/Core/Form/FormValidator.php
@@ -345,8 +348,9 @@ protected function performRequiredValidation(&$elements, FormStateInterface &$fo
-            $form_state->setError($elements, $this->t('An illegal choice has been detected. Please contact the site administrator.'));
-            $this->logger->error('Illegal choice %choice in %name element.', ['%choice' => $v, '%name' => empty($elements['#title']) ? $elements['#parents'][0] : $elements['#title']]);
+            $message_arguments = $message_arguments + ['%choice' => $v];
+            $form_state->setError($elements, $this->t($error_message, $message_arguments));
+            $this->logger->error($error_message, $message_arguments);
           }
         }

@longwave pointed out in slack that we're now calling $this->t on a variable, which won't allow potx to pick up the string. Need to remove the variable assignment and duplicate the string I think.

1. +++ b/core/lib/Drupal/Core/Form/FormValidator.php
   @@ -345,8 +347,9 @@ protected function performRequiredValidation(&$elements, FormStateInterface &$fo
   +            $message_arguments = $message_arguments + ['%choice' => $v];
   

   Here it seems simpler to say

   $message_arguments['%choice'] = $v;
   

2. +++ b/core/lib/Drupal/Core/Form/FormValidator.php
   @@ -364,8 +367,9 @@ protected function performRequiredValidation(&$elements, FormStateInterface &$fo
   +        $message_arguments = $message_arguments + ['%choice' => $elements['#value']];
   

   Similarly here

Updated patch #57 by addressing #58, please review it.

Thanks!

+++ b/core/lib/Drupal/Core/Form/FormValidator.php
@@ -335,6 +335,8 @@ protected function performRequiredValidation(&$elements, FormStateInterface &$fo
+      $message_arguments = ['%name' => $name];

Maybe we can also simplify this part to be something like:

$message_arguments['%name'] = $name;

What do think @longwave?

If using $message_arguments['%name'] = $name;

I prefer

$message_arguments = [];
$message_arguments['%name'] = $name;

Or it's over-optimized.

#57 was reviewed by @longwave, the tiny change in #59 looks good. #59 is a RTBC to me.

Yes, you are right, it looks to go.

Agree with @jungle for #60/61 - I prefer $array = [] or $array = ['key' => 'value'] to make it clear you are initialising the array.

Committed to 10.1.x, thanks!

For the record: the usability meeting mentioned in Comment #28 was #3338999: Drupal Usability Meeting 2023-02-10. The attendees were @AaronMcHale, @benjifisher, @jidrone, @rkoller, @schlaukopf, and @simohell. That issue will have a link to the recording.

At the meeting, I made the same point about the two different words spelled "invalid" that @xjm made in #29. I do not have a link, but @rkoller had a reference that suggested avoiding the word. The distinction is clear to those of us who know English well, but perhaps it is more likely to cause confusion for readers who primarily speak a different language.