Problem/Motivation

#2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush was RTBC for a little while with a deprecation message that said:

Pushing requests without a session onto the request_stack ist deprecated in drupal 10.1.0 and an error will be thrown from drupal 11.0.0. See https://www.drupal.org/node/3337193.

The test bot and core/scripts/dev/commit-code-check.sh were both happy with this, but "ist" isn't a word. I had to be paying close attention and flagged it manually.

ist is not listed anywhere in core/misc/cspell/dictionary.txt. Really not sure how this is passing.

My only theory is that cspell is doing some stemming and thinks that since "ist" is a valid suffix to add on other words (e.g. "social" vs. "socialist"), if it sees the suffix on its own, everything is okay? Or something?

Steps to reproduce

  1. Checkout 10.1.x core
  2. Put 'ist' anywhere in a string literal or comment in a file.
  3. git add the file.
  4. Run core/scripts/dev/commit-code-check.sh --cached
  5. Be amazed to find: "CSpell: passed"

Proposed resolution

Probably we should add 'ist' to the flagWords section of core/.cspell.json.

Maybe we should try to fix this upstream in cspell itself?

Remaining tasks

  1. Figure out why this is happening (if possible).
  2. Decide if we should use flagWords to solve this, or try to fix cspell itself upstream or something.
  3. Documentation updates

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#15 three-letter-word.txt10.84 KBquietone
#2 3355243-2.patch267 bytesdww

Issue fork drupal-3355243

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

dww created an issue. See original summary.

dww’s picture

dww
we/he/they
commented
Status: Active » Needs review
Issue tags: +Bug Smash Initiative
StatusFileSize
new 3355243-2.patch267 bytes

Adding to flagWords for now. Let's see if this breaks anything.

dww’s picture

dww
we/he/they
commented
Title: cspell doesn't think 'ist' is a typo » cspell does not flag "ist" as a typo
dww’s picture

dww
we/he/they
commented

With the patch applied, repeated the steps from the summary and now get:

1/1 ./authorize.php 633.87ms X
/.../core/authorize.php:45:34 - Forbidden word (ist)
CSpell: Files checked: 1, Issues found: 1 in 1 files

CSpell: failed

So that would have caught the bug in #2484991: Add the session to the request in KernelTestBase, BrowserTestBase, and drush. 🎉

dww’s picture

dww
we/he/they
commented
Issue summary: View changes

Yay, bot is happy. So we’re not relying on “ist” to work anywhere else.

Fixing summary to move all the problem/motivation text together.

quietone’s picture

quietone commented

I also tested this. I added "ist" to a file. I also deleted all the dictionaries from cspell.json just to see if 'ist' was a word in any of those dictionaries. I then ran commit-code-check and 'ist' was not reported, unlike all the Drupalisms in the file I changed.

Yes, upstream sounds good.

Edit: dww said this comment was confusing. I am correctly grammar, spelling and adding detail.

dww’s picture

dww
we/he/they
commented

Thanks for clarifying, @quietone. However, not sure why you came to this conclusion:

Yes, upstream sounds good.

vs. adding "ist" to flagWords and being done (per the patch).

Fixing this upstream sounds like a long, painful process. 😬 Adding another entry to flagWords is trivial (and done). 😅 That seems to be exactly what flagWords is for: always flag things in that list, even if cspell itself thinks they're okay (for whatever reason).

andypost’s picture

andypost
he/him
Primary language Russian
commented

It still makes sense to notify upstream

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Needs Review Queue Initiative

Followed the same steps.

Added // ist to a file and ran commit-code-check with 0 issues.

Applied patch and ran the check and it was caught.

quietone’s picture

quietone commented

@dww, I mean that filing an issue upstream makes sense.

dww’s picture

dww
we/he/they
commented

Needs followup https://github.com/streetsidesoftware/cspell/issues/4432 😉

quietone’s picture

quietone commented
Category: Bug report » Task
Issue summary: View changes

@dww, thanks for your patience with my communication challenges on this issue.

There was a response in the cspell issue which suggested using 'trace' to find which dictionary is using the word. It is 'cpp'.

$ yarn cspell trace ist
yarn run v1.22.19
--- trimmed
ist  * cpp                  node_modules/@cspell/dict-cpp/cpp.txt.gz
--- trimmed

But that dictionary is not enabled in cspell.json and if it were there were be a '*" after the dictionary name. It is more likely this is because of the default word length of 4. And that means this is not bug. Therefor, I am changing this to a task. Adding 'documentation updates' to the remaining task so the information on word length is added to the wiki page.

Then, should this change be made? Should spelling errors in words that are less that 4 characters always be put into flagWords? It makes sense for common typos like the current flagWord 'hte'. But is 'ist' a common mistake?

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

There are comments on the upstream issue, do we want to reduce the minimum word length to three (seems like it would start catching lots of acronyms) or should we add a suggestion to flagWords (which seems like a good idea).

quietone’s picture

quietone commented

Per the suggestion in the cspell issue I added "minWordLength": 3, to cspell.json. I then rebuilt the dictionary and have uploaded the results. There were 271 insertions(+), 2 deletions(-) to the dictionary. I don't see any reason to add that extra work. So, I agree with catch that we stay with the default minimum of 4 letter word.

As for using the suggestion, flagWord: ist->is. First, I tried it with "hte->xyz" and made a spelling error in a file. The result was Forbidden word (hte) fix: (xyz) in the output. It is nice that there is a suggestion but someone still needs to read the code to be sure, so I'd rather not have a suggestion at all.

I think that answers the questions above.

quietone’s picture

quietone commented
StatusFileSize
new three-letter-word.txt10.84 KB

Forgot to upload the file.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Can we have a test run with new option and fixed vocabulary

quietone’s picture

quietone commented

@andypost, I am not sure what you mean by 'new option'. Do you mean changing "ist" to "ist->is"?

Also, there is no vocabulary to fix here, this is just adding one line to flag words. There are also no changes to the dictionary.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Based on @quietones research in #14 seems patch #2 is still the preferred solution.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 2: 3355243-2.patch, failed testing. View results

Version: 10.1.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch, which currently accepts only minor-version allowed changes. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

voleger made their first commit to this issue’s fork.

voleger opened merge request !4522

voleger’s picture

voleger
he/his
Primary language Ukrainian
Location Ukraine, Rivne
commented
Status: Needs work » Reviewed & tested by the community

Unrelated failed testing.
I rerolled the patch in the MR as the affected list by the patch was updated.
Setting back to RTBC as agreed on the solution by the reviewers.

  • longwave committed 7bb67dde on 11.x 
    Issue #3355243 by dww, quietone, smustgrave: cspell does not flag "ist"...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 11.x.

Does not backport cleanly to 10.1.x, not sure it really matters given we should catch any spelling issues in 11.x.

Status: Fixed » Closed (fixed)

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

voleger closed merge request !4522