CSpell: Use the "forbidden words" feature to block things like "(web)master", "slave", "whitelist", "blacklist" etc.

[klonos]

This is a follow-up to #4441 and #4451, aiming to stop instances of these words from ever creeping back into our codebase.

See https://cspell.org/docs/forbidden-words on how this works.

[klonos]

It works! 😉 ...the initial PR added these entries in the .cspell/backdrop.dic file:

!webmaster
!master
!slave
!whitelist
!blacklist

I then intentionally added a bunch of these words in a new commit:

• in our README.md file
• in our settings.php file, as an inline comment, a variable, and a couple of array keys.

All these instances were caught in our test (albeit as "unknown words" instead of forbidden words, but hey 🤷🏼 ):

I have double-checked our codebase, and it seems that we have missed numerous instances of forbidden words (which this PR won't catch, since our tests only check changed lines - not the entire codebase each time). I'll add those in the PR.

[klonos]

Should we also add the word drupal in that list? 😝 ...no, seriously though, quite often when I am working on Drupal core backports I might miss a drupal_* function here and there. It would be nice if our tests caught those and warned us.

[klonos]

...OK, I've now included leftover instances of "master" etc. (the majority of them being "display_title": "Default" instances in views config files) that we seem to have missed in #4451. There's now only very few of them remaining, and I've wrapped those that cannot be removed in cspell:disable/cspell:enable statements to have them ignored in our PR tests.

[klonos]

CSpell check passes, but PHPCS unfortunately crashes. The error log is unhelpful though:

Error: Error: Command failed: phpcs --report=json -q --encoding=undefined --standard=../phpcs/Backdrop --runtime-set ignore_errors_on_exit 1 --runtime-set ignore_warnings_on_exit 1 core/includes/bootstrap.inc core/includes/common.inc core/includes/database/database.inc core/includes/file.mimetypes.inc core/modules/comment/tests/comment_views_handler_argument_user_uid.test core/modules/date/tests/date_views.test core/modules/field/tests/field_views.test core/modules/file/file.module core/modules/node/tests/node_views_revision_relations.test core/modules/simpletest/tests/system_test.module core/modules/system/system.module core/modules/taxonomy/tests/taxonomy_views_handler_relationship_node_term_data.test core/modules/user/tests/user_views.test core/modules/user/tests/user_views_argument_default.test core/modules/user/tests/user_views_argument_validate.test core/modules/user/tests/user_views_handler_field_name.test core/modules/user/user.install core/modules/views/tests/handlers/views_handler_argument_string.test core/modules/views/tests/handlers/views_handler_field_bulk_form.test core/modules/views/tests/handlers/views_handler_filter_date.test core/modules/views/tests/handlers/views_handler_manytoone.test core/modules/views/tests/plugins/views_plugin_display.test core/modules/views/tests/styles/views_plugin_style_jump_menu.test core/modules/views/tests/views_access.test core/modules/views/tests/views_argument_default.test core/modules/views/tests/views_argument_validator.test core/modules/views/tests/views_cache.test core/modules/views/tests/views_groupby.test core/modules/views/tests/views_pager.test core/modules/views/tests/views_query.test core/modules/views/tests/views_translatable.test core/modules/views/tests/views_upgrade.test core/modules/views/tests/views_view.test core/modules/views_ui/views_ui.install settings.php

Not sure how to fix this - assuming it is crashing because of the size of the PR (many changed files).

[klonos]

Sorry, I was trying various things with the PR and I've borked it 😬 😅 ...so although the code changes are those intended, there's a lot of commits and reverts now which make it messy. I understand that all of that will be squashed on merge, so it doesn't really matter. The main point however is that despite all the playing around I still wasn't able to figure out why PHPCS is crashing.

I think I'll file a new PR, and will this time be taking it one file at a time to the point where I either I have come to the limit of files that is causing the crash, or perhaps one of the specific changes that does it. Bear with me...

[yorkshire-pudding]

Some feedback just on what is in this issue rather than testing or code review:

Warning: Unknown word ...

seems a bit misleading and won't tell people that actually they shouldn't have it in. If I saw that, I would think it was a problem with the cspell check rather than it was a forbidden word. Surely, if forbidden it should say that?

[klonos]

@yorkshire-pudding yes, that would be ideal, but there is no way that I can see to customize the errors cspell is throwing depending on actual typos vs. forbidden words. So we have two options:

• Don't add these forbidden words, because the errors are misleading. That risks these words creeping back into our codebase though.
• Add the forbidden words, despite the misleading error. That will cause contributors to ask why these words are marked as unknown, in which case we'll simply explain why and point them to policies and past issues etc.

I believe that the benefits of doing the latter outweigh the issue with the misleading cspell error.

[klonos]

The PR is still not ready for review/testing, but I've moved the forbidden words from the .cspell/backdrop.dic dictionary into the .cspell/cspell.json config file, using the flagWords definition. It is now in the following array format:

  "flagWords": [
    "blacklist",
    "master",
    "slave",
    "webmaster",
    "whitelist"
  ],

The good thing is that flagWords supports suggestions (either one or multiple per word), so once I've tested/fixed things, We'll be able to do something like this:

  "flagWords": [
    "blacklist: denylist, deny list, deny, block, denied list, blocked list",
    "blacklisted: disallowed, denied, blocked",
    "blacklisting: disallowing, denying, blocking",
    "master: primary, main, default, base",
    "slave: secondary, replica, fallback",
    "webmaster: admin, administrator",
    "whitelist: allowlist, allow list, allow, allowed list",
    "whitelisted: allowed",
    "whitelisting: allowing"
  ],

[klonos]

I've started a fresh PR, and again PHPCS seems to be chocking when the PR has too many files. It seems that the depth of the files and the length of the overall paths contribute to this, as it results in a long command. This is the error:

Error: Error: Command failed: phpcs --report=json -q --encoding=undefined --standard=../phpcs/Backdrop --runtime-set ignore_errors_on_exit 1 --runtime-set ignore_warnings_on_exit 1 core/includes/bootstrap.inc core/includes/common.inc core/includes/database/database.inc core/modules/comment/tests/comment_views_handler_argument_user_uid.test core/modules/date/tests/date_views.test core/modules/field/tests/field_views.test core/modules/file/file.module core/modules/node/tests/node_views_revision_relations.test core/modules/simpletest/tests/system_test.module core/modules/taxonomy/tests/taxonomy_views_handler_relationship_node_term_data.test core/modules/user/tests/user_views.test core/modules/user/tests/user_views_argument_default.test core/modules/user/tests/user_views_argument_validate.test core/modules/user/tests/user_views_handler_field_name.test core/modules/views/tests/handlers/views_handler_argument_string.test core/modules/views/tests/handlers/views_handler_field_bulk_form.test core/modules/views/tests/handlers/views_handler_filter_date.test core/modules/views/tests/handlers/views_handler_manytoone.test core/modules/views/tests/plugins/views_plugin_display.test core/modules/views/tests/styles/views_plugin_style_jump_menu.test core/modules/views/tests/views_access.test core/modules/views/tests/views_argument_default.test core/modules/views/tests/views_argument_validator.test core/modules/views/tests/views_cache.test core/modules/views/tests/views_groupby.test core/modules/views/tests/views_pager.test core/modules/views/tests/views_query.test core/modules/views/tests/views_translatable.test core/modules/views/tests/views_upgrade.test core/modules/views/tests/views_view.test core/modules/views_ui/views_ui.install settings.php

Perhaps we need to figure out a way to batch the files that we are to pass through PHPCS, or build a temporary list of the files in a .txt file and then point PHPCS to that file. For now, I am going to revert the last commit, when PHPCS started chocking, and lets file two separate PRs. ...or we can simply ignore the PHPCS failure on the tests for this PR 🤷🏼

[klonos]

...testing with a bunch of forbidden words in the README:

So it still shows them as "unknown words", and I am not sure where the suggestions for alternative words should be shown, but we can follow up both of those issues upstream.

[klonos]

...finally!:

[klonos]

@quicksketch I've replaced the multiple instances of cspell:disable/cspell:enable with cspell:disable-next-line. Ready for review when you get a chance.

[avpaderno]

Suggestions are shown when --show-suggestions is passed to the command. (See Getting started / Options.)

[klonos]

@kiamlaluno --show-suggestions is for the command line version of CSpell. We are using GHA version of it, which doesn't support that (yet) AFAIK. Perhaps in a newer version of the CSpell GHA it will be supported though, and we can then benefit from that. Nothing we can do at the moment about it though 🤷🏼

[quicksketch]

@klonos mentioned that same little problem during the weekly dev meeting this past week. For now this is definitely a step forward and once we can start using suggestions that will be even better!

I merged https://github.com/backdrop/backdrop/pull/4553 into 1.x and 1.26.x. Thanks @klonos, @kiamlaluno, and @yorkshire-pudding!

[avpaderno]

@klonos Indeed! It is just a matter of waiting for the GHA to support that option. (I hoped it would already support it, but apparently it is not yet supported, or it is not documented.)

[klonos]

FTR, I have raised that here: https://github.com/streetsidesoftware/cspell-action/issues/1455