Remove "Please" from the codebase

Searching the D10.1.x repo and found 44 instances of the word "please"
At this time this we will need a D10 version of the patch.

We might also be able to use cspell to prevent this from creeping back in again.

Using cspell seems like a nice simple way to move forward. Let's see what using cpell would look like.

I added please to the 'flagwords' and ran spellcheck:core. The results are in the attached files. There are 139 instances of 'please' in 84 files. I have not looked at the files.

This now needs a reroll, that includes this change to cspell. I have updated the Issue Summary with details.

Added The Patch As per suggestion #18 and Remove all "Please" Word As Per #18 "spellcheck.txt".

There’s no interdiff in #19 and with all those changes it probably needs one. Keeping for reroll tag for the next person

Here is a rerolled patch, addressing the pointers from #18.

Addressed all the files shared in spellcheck.txt from #18, expect these, as this issue is focuses on removing 'Please' word only:

/var/www/html/core/tests/Drupal/KernelTests/Core/Config/ConfigCRUDTest.php:174:17 - Unknown word (derp)
/var/www/html/core/misc/cspell/dictionary.txt.rej:7:2 - Unknown word (getcode)
/var/www/html/core/misc/cspell/dictionary.txt.rej:8:2 - Unknown word (getfile)
/var/www/html/core/misc/cspell/dictionary.txt.rej:16:2 - Unknown word (herpderp)
/var/www/html/core/misc/cspell/dictionary.txt.rej:20:2 - Unknown word (inator)
/var/www/html/core/misc/cspell/dictionary.txt.rej:33:2 - Unknown word (vals)
/var/www/html/core/misc/cspell/dictionary.txt.rej:34:23 - Unknown word (viewsviewfiles)
/var/www/html/core/misc/cspell/dictionary.txt.rej:39:2 - Unknown word (vxezb)
/var/www/html/core/misc/cspell/dictionary.txt.rej:40:2 - Unknown word (vxfbk)

Thanks!

Thank you @ankithashetty for all the work.

Applied the patch and I still see the word please 14 times.

Not sure if the migrate ones need to stay

And for the ckeditor5 one not sure if you can replace but have to build the plugins again which might be the error we seeing in the build

Some review comments:

1. +++ b/core/INSTALL.sqlite.txt
   @@ -34,6 +34,6 @@ following in your "Database file" field:
   +downloaded. Check that the file is, indeed, protected by your webserver.
   +If not, Consult the documentation of your webserver on how to protect a
   

   This needs rewrapping and "consult" should not be capitalised.

2. +++ b/core/assets/scaffold/files/drupal.INSTALL.txt
   @@ -1,3 +1,3 @@
   +Read core/INSTALL.txt for detailed installation instructions for your
    Drupal website.
   

   This needs rewrapping.

3. +++ b/core/lib/Drupal/Core/Extension/module.api.php
   @@ -224,7 +224,7 @@ function hook_modules_installed($modules, $is_syncing) {
   + * Be sure that anything added or modified in this function that can
     * be removed during uninstall should be removed with hook_uninstall().
   

   Not sure this sentence makes sense, but perhaps this is out of scope. This needs rewrapping anyway.

4. +++ b/core/lib/Drupal/Core/Render/Element/PathElement.php
   @@ -63,7 +63,7 @@ public static function validateMatchedPath(&$element, FormStateInterface $form_s
   -          $form_state->setError($element, t('You cannot use an external URL, please enter a relative path.'));
   +          $form_state->setError($element, t('You cannot use an external URL, enter a relative path.'));
   

   This is probably better off as two sentences: "You cannot use an external URL. Enter a relative path."

5. +++ b/core/lib/Drupal/Core/Render/theme.api.php
   @@ -1184,7 +1184,7 @@ function hook_page_bottom(array &$page_bottom) {
   + *     set, this can be set to true. Keep in mind that when this is used
     *     by a theme, that theme becomes responsible for making sure necessary
     *     variables are set.
   

   This needs rewrapping.

6. +++ b/core/misc/ajax.js
   @@ -387,7 +387,7 @@
   -        message: Drupal.t('Please wait...'),
   +        message: Drupal.t('Wait...'),
   

   I am not sure that "Wait..." makes much sense on its own, I feel users are used to seeing "Please wait", so not sure what to do here.

7. +++ b/core/misc/drupal.js
   @@ -486,11 +486,11 @@ window.Drupal = { behaviors: {}, locale: {} };
   +   *   The string for the singular case. Make sure it is clear this is
       *   singular, to ease translation (e.g. use "1 new comment" instead of "1
       *   new"). Do not use @count in the singular string.
   

   This needs rewrapping.

8. +++ b/core/misc/drupal.js
   @@ -486,11 +486,11 @@ window.Drupal = { behaviors: {}, locale: {} };
   +   *   The string for the plural case. Make sure it is clear this is
       *   plural, to ease translation. Use @count in place of the item count, as in
       *   "@count new comments".
   

   And again.

9. +++ b/core/modules/file/file.module
   @@ -134,10 +134,10 @@ function file_validate_name_length(FileInterface $file) {
   +    $errors[] = t("The file's name is empty. Give a name to the file.");
   

   "Give a name to the file" doesn't read well. Maybe "Ensure the file has a name."? Though, I'm not sure how this could even happen.

10. +++ b/core/modules/file/tests/file_module_test/src/Form/FileModuleTestForm.php
    @@ -42,7 +42,7 @@ public function buildForm(array $form, FormStateInterface $form_state, $tree = T
    +      '#progress_message' => $this->t('Wait...'),
    

    See above comments on "Please wait", we should be consistent whatever we choose.

11. +++ b/core/modules/jsonapi/src/ResourceType/ResourceTypeRepository.php
    @@ -453,7 +453,7 @@ protected function getRelatableResourceTypesFromFieldDefinition(FieldDefinitionI
    +            'The "%s" at "%s:%s" references the "%s:%s" entity type that does not exist. Take action.',
    

    I think we should remove "Take action" entirely, as it doesn't give me any clues as to what action to take.

12. +++ b/core/modules/system/src/Form/ModulesUninstallConfirmForm.php
    @@ -143,7 +143,7 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    +      $this->messenger()->addError($this->t('The selected modules could not be uninstalled, either due to a website problem or due to the uninstall confirmation form timing out. Try again.'));
    

    Not sure "Try again" is necessary, the user might not want to try again.

13. +++ b/core/modules/user/src/Controller/UserController.php
    @@ -248,7 +248,7 @@ public function resetPassLogin($uid, $timestamp, $hash, Request $request) {
    +    $this->messenger()->addStatus($this->t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Set your password.'));
    

    "Set your password" feels a bit too direct but not sure what would improve it.

14. +++ b/core/modules/views/tests/src/Functional/Plugin/ExposedFormCheckboxesTest.php
    @@ -158,14 +158,14 @@ public function testExposedIsAllOfFilter() {
    +    $this->assertSession()->pageTextNotContains('An illegal choice has been detected. contact the site administrator.');
    ...
    +    $this->assertSession()->pageTextNotContains('An illegal choice has been detected. contact the site administrator.');
    

    contact -> Contact

15. +++ b/core/tests/Drupal/Tests/Traits/PhpUnitWarnings.php
    @@ -32,7 +32,7 @@ trait PhpUnitWarnings {
    -    'The at() matcher has been deprecated. It will be removed in PHPUnit 10. Please refactor your test to not rely on the order in which methods are invoked.',
    +    'The at() matcher has been deprecated. It will be removed in PHPUnit 10. Refactor your test to not rely on the order in which methods are invoked.',
    

    This comes from PHPUnit itself, so we will have to ignore this instance of "please" by adding // cspell:ignore-next-line above it.

Also see changes to composer files is that something we should avoid?

No, those are our Composer plugins and extensions, those are OK to modify.

I think I did everything in #24, except for 6 and 10.

I also ran spellcheck on core and added changes for options_test.module and OptionsWidgetsTest.php.

Adding tag because cspell is being used to fix this.

Appears to have a build failure.

But applied patch #27 and searched for "please"

'You no longer please the robot overlords. Go to your room and chill out

This was good and whoever wrote that kudos haha

There is an instance of please in migrate_drupal test fixture drupal6.php.

Providing patch for review.

@Ashutosh Ahirwal, Thank you for your assistance on this issue!
When making changes to a patch always add an interdiff so reviewers can see what has been changed. See Creating an interdiff.
Starting March 2023, simple rerolls, rebases, or merges will no longer receive issue credit. Only rerolls that address a merge conflict will be credited, and the merge conflict that was resolved must be documented in the text of an issue comment.
To receive credit for contributing to this issue, assist with other outstanding tasks or unaddressed feedback.
See the issue credit guidelines for more information.

I am uploading an interdiff.

This issue is using cspell to find instances of "Please" so that should be used to find remaining instances and not grep or ag. Cspell has a defined set of ignored paths in cspell.json. The changes in the latest patch are to ignored files and directories and also reverting the fix made in PhpUnitWarnings (#24-15). See #3122088: [Meta] Remove spelling errors from dictionary.txt and fix them for working with cspell.

I think we need to restart from #27 and check what still needs to be done in #24.

I rewrapped a few lines and made a change for a failing test.

#24
13 - The change needs to be agreed to.
6, 10 - Todo. This is the 'Please wait..." text. It appears in ajax.js, FileModuleTestForm.php, media_library.ui.js and theme-settings.php

Here is the interdiff I forgot to upload in #30.

Maybe this needs a yarn:build first.

Changes to fix the errors.

Looks good.

@VladimirAus, thanks. Did you see the questions in #31 and the issue summary?

I asked in #ux about #24.6 and 10. benjifisher suggested with 'Processing' and dww suggested several humorous options including 'Wait!'. Instead of those I changed the patch to use 'Processing...'.

+++ b/core/modules/user/src/Controller/UserController.php
@@ -248,7 +248,7 @@ public function resetPassLogin($uid, $timestamp, $hash, Request $request) {
-    $this->messenger()->addStatus($this->t('You have just used your one-time login link. It is no longer necessary to use this link to log in. Please set your password.'));
+    $this->messenger()->addStatus($this->t('You have just used your one-time login link. It is no longer necessary to use this link to log in. It is recommended that you set your password.'));

And do we agree to this change?

My Canon printer says: "Please wait momentarily" 🖨️ 😆
But few suggestion I got from other projects:

• Processing request...
• Loading data, this may take a moment...
• Calculating results, almost there...

But the best I think was "Wait a moment..." instead of "Please wait..." to address 24.6 and 24.10

Moved to MR.

I am tagging this for a Usability review to confirm the changes in text, particularly the 'Please wait...'.

I am a bit confused here. The MR has removed a change recommended by an active member doing usability reviews, which I think we should follow. The change to ajax.js changed one occurrence of the original 'Please wait' string and not the other without explanation. The MR has also introduced changes to the migrate test fixtures which should not be changed. I have updated the Issue Summary to make that clear.

As far as I can tell the patch in #38 is what should be reviewed.

Applying #38 and searching for please. Only occurrences I found came from node_modules (local install), ckeditor5, and text fixtures.

So assuming that's fine.

Added // please to a random file and ran the code-commit script and cspell correctly caught that as a forbidden word..

In #38, @quietone mentioned that we discussed this issue on Slack. Here is the reason I gave for suggesting "Processing" rather than "Wait":

I would rather convey information than tell or request the user to do something.
If this is a generic message for processing a form, then I would use something like

Processing ...

or else a full sentence:

The form is processing.

If we can be more specific, then we should.

Personally, I get a little annoyed when my machines tell me what to do (wait) even if they try to be polite about it (please wait).

Usability review

We reviewed this issue at #3357221: Drupal Usability Meeting 2023-05-05, that issue will have a link to the recording.

For the record, the attendees at today's usability meeting were @AaronMcHale, @benjifisher, @rkoller, @simohell, and @yoroy.

The group is supportive of this issue and endorses the effort to remove the word "please".

The group previously reviewed #2969406: Fix incorrect message after resetting password and made a recommendation on what the wording should be for the status message after clicking a one-time login link. See that issue for more rational behind the specific wording of the recommendation. That recommendation is:

"You have logged in with your one-time login link. Set your new password now, it is not possible to use this link a second time."

The group recommends keeping that issue open as it is at RTBC, and recommends leaving it to the core committers to decide which order to commit these two issues in. If this issue is committed first, then that one will need to be rerolled, if that issue is committed first then this issue will need to remove any changes to that message.

I am adding a link to #2969406: Fix incorrect message after resetting password in the "Remaining tasks" section.

Despite the MR displaying as green above, it is in fact red: https://www.drupal.org/pift-ci-job/2633148

Looks like new coverage from #3354606: Datetime field name missing from validation error message is affected by the changes here.

Fixed DateTimeFieldTest.

@dpi, thanks for checking on this issue. However, the Issue Summary states that is is the patch in #38 that is to be reviewed, not the MR.

Now, it turns out that the patch does not apply anymore. I am setting this to NW and assigning to myself to sort out later.

To me it looks like the MR is the more up to date one. I have hidden all the patches and completed the task, confirming that the change from #24.13 is still in the MR. I think the MR is ready to commit but a final review would be welcome.

The MR may be later but it introduced errors. For one, it is making changes to the drupal7.php migration test fixture. It is also changing the recommendations from the UX review. I think all these are noted in the 5 unresolved comments on the MR.

This rerolls the patch from #38.

Uploading the diff (which I thought I did).

Now repeat #52

Applying the patch and searching for "please" seems there are some new ones to replace

web/core/modules/media_library/src/Form
web/core/modules/sdc/src/Twig
web/core/modules/sdc/tests/src/Kernel
web/core/tests/Drupal/Tests/Traits

Ignored the matches from vendor and migrate tests.

I have updated the patch with latest update suggested by @smustgrave, I would appreciate if someone can review these updates and provide feedback

Think this round is good to go.

#60 doesn't apply to 11.x.

Here is the re-roll.

But I discovered there is more work to do here. \Drupal\Tests\options\Functional\OptionsFieldUITest has three asserts as shown.

$assert_session->pageTextNotContains('Please wait');
Now that Please has been removed this will always pass.

I think the string should be changed to 'Processing...'.

Searching for "please" I only find references in fixtures and Migrate tests which I'm assuming is fine

Rerolled.

There a chance this could get merged? Seems it stays needing to be rerolled

+++ b/core/tests/Drupal/Tests/Traits/PhpUnitWarnings.php
@@ -32,7 +32,8 @@ trait PhpUnitWarnings {
+    'The at() matcher has been deprecated. It will be removed in PHPUnit 10. Refactor your test to not rely on the order in which methods are invoked.',

This line can't change, because the message comes from upstream PHPUnit (this is why we added cspell:disable-next-line above it).

Actually, to save this going round again, I can just fix this on commit:

--- a/core/tests/Drupal/Tests/Traits/PhpUnitWarnings.php
+++ b/core/tests/Drupal/Tests/Traits/PhpUnitWarnings.php
@@ -33,7 +33,7 @@ trait PhpUnitWarnings {
     'assertDirectoryNotIsWritable() is deprecated and will be removed in PHPUnit 10. Refactor your code to use assertDirectoryIsNotWritable() instead.',
     'assertFileNotIsWritable() is deprecated and will be removed in PHPUnit 10. Refactor your code to use assertFileIsNotWritable() instead.',
     // cspell:disable-next-line
-    'The at() matcher has been deprecated. It will be removed in PHPUnit 10. Refactor your test to not rely on the order in which methods are invoked.',
+    'The at() matcher has been deprecated. It will be removed in PHPUnit 10. Please refactor your test to not rely on the order in which methods are invoked.',
     // PHPUnit 9.6.
     'Expecting E_WARNING and E_USER_WARNING is deprecated and will no longer be possible in PHPUnit 10.',
     'Expecting E_ERROR and E_USER_ERROR is deprecated and will no longer be possible in PHPUnit 10.',

This is not eligible for backport to 10.1.x due to user-facing string changes.

Committed 224c673 and pushed to 11.x. Thanks!

@longwave, thank you for fixing that on commit!

This needs a followup because spellcheck was not run on core. Followup, #3357565: Remove remaining uses of string 'bartik' and 'seven' when referring to the removed themes

FYI: this is disruptive for contrib. It caused the test suite for https://www.drupal.org/project/big_pipe_sessionless to fail, because it was explicitly testing an edge case that resulted in a call to _drupal_log_error(), which generated a message that includes this "please".