Problem/Motivation

Fix 45 words in migrate files. Quite a few of these are used in less than 5 files.
1 actionid
2 blogapi
3 blorf
4 calendarsignup
5 ddblock
6 destid
7 entityreference
8 errored
9 filefield
10 flexslider
11 foobartik
12 foobaz
13 fubar
14 highwater
15 ichars
16 idmap
17 ieid
18 isid
19 janeway
20 johnsmith
21 larowlan
22 mapkey
23 mapwith
24 multigroup
25 multirow
26 nodelink
27 noslash
28 nzdt
29 openid
30 rdftype
31 reiss
32 slackjaw
33 somefieldname
34 sourceid
35 spamspan
36 terok
37 testformat
38 testproperty
39 throttleable
40 unmigrated
41 uploadsize
42 validatable
43 vancode
44 whois
45 wonkiness
46 wontmatter

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Comments

quietone created an issue. See original summary.

quietone’s picture

Title: Fix spelling for 46 migrate related words » Fix spelling for 45 migrate related words
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new93.28 KB
gauravvvv’s picture

@quietone, Please write down the steps to test this issue.

longwave’s picture

Status: Needs review » Needs work

This looks fairly uncontroversial but at least a few changes are needed:

  1. +++ b/core/misc/cspell/dictionary.txt
    @@ -577,8 +569,7 @@ fieldlinks
    +filefields ***
    

    This looks wrong.

  2. +++ b/core/modules/file/src/Plugin/migrate/field/d7/FileField.php
    @@ -5,6 +5,8 @@
    +// cspeLL:ignore filefield
    

    cspeLL -> cspell

  3. +++ b/core/modules/migrate/src/Plugin/MigrateValidatableEntityInterface.php
    @@ -4,6 +4,8 @@
    +// cspell:ignore validatable
    

    As "validatable" is used in the class name and could be used elsewhere in core, I wonder if it should just be in the dictionary.

  4. +++ b/core/modules/migrate_drupal/tests/src/Traits/NodeMigrateTypeTestTrait.php
    @@ -4,6 +4,8 @@
    +// cspell: ignore destid sourceid
    

    No space between cspell: and ignore. I am surprised this passes the spellcheck.

  5. +++ b/core/modules/migrate_drupal/tests/src/Unit/source/d6/Drupal6SqlBaseTest.php
    @@ -9,6 +9,8 @@
    +// cspell:ignore throttleable
    

    This word is in many English dictionaries, should we just include it in our dictionary?

  6. +++ b/core/modules/migrate_drupal_ui/tests/src/Functional/d7/files/sites/default/files/ds9.txt
    @@ -74,6 +74,7 @@
    +// cspell:ignore reiss
    

    The contents of this file don't seem to be used in the test, maybe we should just replace it with a few stars like the other test files here (even cube.jpeg isn't a JPEG).

  7. +++ b/core/modules/path/tests/src/Kernel/Migrate/d7/MigrateUrlAliasTestBase.php
    @@ -59,7 +59,7 @@ public function testUrlAlias() {
    +    $path_alias = $this->loadPathAliasByConditions(['alias' => '/source-noSlash']);
    

    There are other instances of "noslash" in the codebase, do they need replacing too?

quietone’s picture

Title: Fix spelling for 45 migrate related words » Fix spelling for 46 migrate related words
Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new6.69 KB
new98.04 KB

I really must remember to leave a not when I intend to do a self review.

#4
1,2,4 Fixed
3, 5. Yea, the -able words are challenging, it seems to depend on which dictionary you check.
validatable is identified as a spelling error in two files, core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php, core/modules/migrate/src/Plugin/MigrateValidatableEntityInterface.php. And it does only appear twice,

$grep -ri validatable core | awk -F: '{print $1}' | sort -u
core/misc/cspell/dictionary.txt
core/modules/migrate/src/Plugin/migrate/destination/EntityContentBase.php
core/modules/migrate/src/Plugin/MigrateValidatableEntityInterface.php

I decided to change it since the word is limited to the migration system.

Similarly, throttable, is also limited to the migrate system and specifically a Drupal 6 test. I see no reason not to change it.

grep -ri throttleable core | awk -F: '{print $1}' | sort -u
core/misc/cspell/dictionary.txt
core/modules/migrate_drupal/tests/src/Unit/source/d6/Drupal6SqlBaseTest.php

6. Changed the contents of ds9.txt to '***'. ds9.txt is in the Drupal7 test fixture as well so it could be tested in any Migration Kernel test. I'll let the testbot find any failures. This change also means 'terok' can be removed from the list as well.
I do think phenaproxima will be disappointed 2673960-#50.
7. Interesting. The other occurrences are in Migrate test files and this one is a Migrate test base class. We could add an exclusion for those but I'd rather not since some are in migrate module, list below. I think we need to keep this change.

$ find . -name \*Migrate\*TestBase.php
./core/modules/path/tests/src/Kernel/Migrate/d7/MigrateUrlAliasTestBase.php
./core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeExecuteTestBase.php
./core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
./core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6TestBase.php
./core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7TestBase.php
./core/modules/migrate_drupal/tests/src/Kernel/MigrateDrupalTestBase.php
./core/modules/migrate/tests/src/Kernel/MigrateSourceTestBase.php
./core/modules/migrate/tests/src/Kernel/MigrateSqlSourceTestBase.php
./core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
./core/modules/node/tests/src/Kernel/Migrate/d6/MigrateNodeTestBase.php

I've been interrupted a few time while working on this and it feels like I have forgotten something.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

vikashsoni’s picture

StatusFileSize
new350.72 KB
new340.14 KB
new203.8 KB
new56.23 KB

I am getting error when applying the patch for ref sharing screenshot

longwave’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
quietone’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll

@vikashsoni, When a patch no longer apply you can just leave a comment that it no longer applies and tag as 'needs reroll'. Sceenshots of applying a patch are not useful. Removing credit as per How is credit granted for Drupal core issues.

Rerolled the patch and fixed new occurrences of 'foobaz' in MigrationLookupTest. There are two more fixes to make, but it is too late to start that.

/var/www/html/core/modules/migrate_drupal_ui/tests/src/Functional/d7/FilePathTest.php:279:24 - Unknown word (Terok)
/var/www/html/core/modules/migrate_drupal_ui/tests/src/Functional/d7/FilePathTest.php:280:31 - Unknown word (Terok)
longwave’s picture

Status: Needs review » Needs work

@quietone no patch attached :)

quietone’s picture

Status: Needs work » Needs review
StatusFileSize
new7.96 KB
new98.75 KB

Looks like I really needed to get to bed.

quietone’s picture

StatusFileSize
new408 bytes
new99.32 KB

Added a cspell:ignore line for 'terok'.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

This is a nice little cleanup, I scanned through the whole patch and nothing jumps out as being a problem, all previous nits were fixed.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 12: 3209934-12.patch, failed testing. View results

quietone’s picture

quietone’s picture

Status: Needs work » Reviewed & tested by the community

Passing tests again, setting back to RTBC.

alexpott’s picture

StatusFileSize
new2.33 KB
new101 KB

I re-rolled this the was a conflict in a test. I ran the test and the spellcheck. It's all good. The dictionary should always be generated with yarn run spellcheck:make-drupal-dict

Here's the resulting patch.

The interdiff is the changes to the dictionary. The patch in #12 was adding back some words to the dictionary that it shouldn't have. Plus there have been other core changes that have resulted in some words no longer being used.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 57d91db and pushed to 9.3.x. Thanks!

  • alexpott committed 57d91db on 9.3.x
    Issue #3209934 by quietone, alexpott, longwave: Fix spelling for 46...
huzooka’s picture

Status: Fixed » Needs work

Could you please restore ds9.txt? Or at least restore its original size? I am using it in a test (which tracks what happens with core migrations), and its history is very annoying:

  1. It was added by #2673960: Unable to migrate D7 User cck fields (commit https://git.drupalcode.org/project/drupal/-/commit/b844712), the real file size was 4878, but the record in file_managed (which was manually added) defines file size 4720(bytes)
  2. Now you changed its content to 3 stats (and a line break), so now its size is 4 bytes. But noone has updated the record in the file_managed table.

The real solution is entirely ignore the whole core/modules/migrate_drupal_ui/tests/src/Functional/d7/files directory with cspell check. This is exactly the same situation what we have with the database fixture files: they're ignored entirely on phpcs checks.

If we keep not following the online documentation for updating the database fixtures for migration tests, then this will keep happening. Maybe we need a test that does basic integrity checks for the database migration fixtures? 😅 A … "test test"?

huzooka’s picture

quietone’s picture

Yes, I did forget to update the file_managed table. If you open up a new issue for that I will make a patch later this week, setting the size to 4.

Although they are test files, I am not convinced that excluding that directory from cspell is the solution.

alexpott’s picture

Status: Needs work » Fixed

Status: Fixed » Closed (fixed)

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