
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
Comment | File | Size | Author |
---|---|---|---|
#17 | 3209934-17.patch | 101 KB | alexpott |
#17 | 12-17-interdiff.txt | 2.33 KB | alexpott |
#12 | 3209934-12.patch | 99.32 KB | quietone |
#12 | interdiff-9-12.txt | 408 bytes | quietone |
#11 | 3209934-9.patch | 98.75 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedComment #3
gauravvvv CreditAttribution: gauravvvv at OpenSense Labs commented@quietone, Please write down the steps to test this issue.
Comment #4
longwaveThis looks fairly uncontroversial but at least a few changes are needed:
This looks wrong.
cspeLL -> cspell
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.
No space between cspell: and ignore. I am surprised this passes the spellcheck.
This word is in many English dictionaries, should we just include it in our dictionary?
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).
There are other instances of "noslash" in the codebase, do they need replacing too?
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedI 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,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.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.
I've been interrupted a few time while working on this and it feels like I have forgotten something.
Comment #7
vikashsoni CreditAttribution: vikashsoni as a volunteer and at Zyxware Technologies commentedI am getting error when applying the patch for ref sharing screenshot
Comment #8
longwaveComment #9
quietone CreditAttribution: quietone as a volunteer commented@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.
Comment #10
longwave@quietone no patch attached :)
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedLooks like I really needed to get to bed.
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedAdded a cspell:ignore line for 'terok'.
Comment #13
longwaveThis is a nice little cleanup, I scanned through the whole patch and nothing jumps out as being a problem, all previous nits were fixed.
Comment #15
quietone CreditAttribution: quietone as a volunteer commentedLooks like random failing, #3056848: Investigate LayoutBuilderDisableInteractionsTest random fails further. Retesting
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedPassing tests again, setting back to RTBC.
Comment #17
alexpottI 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.
Comment #18
alexpottCommitted 57d91db and pushed to 9.3.x. Thanks!
Comment #20
huzookaCould 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:4878
, but the record infile_managed
(which was manually added) defines file size4720
(bytes)4
bytes. But noone has updated the record in thefile_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"?
Comment #21
huzookaComment #22
quietone CreditAttribution: quietone as a volunteer commentedYes, 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.
Comment #23
alexpottI opened #3246053: Fix file_managed table for ds9.txt to address #20