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

Created on 21 February 2022, over 2 years ago
Updated 17 February 2023, over 1 year ago

Problem/Motivation

In certain situations where validation fails, Core displays an error message:

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

This message can be confusing to users in particular contexts, because of the use of the word 'illegal'. It implies the user has broken the law, which obviously they have not.

Steps to reproduce

Enable test modules by adding

$settings['extension_discovery_scan_tests'] = TRUE;

to settings.php. Alternatively, copy core/modules/system/tests/modules/form_test to modules/.

  1. Install form_test.
  2. Go to /form-test/input-forgery.
  3. Check both boxes and submit the form to see the error.

Proposed resolution

Change the message to read something more descriptive:

The submitted value FORGERY in Checkboxes element is not allowed.

-- See #44

Remaining tasks

  • Get RTBC.

API changes

NA

Data model changes

NA

Release notes snippet

📌 Task
Status

Fixed

Version

10.1

Component
UI text 

Last updated 3 days ago

No maintainer
Created by

🇬🇧United Kingdom nicrodgers Monmouthshire, UK

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • 🇺🇸United States smustgrave

    Tagging for usability review, will slack them also

    In the meantime can the MR please be updated for 10.1

  • 🇮🇳India kkalashnikov Ghaziabad, India

    Patch for Drupal version 10.1.x

  • 🇮🇳India kkalashnikov Ghaziabad, India
  • 🇬🇧United Kingdom nicrodgers Monmouthshire, UK

    Thanks @kklalashnikov for the re-roll.

    I've applied the patch and searched the codebase for the original string. It is still in the following places and needs updating:

    core/modules/block/tests/src/FunctionalJavascript/BlockAddTest.php:    $assert_session->pageTextNotContains('An illegal choice has been detected. Please contact the site administrator.');
    core/modules/comment/tests/src/Functional/CommentFieldsTest.php:    $this->assertSession()->pageTextNotContains('An illegal choice has been detected. Please contact the site administrator.');
    
    
  • Status changed to Needs review over 1 year ago
  • 🇧🇷Brazil lucassc Rio de Janeiro

    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.

  • Status changed to Needs work over 1 year ago
  • 🇬🇧United Kingdom nicrodgers Monmouthshire, UK

    Thanks for updating those remaining strings, lucassc. However, I do not think the comma after "Please" is correct. It reads much better without it. Setting back to NW to revert the comma change.

  • 🇮🇳India Akram Khan Cuttack, Odisha

    Added patch consider #12

  • 🇬🇧United Kingdom nicrodgers Monmouthshire, UK

    There are unrelated composer changes in the patch in #14

  • Status changed to Needs review over 1 year ago
  • 🇧🇷Brazil lucassc Rio de Janeiro

    Hi, nicrodgers! Thanks for review.

    In #11 I already added an alternative patch without the commas and its interdiff with patch #9.

    Please, review.

  • Status changed to Needs work over 1 year ago
  • 🇬🇧United Kingdom nicrodgers Monmouthshire, UK

    Reviewing the file no_commas-3265616-11.patch from #11, it looks good apart from these two lines:

    +++ b/core/modules/block/tests/src/FunctionalJavascript/BlockAddTest.php
    @@ -44,7 +44,7 @@ public function testBlockAddThemeSelector() {
    +    $assert_session->pageTextNotContains('An invalid choice has been detected. Please contact the site administrator..');
    
    +++ b/core/modules/comment/tests/src/Functional/CommentFieldsTest.php
    @@ -183,7 +183,7 @@ public function testCommentFieldCreate() {
    +    $this->assertSession()->pageTextNotContains('An invalid choice has been detected. Please contact the site administrator..');
    

    An extra full stop has appeared here.

  • Status changed to Needs review over 1 year ago
  • 🇧🇷Brazil lucassc Rio de Janeiro

    ops, damn ADHD haha

    extra full stops removed.

  • 🇧🇷Brazil lucassc Rio de Janeiro

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

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    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.

  • Status changed to Needs review over 1 year ago
  • 🇧🇷Brazil lucassc Rio de Janeiro

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

    Please review.

  • 🇬🇧United Kingdom longwave UK

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

  • Status changed to RTBC over 1 year ago
  • 🇺🇸United States smustgrave

    Thanks for making those changes!

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States xjm

    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!

  • Status changed to Needs review over 1 year ago
  • Addressed the requirements as per #24 (Updated the Menu UI module inline comment)

  • 🇺🇸United States benjifisher Boston area

    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.

  • 🇺🇸United States xjm

    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.

  • Status changed to Needs work over 1 year ago
  • 🇨🇴Colombia jidrone

    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

  • 🇺🇸United States xjm

    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!

  • Status changed to Needs review over 1 year ago
  • 🇨🇴Colombia jidrone

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

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States xjm

    The followup issue has been filed at 🌱 [meta] Remove other uses of “illegal” from core Active -- 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.
    
  • Status changed to Needs review over 1 year ago
  • 🇨🇳China jungle Chongqing, China

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

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    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?

  • Status changed to Needs review over 1 year ago
  • 🇨🇳China jungle Chongqing, China

    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',
    ]));
    

  • 🇺🇸United States smustgrave

    Not sure I see the difference?

  • 🇨🇴Colombia jidrone

    Hi smustgrave,

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

  • 🇺🇸United States smustgrave

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

  • 🇨🇳China jungle Chongqing, China

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

    The format 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 format in IS the above

    It might be formatted 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 tags.

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

  • Status changed to Needs work over 1 year ago
  • 🇺🇸United States smustgrave

    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.

  • Status changed to Needs review over 1 year ago
  • 🇨🇳China jungle Chongqing, China

    Update the patch to follow the suggestion in IS

  • 🇨🇳China jungle Chongqing, China
  • 🇨🇴Colombia jidrone

    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.

  • 🇨🇳China jungle Chongqing, China

    @jidrone did the UX review before, so I think his option is good to go. IS Updated. attach the new patch per #44

  • Status changed to Needs work over 1 year ago
  • 🇬🇧United Kingdom nicrodgers Monmouthshire, UK

    As the original creator of this issue, I like the way this has improved since the UX review, however I do question the wording of the new message - it doesn't read very well at the moment. Adding in makes the whole thing much better:

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

    Obviously 'the' shouldn't be in bold, I have highlighted it to make it clearer what I am suggesting should be added.

  • Status changed to Needs review over 1 year ago
  • 🇨🇳China jungle Chongqing, China

    +1 to #46, thanks!

    #46 addressed

  • 🇨🇳China jungle Chongqing, China
    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

  • Status changed to RTBC over 1 year ago
  • 🇺🇸United States smustgrave

    Thanks @jungle for sticking with this one!

  • 🇺🇸United States xjm
    +++ 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 (TLDR, <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.

  • 🇺🇸United States xjm

    Forgot to also remove the use statement.

  • Status changed to Fixed over 1 year ago
  • 🇬🇧United Kingdom catch

    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!

    • catch committed 66c80122 on 10.1.x
      Issue #3265616 by jungle, lucassc, xjm, nicrodgers, kkalashnikov, Akram...
  • 🇬🇧United Kingdom catch
    • catch committed a386abdf on 10.1.x
      Revert "Issue #3265616 by jungle, lucassc, xjm, nicrodgers, kkalashnikov...
  • Status changed to Needs work over 1 year ago
  • 🇬🇧United Kingdom catch

    @longwave pointed out in slack the

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

  • Status changed to Needs review over 1 year ago
  • 🇨🇳China jungle Chongqing, China
  • Status changed to Needs work over 1 year ago
  • 🇬🇧United Kingdom longwave UK
    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

  • Status changed to Needs review over 1 year ago
  • 🇮🇳India mrinalini9 New Delhi

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

    Thanks!

  • 🇨🇴Colombia jidrone
    +++ 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?

  • Status changed to RTBC over 1 year ago
  • 🇨🇳China jungle Chongqing, China

    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. #58 is a RTBC to me.

  • 🇨🇴Colombia jidrone

    Yes, you are right, it looks to go.

    • longwave committed 3789efb3 on 10.1.x
      Issue #3265616 by jungle, lucassc, xjm, nicrodgers, kkalashnikov, Akram...
  • Status changed to Fixed over 1 year ago
  • 🇬🇧United Kingdom longwave UK

    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!

  • 🇺🇸United States benjifisher Boston area

    For the record: the usability meeting mentioned in Comment #28 was 📌 Drupal Usability Meeting 2023-02-10 Fixed . The attendees were @AaronMcHale, @benjifisher, @jidrone, @rkoller, @schlaukopf, and @simohell.

    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.

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

Production build 0.69.0 2024