Drupal Core has some pretty atrocious spelling errors in it.
Now, on one hand, we're geeks. Why would spelling matter?
However, as part of reaching a global audience, misspelled English words are a real affront to folks whose first language isn't English.
Finding text through search/replace is also more of a challenge when not all of the words are consistently used.
It also just looks pretty amateur. If this is an enterprise system, we can't be filled with lots of stupid typos in the code.
Javascript - #2383865: Spellchecking Drupal - Javascript
Comments - #2383863: Spellchecking Drupal - Comments
PHP - #2383871: Spellchecking Drupal - PHP
------
Original:
The core is spellchecked. The typos which don't break the code were fixed.
Comments
Comment #1
maximpodorov CreditAttribution: maximpodorov commentedComment #4
andypostplease provide a way you does this patch to allow reviewers make sure that nothing is left ;)
Comment #5
maximpodorov CreditAttribution: maximpodorov commentedThat was just a magic. :)
I ran code inspection in my IDE (using spell checker only) and fixed typos. After many iterations with adding legitimate words in IDE's dictionary and fixing obvious typos in the core, there are about 1500 unknown words left which are mostly country and language names, Latin words from "Lorem ipsum", and strange php variable names. This amount is quite observable, so I think most problems are fixed by this patch. There are test function names with typos and even Drupal variable name and JavaScript property name with typos, but I don't want to touch them.
Comment #6
jhodgdonThis is a nice patch probably... but very large and thus hard to review... Also, we have a policy of not fixing bugs in Drupal 7 until the corresponding bug in Drupal 8 has been fixed first. So, this one is going to be difficult to get committed.
Comment #7
maximpodorov CreditAttribution: maximpodorov commentedCan typos in D8 be the corresponding bugs to the typos in D7?
Comment #8
jhodgdonBasically, this issue needs to be Drupal 8, and then backported to Drupal 7. But you will have better luck getting it reviewed if you break it up into smaller patches.
Comment #9
maximpodorov CreditAttribution: maximpodorov commentedHere are patches for Drupal 8 split by the core/* directory.
Comment #10
maximpodorov CreditAttribution: maximpodorov commentedComment #11
maximpodorov CreditAttribution: maximpodorov commentedComment #16
jhodgdonThanks! Those four patches in #9 and #10 have been committed to Drupal 8.0.x. I suspect you have more patches coming...
By the way... one of the patches had several variable names whose spelling was fixed... I had to check those more carefully, but they were fine. I'll wait to commit the next batch of patches until bot reviews are done because when you change variable names, we need to make sure the tests still pass. That next patch is pretty big!
Comment #17
jhodgdonHiding the 4 patches I committed and the huge one for 7. Something happened to the patch in #11, can you reupload maybe?
Comment #18
maximpodorov CreditAttribution: maximpodorov commentedThe huge patch is huge because of diff context. The changes are small.
Comment #19
jhodgdonCommitted the patch in #18 to 8.0.x. Back to "Active" status in case there are more patches coming. Thanks again!
Comment #21
maximpodorov CreditAttribution: maximpodorov commentedOne more patch for 'modules' directory. And that's all for Drupal 8. Only 'assets' and 'vendor' directory are left, but they contain external files not owned by Drupal.
Comment #22
maximpodorov CreditAttribution: maximpodorov commentedComment #24
maximpodorov CreditAttribution: maximpodorov commentedThis patch is the final one.
Comment #25
jhodgdonIf you are going to fix the spelling of "htmls", please change it to "HTML". It's an acronym.
Also, I think in BlockContentSaveTest, determing should be determining, not determine.
And I'm a bit concerned about changes to .js files. They are not tested by the bot. So can you put them in a separate patch so we can review/test them manually? Actually... there are quite a lot of code changes in this patch (as opposed to comment changes). Can you please put them in a separate patch? I'm comfortable doing a review-and-commit on the comment spelling changes, but if you are changing PHP method names, or strings in PHP code that are part of tests, they need to be reviewed more carefully. This patch is pretty big...
Comment #26
maximpodorov CreditAttribution: maximpodorov commentedThe patch for modules JS code.
Comment #27
maximpodorov CreditAttribution: maximpodorov commentedThis patch is for modules code changes.
Comment #28
maximpodorov CreditAttribution: maximpodorov commentedAnd the last patch for comments.
Comment #29
jhodgdonOh, sorry, I think I didn't make myself clear.
Any change that is in a code comment, you can put into one patch. That patch I can easily review and commit quickly.
Changes that are to actual JS code (changing function or variable names or literal strings in code) should go in a second patch, so it can be reviewed more carefully. Your patch in #26 has a bunch of comment changes in it -- please add those to the comments-only patch and they'll get taken care of sooner.
Changes that are to actual PHP code (changing function or variable names or literal strings in code) should go in a third patch, so it can be reviewed more carefully. Your patch in #27 has a bunch of comment changes in it -- please add those to the comments-only patch and they'll get taken care of sooner.
The patches in #26 and #27 will take a while to review and since they just fix typos, they are not going to be anyone's highest priority right now... so the more you can get out of those patches and into a "this is just typos in comments" patch, the better.
Comment #31
maximpodorov CreditAttribution: maximpodorov commentedJavaScript code change patch.
Comment #32
maximpodorov CreditAttribution: maximpodorov commentedComments patch.
Comment #33
maximpodorov CreditAttribution: maximpodorov commentedPHP code patch.
Comment #35
maximpodorov CreditAttribution: maximpodorov commentedPHP code patch without incorrect code left from the outdated commits.
Comment #36
maximpodorov CreditAttribution: maximpodorov commentedComment #37
mbrett5062 CreditAttribution: mbrett5062 commentedSome small nitpicks/corrections I found in the "Comments" patch. #32
I think this would read better as so:
Tests if a block can be configured as only visible on a particular language.
*NOTE language can now move up, so making this a single line, now less then 80 characters.
Should this be:
// Check the tar contains the expected content for the collections.
Extra 'H' left in here I think, should be:
* Contains \Drupal\system\Tests\Entity\EntityAccessControlHandlerTest.
Should this be "internally" not "internal"
I think this reads better as follows:
* Field handler which shows machine name content as a human readable name.
Should be "distinct" not "distincted"
"linkclass" should be "link_class"
"Applies an" not "Applies a" I think reads better.
Comment #38
mgiffordI looked through drupal8-spellchecked-modules_code-2329703-35.patch
It's in function & variable names, tests & the documentation. Quite hilarious actually. All those folks whose first language isn't English who who may be stumped by all these errors. All the propagation of these errors (because folks copy/paste prior examples). I'm including a list of errors discovered that were uncovered in the patch:
Not that I'm much of a speller or noticed any of these on my own. Even when comparing lines it was sometimes hard to see where the problem was.
This isn't a problem with the patch, but noticed that the whole $optipns key isn't being used in this foreach:
Is it worth removing in another issue?
Comment #39
mgiffordJust saw @mbrett5062's review.
Comment #40
jhodgdonRegarding mbrett5062's review, this patch is *just* fixing spelling errors, as other patches did above. If it gets into fixing grammatical errors (which existed before this patch), it gets more difficult to review. So let's not fix those here. Or any other issues. Just spelling of individual words.
So there are 3 active patches... which one(s) have you reviewed mgifford? And if it's the code patches, I'm very concerned about the code changes and they need to be reviewed more carefully than just verifying that the spellings are now correct...
Comment #41
mbrett5062 CreditAttribution: mbrett5062 commentedI think 1,3,6, & 7 from my review should be considered spelling corrections, not grammatical changes.
#NOTE 1 points out that "configure" should be "configured", as well as the grammar change.
I also think that 2 can be considered a spelling correction.
Comment #42
mgifford@jhodgdon I am marking the code #35 & #31 as RTBC. The corrections to the JS code brings us a few additional errors like:
The bot would have most likely discovered any changes in variable names that would cause problems. For the next 6hrs there will be examples running up here, here & here for folks to investigate. They all look good to me.
I agree that the comment code in #32 still needs a bit more work. Some of @mbrett5062's suggestions go beyond spell checking the comments. I think they should be made though if @jhodgdon thinks they are an improvement. I don't know that they need to be brought into a new issue but maybe.
Maybe the JS, Code & Comments should be 3 separate issues so we can track them separately and not get confused. That being said, these seem pretty simple. It is important though that they are fixed before they spread.
These are pretty straight forward changes though...
Comment #43
star-szrWith any patch like this, it's worth bringing up the awesomeness that is
git diff --color-words
. Hat tip to @xjm for that one.Comment #44
mgiffordNice. That's way easier. Thanks @Cottser & @xjm. Produces a display like:
$this->assertFalse($javascript['core/misc/collapse.js']['preprocess'], 'Setting cache to FALSE sets proprocesspreprocess to FALSE when adding JavaScript.');
So you don't have to dig though and try to find the differences in the diff (like I did).
Comment #45
maximpodorov CreditAttribution: maximpodorov commentedMaybe it's better to wait for #31 and #35 committing, and then continue to fix #32 in this issue.
Comment #46
mgiffordPart of my urgency on this is that in my experience with other low hanging issues like:
https://www.drupal.org/project/issues/search?issue_tags=typography
They are big patches that need to be rerolled & rerolled & rerolled... Never really getting into Core.
Perfect is the enemy of good. This is an issue that really doesn't need to be perfect. It's clearly an improvement that will address some embarrassments.
Comment #47
jhodgdonThere is another issue marked "avoid commit conflicts" that also touches a bunch of files in core/modules. I am not going to take action on any of these patches until #2331919: Implement StackNegotiation is finished (that is higher priority). Sorry.
Also I'm not going to commit the JS and PHP code changes patches. Too risky for me. So... I plan to review/commit the "comments" patch when there aren't any possibly-conflicting "avoid commit conflicts" issues. Then we'll need to change the component to something other than "documentation" and get another committer involved.
Comment #49
mgiffordLooks like testTagDeletetions was corrected. Not before Google caught it in the API mind you https://www.google.ca/search?q=testTagDeletetions
Anyways, there are over 40 spelling mistakes which are caught in the modules patch which I just re-rolled.
Comment #50
jhodgdonSince this issue has multiple patches, please hide old files when you upload replacement patches. Thanks!
Comment #51
mgiffordThat's a good pattern. I haven't applied that in the past... Good suggestion though. Thanks for implementing it.
Comment #52
tim.plunkettSorry @mgifford, this is not a major bug by any definition.
Comment #53
mgiffordFine... But then we're bound to fix this problem in 20 separate issues that get opened over the next year rather than this one were it's been consolidated.
Comment #54
jhodgdon@mgifford - tim.plunkett is right. Fixing typos like this -- especially ones that end users do not see and that do not affect functionality -- does not take precedence over substantive issues. We will get this done; if you look at the history above you'll see that I've already committed quite a number of patches on this issue.
The last 3 patches that have been submitted here are just especially problematic to commit, as they are large (therefore can lead to rerolls for many many other more substantive issues) and the two that affect code need to be examined very carefully before they can be committed. Patience, please -- these last few patches have only been here for a few days. There are plenty of more important issues in the RTBC list that have been waiting at least as long, most of the time, before they're finally committed.
Comment #55
maximpodorov CreditAttribution: maximpodorov commentedMaybe Drupal 7 can be cleaned from the typos meanwhile?
Comment #56
jhodgdonOur policy is always to fix Drupal 8, then port to Drupal 7.
Comment #57
mgiffordComment #58
rpayanmrerolling...
Comment #59
mgiffordComment #60
mgifforderror: patch failed: core/modules/language/src/Tests/LanguageUILanguageNegotiationTest.php:314
error: core/modules/language/src/Tests/LanguageUILanguageNegotiationTest.php: patch does not apply
Comment #63
jhodgdonThere are several patches now on this issue.
Can you please:
a) Name files consistently so we know which reroll applies to which patch without having to check them all.
b) Hide obsolete patch files so only patches currently under consideration are still visible at the top of this issue.
Comment #64
rpayanmohh sorry I never worked with a one issue with multiples patches :)
reroll again...
Comment #65
rpayanmjs
Comment #66
rpayanmComment #67
rpayanmComment #68
jhodgdonThanks, that's much less confusing!
So, all of these patches still need a review... The PHP and JS ones should probably be spun off into their own issues, because they will each require some careful review and/or testing, and I'm not sure whether they meet the "beta patch" criteria or not.
Comment #69
Valentine94Some back-port for D7.
Comment #71
Valentine94Try to resolve failing tests.
Comment #72
Valentine94Should I divide this patch by some conditions like in #64 #65 #66?
Thanks.
Comment #73
jhodgdonPlease read https://www.drupal.org/node/1319154#multiple-versions
We need to resolve Drupal 8 before going to Drupal 7.
Comment #74
mgiffordYup! And with the D7 stuff it's going to be a bit of a challenge as we wouldn't want to break any existing sites. Would really limit what we can roll out.
Still, what are the next stages to get this into Core?
Do we need different issues for #64, #65 & #66?
Comment #77
mgifford#66 needs a reroll. The other two seem to work just fine still.
Comment #78
rpayanmRerolled...
Comment #79
jhodgdonAs for how to proceed... If someone wants to give the "comments" patch a careful review, that would probably be good, and then we could get that one in if it truly is only affecting comments.
The other two are more problematic. The PHP code one needs to be checked over carefully... but since we at least have a lot of tests for the PHP code, as long as the changes don't change how many test assertions are run we can be fairly certain their probably OK. The JS one is the really dangerous one IMO, because we do not have tests for JS. So someone needs to identify what could be affected and then I guess manually test it.
So yes: let's move at least the JS patch to its own issue, and put it in the JavaScript component, so that the JS maintainers can test/review. I think we can continue to deal with the PHP ones here.
I've also unhidden the "code" patch.
Comment #81
mgiffordThis is so annoying. I almost RTBC'd the comment patch. I'd so like this to just get in quickly..
Sadly, in cases where the php variable is incorrectly spelled, we should probably change the comments and code in the same patch. I looked at every line and grepped for instances where the variable name was spelled wrong (in this patch) and was also wrong in the code. I could only find:
I really think that those should go in the same patch since it makes no sense to change the comments to incorrectly describe the variable.
If that's already been cleared elsewhere, I'm fine with it, but....
Comment #82
rpayanm@mgifford wow... nice catch.
Here the patch again...
Comment #83
mgiffordThat works for me. #82 is just one part of this issue, but let's see if we can't get it into core.
Comment #84
alexpottThese are bugs and should be separated out into a patch that can go in now. The rest of this patch is minor and is subject to https://www.drupal.org/core/beta-changes. The change benefit vs the disruption of this patch just does not play out. Can this be separated out into two issues one to address the PHPDoc bugs and one for the spelling mistakes.
Implements
Comment #86
jhodgdonPlease do not requeue these patches. @alexpott asked that this issue be split out into what is actual bugs vs. minor cleanup. We are not going to do the minor cleanup at this time on this scale. It is too disruptive.
So the parts that would actual bugs would be things like:
- @param vs. @parm or other misspellings
- When the documentation does not match the function signature, like @param $fooxyz vs. function($foo). Like here:
- When a "Contains" in the @file does not match the class name
- Text that would appear in the user interface, with misspelled words in it.
Minor fixes that we need to postpone would include:
- Test method names that have misspelled words in them
- Test assertion message text with misspelled words
Comment #87
jhodgdonActually, documentation fixes are supposed to be prioritized and see #2343099-8: Add @param and @return or fix types in @param and @return in core/lib/Drupal/Core/Database/... apparently "it will conflict with other patches" is not supposed to qualify a patch as 'disruptive'.
So. I think most of the documentation fixes are actually bugs and we should fix them.
Comment #88
mgiffordI'm for fixing them. They are embarrassing. I can help reroll some of the patches that are affected.
Comment #89
jhodgdonBefore we reroll any new patches, let's please split this off into one issue per remaining patch.
Comment #90
mgiffordComment #91
mgiffordI think this is what is needed. Let me know if there is anything missing.
Comment #92
alexpottJust filed #2374067: System Module - Corrections as a duplicate.
I take issue with this statement - having seen my fair share of enterprise code I don't think that "stupid typos" in code comments are a high priority for enterprise adoption. User facing text is different and definitely should be a priority.
I can see how we might want to classify a spelling mistake as a bug, fair enough. If it is in a code comment it probably should be a minor bug. Incorrect PHPDoc is more important and should be filed as a normal bug. User facing text is probably a major or critical depending on where the text appears.
I do think that having all the code comments spelt correctly is nice; it shows we care, have a good attention to detail and is way less confusing for people who have a limited proficiency in English.
So in order to make this easier to review and not 100k of change we need to find someway of breaking this up.
Comment #93
mgiffordA) Short & Sweet Already - #2383865: Spellchecking Drupal - Javascript
B) Minor in consequence but difficult to translate with over 2400 lines in the diff #2383863: Spellchecking Drupal - Comments
C) Not too terribly long, has lots of user facing code, but 45 files & 740 line diff - #2383871: Spellchecking Drupal - PHP
Main concern I think is B. We could separate Views patches. Tests. Would it help if it were just broken down into 5 patches each under 500 lines of code? Just something to make it somewhat less disruptive.
Sorry you didn't like my statement about "enterprise systems". Seems if we are trying to reach out of an English audience, having the comments be at least written in English should help adoption as it's way easier than dealing with the spelling that right now in Core. I can't spell, but think this is something that has largely already been fixed (and folks are asking about back-porting to D7).
Comment #96
mgiffordHere's a re-roll. Nice to see that some spelling errors have been fixed by other patches.
Comment #97
jhodgdonI'm confused. We have 3 child issues. What's supposed to be in this patch that's not in the children?
Comment #98
mgifford@jhodgdon
This has the drupal modules in it. There are only 2 children really. And one duplicate.I have to look a bit closer at #2383871: Spellchecking Drupal - PHP.
Comment #99
Pere OrgaClosing #2488476: Fix 14 typos in code as a duplicate of this one, and adding patch that should be merged with current/new patches.
Comment #100
maximpodorov CreditAttribution: maximpodorov commentedSo, Drupal 8 is perfectly spelled now. Let's go back to Drupal 7.
Comment #101
maximpodorov CreditAttribution: maximpodorov commentedThe very first patch is re-rolled, and two typos was already fixed in 7.x.
Comment #102
Pere OrgaThis should be done in multiple patches too?
Comment #103
Pere OrgaLet's postpone this until #2494313: Follow up to Spellchecking Drupal - PHP (affects D7)
Comment #104
Pere OrgaAnd #2494319: Follow up to Spellchecking Drupal - Comments
Comment #105
maximpodorov CreditAttribution: maximpodorov commentedI don't think this should be postponed now, when the majority of typos in D8 is fixed. New typos can appear in every new commit, and they can't stop fixing D7 which is quite stable (in terms of new large changes).
Comment #107
Pere OrgaPlease split the file and include the changes in #2494313: Follow up to Spellchecking Drupal - PHP and #2494319: Follow up to Spellchecking Drupal - Comments
Comment #109
mgiffordAll the child issues are fixed, so this should be too... Although I don't know that it has been backported in all the child issues.