Drupal Core has some pretty atrocious spelling errors in it.
Now, on one hand, we're geeks. Why would spelling matter?
However, as part o. 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.
This is an issue to look specifically at the Javascript errors.
Patch from #2329703-78: [meta] Spellchecking Drupal
Comment | File | Size | Author |
---|---|---|---|
#18 | interdiff.txt | 792 bytes | pguillard |
#17 | spellchecked-2383865-17.patch | 9.94 KB | pguillard |
#6 | 2383865-6.patch | 9.94 KB | rpayanm |
#6 | 2383865-interdiff.txt | 7.32 KB | rpayanm |
Comments
Comment #1
mgiffordComment #3
mgiffordcore/modules/ckeditor/js/ckeditor.js was fixed "saveCallack: null" was corrected by an earlier patch!
Comment #4
nod_That works, thanks.
Comment #5
alexpottin core/modules/quickedit/js/models/EntityModel.js
in core/modules/menu_ui/menu_ui.admin.js
in core/modules/filter/filter.filter_html.admin.js
in core/modules/menu_ui/menu_ui.js - this patch converted all the other instances here - so this is likely to be introducing a bug.
in core/modules/ckeditor/js/ckeditor.stylescombo.admin.js
I think this should be
jQuery.Event
in core/modules/quickedit/js/views/EntityToolbarView.jsin core/modules/file/file.js
in core/modules/contextual/js/contextual.js
in core/modules/quickedit/js/views/AppView.js
in core/modules/views_ui/js/views-admin.js
we don't hyphenate - in core/modules/editor/js/editor.admin.js
Comment #6
rpayanmComment #7
jhodgdonOK, this all looks good to me.
But because it is JavaScript, we have no automated tests. Some of the patch is just changing comments, but some of it is changing JavaScript function names or variable names, which could potentially cause problems or break functionality.
The changes to actual JS code are in:
core/modules/contextual/js/toolbar/models/StateModel.js
core/modules/menu_ui/menu_ui.js
core/modules/quickedit/js/models/EntityModel.js [this is changing the string returned by the function; no idea what that is used for, it doesn't look user-facing?]
I personally have no idea what any of this JS code does, but I guess it should be tested manually, and we should also make sure that all uses of it have corresponding changes?
Comment #8
mgiffordmenu_ui.js is missing the the @file header for some reason. That should be a new bug I think core/modules/menu_ui/menu_ui.js
However, they are described for both the StateModel.js & EntityModel.js
core/modules/contextual/js/toolbar/models/StateModel.js
/**
* @file
* A Backbone Model for the state of Contextual module's edit toolbar tab.
*/
core/modules/quickedit/js/models/EntityModel.js
/**
* @file
* A Backbone Model for the state of an in-place editable entity in the DOM.
*/
Could be clearer, I think, but that's a new issue.
I applied this patch and tested it in Bartik & Seven. Seems fine. The menu seems to work fine, so doe the in-place editor.
I reviewed the code that was patched and the changes are consistently applied. If they appeared in other files, then this would be a potential problem, but the corrections are pretty specific within the file.
@alexpott was concerned that changing menuLinkAutomaticTitleOveridden in menu_ui.js would be "likely to be introducing a bug" but it didn't seem to and seemes to be exclusively contained in menu_ui.js
I reviewed #5's changes to see that they were in the diff. I'm pretty sure this is good to go.
Comment #11
mgiffordI think this is all still fine. Just reapplied the patch with no problem.
Comment #12
alexpott@mgifford the bug that #3 would have introduced was fixed in #6
Given that I'm not 100% confident of the rtbc. I'm putting back to needs review and leaving a tell for _nod in IRC.
Comment #13
alexpottAlso @mgifford rtbc'ing your own patches is against policy -see https://www.drupal.org/node/156119#rtbc
Comment #14
mgiffordSorry @alexpott - I just went up to the latest patch which was from @rpayanm and didn't look any higher. Nor did I compare it with the previous patch (which I rolled). Hopefully @_nod can get to it.
It is very common in issues for people who have written a patch historically to mark the latest patch RTBC, which is what I did. I don't think #13 is appropriate in this case (unless #6 & #3 are identical).
This is one of several issues addressing stupid spelling mistakes. It really shouldn't be that hard to get it marked RTBC. Surely if we were confident about our use of the english language it would produce less errors going ahead.
Comment #15
mgiffordComment #17
pguillard CreditAttribution: pguillard commentedReview :
Most of the spelling errors are in comment line. Concerning those in the code, I verified that no one remains in code with wrong typo.
During this time views-admin.js has changed. I slightly rerolled patch #6
Comment #18
pguillard CreditAttribution: pguillard commentedForgot the interdiff..
Comment #19
Jody LynnI reviewed the latest patch and latest interdiff.
Comment #20
xjmNote that this patch changes more than just documentation so it's no unfrozen during the beta. The changes I've highlighted below are the non-docs ones. However, misspelled variable names introduce fragility (which is a specific example we included in https://www.drupal.org/core/beta-changes#prioritized), and there isn't really any disruption from fixing the variable names in this patch, so this is a good change to make during the beta.
Per @alexpott's comment in #11, I also grepped to ensure there were no more instances of these misspellings after applying the patch:
I also reviewed locally with
git diff --color-words
to ensure all the changes looked correct.