Spellchecking Drupal - PHP

Following on comments by @alexpott in the parent issue...

It seems to me that spelling errors in test modules and test classes are very minor problems.

On the other hand, spelling errors that are displayed in Drupal sites are real problems.

So can we perhaps separate this out into two issues/patches: spelling errors in test code and spelling errors that are outside of tests? We can prioritize the "outside of tests" part while leaving the "in tests" part until a less disruptive time in the beta cycle.

again.

and again.

I read through this whole patch again. I'm a bit concerned by:

--- a/core/modules/toolbar/css/toolbar.module.css
+++ b/core/modules/toolbar/css/toolbar.module.css
@@ -134,7 +134,7 @@ body.toolbar-tray-open.toolbar-fixed.toolbar-vertical .toolbar-oriented {
   display: none;
   z-index: 501;
 }
-.toolar .toolbar-tray > .toolbar-lining {
+.toolbar .toolbar-tray > .toolbar-lining {
   position: relative;
 }

Why is this in the PHP patch and has someone tested this CSS change? Presumably this CSS has never worked, and if it wasn't needed it should probably just be removed, not spellchecked into now suddenly doing something.

This part is also a bit scary:

+++ b/core/modules/update/tests/modules/update_test/src/MockFileTransfer.php
@@ -27,7 +27,7 @@ public static function factory() {
    */
   public function getSettingsForm() {
     $form = array();
-    $form['udpate_test_username'] = array(
+    $form['update_test_username'] = array(
       '#type' => 'textfield',
       '#title' => t('Update Test Username'),
     );

This is changing a form element ID and I cannot believe it was working correctly both without and with this patch. Either this piece of settings form is never accessed in a test or ... something else is wrong.

Looks correct. Most changes are test messages (and test method names) now, and the rest are trivial changes. Anyway, after applying the last patch, new iteration of spell checking can be performed to find the new and missing typos.

The patch is re-rolled.

overridden was misspelled in core/modules/simpletest/src/TestBase.php, fixed it in the new patch.

interdiff attached as well.

Initially I don't see why the patch would cause that fail.

Are grammatical errors within the scope of this patch? If so, in the patched code: "invalidate" -> "invalid"

OK to set this to RTBC?

Thanks all for your work on this!

Just a not that I disagree with "atrocious" spelling errors -- none of these are user-facing, and most of them are only in test code. So for the most part this is a minor issue. It also requires very careful review to ensure that none of the changes are introducing regressions, and I'd prefer not to be doing this during this phase of the release cycle, even though it's technically mostly unfrozen. However, the patch is ready, and I've reviewed it carefully, so I decided to go ahead with it since this has a higher chance of disruption during RC or a patch release than other cleanups of this nature. And, after all, the spelling errors are irritating and a bit embarrassing. :)

Going forward, though, let's always do a beta evaluation and consider what the impact vs. the disruption of a minor change is, and whether it's worth the resources during the beta. (For reference, it took me 20 minutes to review this and make sure it wasn't breaking anything.)

A few notes from my review follow:

1. +++ b/core/modules/ckeditor/tests/modules/src/Plugin/CKEditorPlugin/LlamaButton.php
   @@ -26,7 +26,7 @@ class LlamaButton extends Llama implements CKEditorPluginButtonsInterface {
   -        'label' => t('Insert Lllama'),
   +        'label' => t('Insert Llama'),
   

   But if "llama" with two Ls is cool, isn't three Ls even cooler? ;)

2. +++ b/core/modules/node/src/Entity/NodeType.php
   @@ -143,8 +143,8 @@ public function displaySubmitted() {
   -  public function setDisplaySubmitted($display_submtited) {
   -    $this->display_submitted = $display_submtited;
   +  public function setDisplaySubmitted($display_submitted) {
   +    $this->display_submitted = $display_submitted;
   
   +++ b/core/modules/node/src/NodeTypeInterface.php
   @@ -52,7 +52,7 @@ public function displaySubmitted();
   -  public function setDisplaySubmitted($display_submtited);
   +  public function setDisplaySubmitted($display_submitted);
   

   Fixing spelling errors in variable names is a prioritized change (though small impact here). I grepped to confirm there were no other instances of display_submtited. This is the one change in this patch that is indeed prioritized for the beta, I think.

3. +++ b/core/modules/quickedit/src/Form/QuickEditFieldForm.php
   @@ -143,7 +143,7 @@ protected function init(FormStateInterface $form_state, EntityInterface $entity,
   -    foreach ($display->getComponents() as $name => $optipns) {
   +    foreach ($display->getComponents() as $name => $options) {
   

   It raised a red flag for me that I only see this variable changed where it's defined, but $options isn't actually used. Only $name is. It might be better as array_keys($display->getComponents() -- but that would be out of scope. So this change is okay.

4. +++ b/core/modules/simpletest/src/AssertContentTrait.php
   @@ -645,13 +645,13 @@ protected function assertUniqueTextHelper($text, $message = '', $group = 'Other'
   -    $first_occurance = strpos($this->getTextContent(), $text);
   -    if ($first_occurance === FALSE) {
   +    $first_occurrence = strpos($this->getTextContent(), $text);
   +    if ($first_occurrence === FALSE) {
          return $this->assert(FALSE, $message, $group);
        }
   -    $offset = $first_occurance + strlen($text);
   -    $second_occurance = strpos($this->getTextContent(), $text, $offset);
   -    return $this->assert($be_unique == ($second_occurance === FALSE), $message, $group);
   +    $offset = $first_occurrence + strlen($text);
   +    $second_occurrence = strpos($this->getTextContent(), $text, $offset);
   +    return $this->assert($be_unique == ($second_occurrence === FALSE), $message, $group);
   

   Another misspelled variable name. I confirmed there are no other instances of first_ nor second_occurance.

5. +++ b/core/modules/system/src/Form/PerformanceForm.php
   @@ -61,11 +61,11 @@ class PerformanceForm extends ConfigFormBase {
   -  public function __construct(ConfigFactoryInterface $config_factory, CacheBackendInterface $render_cache, DateFormatter $date_formater, AssetCollectionOptimizerInterface $css_collection_optimizer, AssetCollectionOptimizerInterface $js_collection_optimizer) {
   +  public function __construct(ConfigFactoryInterface $config_factory, CacheBackendInterface $render_cache, DateFormatter $date_formatter, AssetCollectionOptimizerInterface $css_collection_optimizer, AssetCollectionOptimizerInterface $js_collection_optimizer) {
        parent::__construct($config_factory);
    
        $this->renderCache = $render_cache;
   -    $this->dateFormatter = $date_formater;
   +    $this->dateFormatter = $date_formatter;
        $this->cssCollectionOptimizer = $css_collection_optimizer;
   

   Another misspelled variable. I grepped to confirm there are no other instances of date_formater.

6. +++ b/core/modules/system/src/Tests/Module/InstallUninstallTest.php
   @@ -101,7 +101,7 @@ public function testInstallUninstall() {
   -      $this->assertSuccessfullUninstall($name, $package);
   +      $this->assertSuccessfulUninstall($name, $package);
   
   @@ -116,7 +116,7 @@ public function testInstallUninstall() {
   -          $this->assertSuccessfullUninstall($name, $package);
   +          $this->assertSuccessfulUninstall($name, $package);
   
   @@ -151,7 +151,7 @@ public function testInstallUninstall() {
   -  protected function assertSuccessfullUninstall($module, $package = 'Core') {
   +  protected function assertSuccessfulUninstall($module, $package = 'Core') {
   

   Misspelled method name used only within this test. I confirmed there are no other assertSuccessfullUninstall.

7. Most of the changes in the patch are to test method names and assertion messages. The assertion messages have no impact, but the test method names could if the classes were overridden in extending tests (a pattern that we use in a few rare cases). I confirmed that none of these were that.

Committed and pushed to 8.0.x. Thanks everyone for your attention to detail with this issue. :)

Halleluiah! Let's go back to 7.x in #2329703: [meta] Spellchecking Drupal!

See #2494313: Follow up to Spellchecking Drupal - PHP