Part of meta-issue #1518116: [meta] Make Core pass Coder Review

  • Yellow warning Drupal 8 branch test code review tab (3 Minor, 0 Critical, 1 Normal reported, not enumerated)
  • Drupal Code Sniffer reports two errors with the latest patch applied:
    FILE: /home/quickstart/websites/d8.dev/core/modules/toolbar/toolbar.css
    --------------------------------------------------------------------------------
    FOUND 2 ERROR(S) AFFECTING 1 LINE(S)
    --------------------------------------------------------------------------------
    45 | ERROR | Each style definition must be on a line by itself
    45 | ERROR | Expected 1 space after colon in style definition; 0 found
    --------------------------------------------------------------------------------

    These refer to the following line and are both false positives.
    filter: progid:DXImageTransform.Microsoft.Shadow(color=#000000, direction='180', strength='10');

  • Coder Review finds no problems at the minor (strictest) severity warning level.
Files: 
CommentFileSizeAuthor
#7 toolbar_standardized-1533404-7.patch3.89 KBAlbert Volkman
PASSED: [[SimpleTest]]: [MySQL] 36,805 pass(es).
[ View ]
#2 toolbar_standardized-1533404.patch4.36 KBFluxSauce
PASSED: [[SimpleTest]]: [MySQL] 35,368 pass(es).
[ View ]

Comments

FluxSauce’s picture

Assigned:Unassigned» FluxSauce
FluxSauce’s picture

StatusFileSize
new4.36 KB
PASSED: [[SimpleTest]]: [MySQL] 35,368 pass(es).
[ View ]

Completed.

TravisCarden’s picture

Status:Active» Needs review
FluxSauce’s picture

Summarized, sorry about that.

Jody Lynn’s picture

Status:Needs review» Reviewed & tested by the community

Confirmed that all changes are style only and are correct coding standards.

TravisCarden’s picture

Status:Reviewed & tested by the community» Needs work

I'm sorry. Per the initiative, adding param/return datatypes will be handled in a separate issue, so those changes need to be removed from the patch. Additionally, there are still style issues to fix:

FILE: /home/quickstart/websites/d8.dev/core/modules/toolbar/toolbar-rtl.css
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
22 | ERROR | Multiple CSS properties should be listed in alphabetical order
--------------------------------------------------------------------------------

FILE: /home/quickstart/websites/d8.dev/core/modules/toolbar/toolbar.css
--------------------------------------------------------------------------------
FOUND 6 ERROR(S) AFFECTING 5 LINE(S)
--------------------------------------------------------------------------------
  45 | ERROR | Each style definition must be on a line by itself
  45 | ERROR | Expected 1 space after colon in style definition; 0 found
100 | ERROR | Expected 1 space after colon in style definition; 2 found
103 | ERROR | Expected 1 space after colon in style definition; 2 found
107 | ERROR | Expected 1 space after colon in style definition; 2 found
133 | ERROR | Multiple CSS properties should be listed in alphabetical order
--------------------------------------------------------------------------------

Be sure to be using the latest copy of the code sniffer, by the way. It moves fast. I always update mine via Git before running it.

Albert Volkman’s picture

Status:Needs work» Needs review
StatusFileSize
new3.89 KB
PASSED: [[SimpleTest]]: [MySQL] 36,805 pass(es).
[ View ]

Param/return datatypes removed. Additional coder issues addressed except for the first issue in toolbar-rtl.css. It looks correct to me, so possibly a false positive?

Albert Volkman’s picture

Issue summary:View changes

Summarizing work done

TravisCarden’s picture

Status:Needs review» Reviewed & tested by the community

Issue status updated with latest drupalcs report.

Thank you, @Albert Volkman, you're right; both errors are false positives. Would you like to create issues for them and add links to them in the summary here?

Albert Volkman’s picture

jhodgdon’s picture

I thought we weren't in agreement on the coding standard yet, regarding this change:

-  $admin_link = db_query('SELECT * FROM {menu_links} WHERE menu_name = :menu_name AND module = :module AND link_path = :path', array(':menu_name' => 'management', ':module' => 'system', ':path' => 'admin'))->fetchAssoc();
+  $args = array(
+    ':menu_name' => 'management',
+    ':module' => 'system',
+    ':path' => 'admin',
+  );
+  $admin_link = db_query('SELECT * FROM {menu_links} WHERE menu_name = :menu_name AND module = :module AND link_path = :path', $args)->fetchAssoc();

I'm also confused about these changes (two like this) -- why are they necessary/desirable?

+++ b/core/modules/toolbar/toolbar-rtl.css
@@ -18,8 +18,8 @@
   float: left;
}
#toolbar ul#toolbar-user li {
-  float: none;
   display: inline;
+  float: none;
TravisCarden’s picture

Status:Reviewed & tested by the community» Postponed

Oops. I'm sorry, @jhodgdon: that first one got by me.

As to the second one, drupalcs complains about CSS properties that aren't in alphabetical order.

jhodgdon’s picture

Alphabetical CSS -- OK -- I see that on the CSS coding standards page: http://drupal.org/node/302199

FluxSauce’s picture

That's why I haven't rolled any new patches recently, I thought this whole thing was postponed. Kind of frustrating, really, I do want to contribute.

FluxSauce’s picture

Assigned:FluxSauce» Unassigned
FluxSauce’s picture

Issue summary:View changes

Updated issue status.

mgifford’s picture

Issue summary:View changes
Status:Postponed» Needs work

I think there's agreement on the coding standards now https://www.drupal.org/node/1886770

xjm’s picture

Version:8.0.x-dev» 8.1.x-dev
Priority:Normal» Minor
Status:Needs work» Postponed
Issue tags:-Novice

Thanks for all the work here so far. See #1518116-86: [meta] Make Core pass Coder Review. This issue is postponed until the meta issue is either closed or reopened.