Problem/Motivation

Currently any patch that touches core/modules/views/src/Plugin/views/filter/FilterPluginBase.php will fail TestBot due to cspell not liking $wrapper_identifer any more. (Seems not to happen when testing on 9.2.x.

For example see: https://www.drupal.org/pift-ci-job/2053136

Steps to reproduce

Alter something in core/modules/views/src/Plugin/views/filter/FilterPluginBase.php
Upload Patch
CLUNK

Proposed resolution

Rename $wrapper_identifer to $wrapper_identifier and rainbows and unicorns rule the universe once more.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3212521

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

Spokje created an issue. See original summary.

spokje’s picture

spokje
Primary language Dutch
commented
Status: Active » Needs review

It would be nice if this could be reviewed and committed to 9.3.x quickly, since I think a lot of patches will fail the Custom Command phase, which has the added bonus it won't change the issue status back to Needs Work when it fails, see #3208931: TestBot returns with "Build Successful" when there are too many test failures to report.

Spokje opened merge request !640

longwave’s picture

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

Unsure how this got past cspell, maybe the daily branch tests need a full cspell run?

spokje’s picture

spokje
Primary language Dutch
commented

Unsure how this got past cspell, maybe the daily branch tests need a full cspell run?

Only the changed files are checked by cspell and phpcs, so somewhere cspell got stricter but this particular file was never touched, so nobody noticed before.

It _might_ be a good idea for a follow-up to do a full run on all files with phpcs and cspell to see what else fails indeed.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Ahh the cspell dictionaries were updated in #3211810: [security] Update Nightwatch and locked dev dependencies to address security issues, that must be the cause.

spokje’s picture

spokje
Primary language Dutch
commented
Related issues: +#3211810: [security] Update Nightwatch and locked dev dependencies to address security issues

Ran ../vendor/bin/phpcs -s on 9.3.x with core/phpcs.xml.dist copied to core/phpcs.xml without an error/warning-message, except for

------------------------------------------------------------------------------------------------------------------------------------------------------------------
FOUND 0 ERRORS AND 1 WARNING AFFECTING 1 LINE
------------------------------------------------------------------------------------------------------------------------------------------------------------------
 1 | WARNING | Remove "version" from the info file, it will be added by drupal.org packaging automatically (Drupal.InfoFiles.AutoAddedKeys.Version)
------------------------------------------------------------------------------------------------------------------------------------------------------------------

which we can ignore,

Which is as expected due to the way we add new sniffs.

Seeing the above comment I'll try to get cspell running over all files in that branch next

  • catch committed f09730b on 9.3.x 
    Issue #3212521 by Spokje, longwave: cspell dislikes identifer in core/...
catch’s picture

catch
he/him
Primary language English
commented
Title: cspell dislikes identifer in core/modules/views/src/Plugin/views/filter/FilterPluginBase.php and will fail any patch touching that file » [backport] cspell dislikes identifer in core/modules/views/src/Plugin/views/filter/FilterPluginBase.php and will fail any patch touching that file
Version: 9.3.x-dev » 9.2.x-dev

Committed/pushed to 9.3.x, thanks!

Do we want to backport this to 9.2.x anyway despite cspell apparently not complaining?

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

Yes:

$ git checkout 9.2.x
$ git pull
$ yarn install
...
$ yarn spellcheck modules/views/src/Plugin/views/filter/FilterPluginBase.php
yarn run v1.22.10
$ cspell modules/views/src/Plugin/views/filter/FilterPluginBase.php
.../core/modules/views/src/Plugin/views/filter/FilterPluginBase.php:966:29 - Unknown word (identifer)
.../core/modules/views/src/Plugin/views/filter/FilterPluginBase.php:969:57 - Unknown word (identifer)
.../core/modules/views/src/Plugin/views/filter/FilterPluginBase.php:973:31 - Unknown word (identifer)
.../core/modules/views/src/Plugin/views/filter/FilterPluginBase.php:974:22 - Unknown word (identifer)
CSpell: Files checked: 1, Issues found: 4 in 1 files

  • catch committed 5ddbcc7 on 9.2.x 
    Issue #3212521 by Spokje, longwave: cspell dislikes identifer in core/...
catch’s picture

catch
he/him
Primary language English
commented
Version: 9.2.x-dev » 9.1.x-dev

Went ahead and backported, even in the unlikely event we have to release a hotfix for the alpha, we'd probably want a full cspell run to pass.

Same problem on 9.1.x but will leave that until we unfreeze.

spokje’s picture

spokje
Primary language Dutch
commented

Follow-up in #3212547: cspell Dictionaries changed, checking all files

spokje’s picture

spokje
Primary language Dutch
commented

I see there are commits again on 9.1.x (https://git.drupalcode.org/project/drupal/-/commits/9.1.x)

Do we still want to backport this one? Or wait until #3212547: cspell Dictionaries changed, checking all files which might/might not fix this as well?

  • catch committed 9fc59b1 on 9.1.x 
    Issue #3212521 by Spokje, longwave: cspell dislikes identifer in core/...
catch’s picture

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

Let's backport, thanks for the reminder.

Spokje closed merge request !640

spokje’s picture

spokje
Primary language Dutch
commented
Issue tags: +Bug Smash Initiative

Status: Fixed » Closed (fixed)

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