Problem/Motivation

Typos noticed 'libaries' and 'Dupalsettings', couldn't find issue.

Proposed resolution

Fix typo, clean up docblock coding standards:

  1. Extra line-breaks in function docblocks
  2. Missing semicolons at end or array lines
  3. Single quotes where not escaped and short array syntax

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#14 interdiff-2646962-9-14.txt2.65 KBwalangitan
#14 2646962-AjaxPageStateTest-cleanups-14.patch1.14 KBwalangitan
#9 2646962-AjaxPageStateTest-cleanups-9.patch3.37 KBwalangitan
#7 interdiff-2646962-2-7.txt1.66 KBklidifia
#7 2646962-AjaxPageStateTest-cleanups-7.patch3.37 KBklidifia
#2 2646962-AjaxPageStateTest-cleanups-2.patch3.19 KBklidifia
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joelpittet created an issue. See original summary.

klidifia’s picture

klidifia CreditAttribution: klidifia as a volunteer commented
Status: Active » Needs review
FileSize
2646962-AjaxPageStateTest-cleanups-2.patch3.19 KB

I didn't see any missing semicolons at end or array lines but some missing commas.
Full stop on end of docblock start comment.
Short array syntax.
Removed extra lines in docblocks.
Corrected typos.

Status: Needs review » Needs work

The last submitted patch, 2: 2646962-AjaxPageStateTest-cleanups-2.patch, failed testing.

The last submitted patch, 2: 2646962-AjaxPageStateTest-cleanups-2.patch, failed testing.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented

Thank you @klidifia, I think maybe this could be tweaked a bit further:

+++ b/core/modules/system/src/Tests/Render/AjaxPageStateTest.php
@@ -58,18 +58,17 @@ public function testLibrariesAvailable() {
     $this->drupalGet('node',
...
+      [
...
+          ]
+      ]
     );

The indenting should be one back and move the first [ the function call line and the last one with the last parenthesis, I think that would cover it.

There is examples this in core if it's not clear what I'm asking just search for ]);

Testbot looks to be acting weird.

The last submitted patch, 2: 2646962-AjaxPageStateTest-cleanups-2.patch, failed testing.

klidifia’s picture

klidifia CreditAttribution: klidifia as a volunteer commented
Assigned: Unassigned » klidifia
Status: Needs work » Needs review
FileSize
2646962-AjaxPageStateTest-cleanups-7.patch3.37 KB
interdiff-2646962-2-7.txt1.66 KB

Gotcha thanks. I've been noticing issues with testbot over last couple of days.
New patch and interdiff.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
CreditAttribution: jhodgdon commented
Component: documentation » system.module
Status: Needs review » Needs work

Thanks! As you are changing Code and not just docs, I am moving this to system.module.

I'm also not sure that we want to do patches like this that do a bunch of random fixes in one file but I will leave that to @xjm or @alexpott to decide.

Meanwhile, I don't think these changes are quite right:

  1. +++ b/core/modules/system/src/Tests/Render/AjaxPageStateTest.php
@@ -41,14 +41,14 @@ protected function setUp() {
+      'The Drupalsettings library from core should be loaded.'

    DrupalSettings I think? 

  2. +++ b/core/modules/system/src/Tests/Render/AjaxPageStateTest.php
@@ -78,28 +74,24 @@ public function testHtml5ShivIsNotLoaded() {
+      'The Drupalsettings library from core should be loaded.'

    DrupalSettings I think? 

  3. +++ b/core/modules/system/src/Tests/Render/AjaxPageStateTest.php
@@ -78,28 +74,24 @@ public function testHtml5ShivIsNotLoaded() {
+   * Test if multiple libraries can be excluded.

    If you're going to fix this line, also

    Test if -> Tests whether 

  4. +++ b/core/modules/system/src/Tests/Render/AjaxPageStateTest.php
@@ -107,7 +99,7 @@ public function testMultipleLibrariesAreNotLoaded() {
+      'The Drupalsettings library from core should be excluded from loading.'

    Should be DrupalSettings I think?

walangitan’s picture

walangitan CreditAttribution: walangitan at Chromatic commented
FileSize
2646962-AjaxPageStateTest-cleanups-9.patch3.37 KB

klidifia, jhodgdon - I looked at other examples of Drupalsettings and found that it's exclusively written as drupalSettings. Updated patch #7 with the change as well as changing `Test if` to `Tests whether` from the suggestions in #8.

joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Assigned: klidifia » Unassigned
Status: Needs work » Reviewed & tested by the community

Thanks for your help! I think @klidifia was intending to work on it but I think you got it. The changes look great to me.

@walangitan it's good to get in the habit of adding an interdiff so we can see what changes you made without re-reading the entire patch.

walangitan’s picture

walangitan CreditAttribution: walangitan at Chromatic commented

Excellent! @joelpittet, I'll start adding interdiffs for future issues.

klidifia’s picture

klidifia CreditAttribution: klidifia as a volunteer commented

Nice team effort :) looks good!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Needs work 
+++ b/core/modules/system/src/Tests/Render/AjaxPageStateTest.php
@@ -17,7 +17,7 @@
-   * User account with all available permissions
+   * User account with all available permissions.

@@ -41,14 +41,14 @@ protected function setUp() {
-    $this->drupalGet('node', array());
+    $this->drupalGet('node', []);
...
-      'The Dupalsettings library from core should be loaded.'
+      'The drupalSettings library from core should be loaded.'

These are three different types of changes. Please read https://www.drupal.org/core/scope for how to scope an issue. I think we should refocus this issue should just focus on fixing Dupalsettings to drupalSettings - if looked for other misspellings of drupalSettings in core and couldn't find any.

Also there is no preference between array() and [] so this is not an error.

walangitan’s picture

walangitan CreditAttribution: walangitan at Chromatic commented
FileSize
2646962-AjaxPageStateTest-cleanups-14.patch1.14 KB
interdiff-2646962-9-14.txt2.65 KB

Per @alexpott's suggestion, refocusing this to just fix the dupalsettings typo and reigning in the scope. Perhaps we have follow up issues to resolve further cleanup and expand that to resolve any other issues within core, namely the first two below?

* Extra line-breaks in function docblocks
* Missing semicolons at end or array lines
* Single quotes where not escaped and short array syntax

walangitan’s picture

walangitan CreditAttribution: walangitan at Chromatic commented
Status: Needs work » Needs review
joelpittet’s picture

joelpittet
Primary language English
Location Vancouver
CreditAttribution: joelpittet commented
Status: Needs review » Reviewed & tested by the community
Issue tags: +Documentation
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Chapter Three commented
Status: Reviewed & tested by the community » Fixed

Committed ca055a3 and pushed to 8.0.x. Thanks!

@walangitan there are plenty of coding standards issue in the queue - a good place to start is #2571965: [meta] Fix PHP coding standards in core

  • alexpott committed 09dc9cb on 8.1.x 
    Issue #2646962 by walangitan, klidifia: AjaxPageStateTest typo and test...

  • alexpott committed ca055a3 on 
    Issue #2646962 by walangitan, klidifia: AjaxPageStateTest typo and test...

Status: Fixed » Closed (fixed)

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