Problem/Motivation

Currently Drupal.Commenting.InlineComment.InvalidEndChar is ignored if the comment starts with the string cspell. There are two issues with that:

  1. Per the documentation both the strings cSpell and spell-checker are allowed as well, so we should apply the same handling in those cases
  2. If an "actual" comment precedes the cspell comment line, the exception is not triggered. For example the following stanza:
    // Give a reason for the following cspell line.
// cspell:ignore bananarama

    would lead to the $commentText being Give a rason for the following cspell line. cpsell:ignore banarama and that does not start with cspell so it would be raised as an error

Steps to reproduce

Add the following to a file:

// cspell:ignore bananarama

and see that it correctly does not raise a warning.

Add any of the following to a file:

// cSpell:ignore bananarama

// spell-checker:ignore bananarama

// Ignore the word "banarama".
// cspell:ignore bananarama

and see it incorrectly (?!) raises a warning.

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Comments

tstoeckler created an issue. See original summary.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Issue summary: View changes

Oops forgot to link the docs in the issue summary.

quietone’s picture

quietone commented

Just ran into this with core. It is certainly a nuisance that only 1 spelling of cspell is recognized.

quietone’s picture

quietone commented

In core the following are also detected and reported as errors.

// cspell:disable
// cspell:disable-next-line
// cspell:ignore·fffffg
// cspell:ignore·tést·fïle·nàme

This, // cspell:ignore·fffffg, fails when on line 719 on the file but not on line 19. I copied and pasted the line. And similar with cspell:ignore·tést·fïle·nàme it fails on line 101 but passes on line 13. And the // cspell:enable after the disable mentioned about is not reported as an error. But if I move the 'disable' a line down, it is not detected. For these cases, maybe it because the line before is also an in-line comment and coder thinks it is more text.

quietone’s picture

quietone commented
Category: Task » Bug report
Issue tags: +blocker

Since this is detecting valid spellings of cspell as errors as well as valid cspell lines as error, I am changing this to a bug.

It is also blocking core issue #2719663: Fix 'Drupal.Commenting.InlineComment.InvalidEndChar' coding standard

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Active » Fixed

Thanks for reporting, I pushed a fix!

jonathan1055’s picture

jonathan1055 commented
Status: Fixed » Needs work

For reference, this is the commit you made
https://git.drupalcode.org/project/coder/-/commit/b4972e78ac25e514ee00e1...

I think there were actually two problems reported in the issue summary, and only one has been dealt with here.

The change from the specific strpos($commentText, 'cspell:') to the more general preg_match('/(cspell|spell\-checker):ignore/i', $commentText) certainly solves the problem. But this, technically, is too relaxed. It means that if 'cspell:ignore' or 'spell-checker:ignore' are found anywhere in the comment then the comment will be ignored. So if 'cspell:ignore' was put in the middle of a line, say when refering to it in text, that would still be matched. I know this is a rare edge case, so maybe its OK. But is does also mean that the actual real comment text is not checked for the end char. For example

// This comment is now not checked for valid line end char 
// cspell:ignore bananarama

Setting back to 'needs work' so that the issue does not automatically close just yet, to allow the discussion to continue. I can help on the sniff, I am not saying you have to do the work :-)

jonathan1055’s picture

jonathan1055 commented

Also as per the cSpell documentation, we should allow 'spellchecker' without a hyphen. And we should remove 'ignore' from the regex, because this change should also cater for 'disable', 'disable-next-line' and 'enable'. So just having ':' at the end of the regex pattern should be sufficient.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented

Thanks, good points.

For the too relaxed edge case: I think we don't care. I assume it would make the sniff code more complex to detect/split comments. But I'm very happy to review and accept pull requests that try to address this!

For the additional keywords: I think your proposal makes sense to just match for the colon at the end of the regex pattern and add "spellchecker". I will check if I can do that quickly now.

  • klausi committed c58e5a0c on 8.3.x 
    fix(InlineCommen): Allow spellchecker keyword (#3424914)
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Fixed

OK, that was quick and we already changed the regex to only match for the colon.

Closing this as fixed again, please make a new issue if you want to address the too relaxed edge case :-)

Status: Fixed » Closed (fixed)

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