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

CommentFileSizeAuthor
#19 entity-coder_review-1533230-19.patch5.27 KBlittledynamo
#11 entity-coder_review-1533230-11.patch17.74 KBcs_shadow
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

NROTC_Webmaster’s picture

NROTC_Webmaster CreditAttribution: NROTC_Webmaster commented

Cross-referencing to #1315340: Clean up API docs for entity.inc to ensure it gets closed before a backport is attempted

infiniteluke’s picture

infiniteluke CreditAttribution: infiniteluke commented
Assigned: Unassigned » infiniteluke
infiniteluke’s picture

infiniteluke CreditAttribution: infiniteluke commented

1. Checked against http://qa.drupal.org/8.x-status

modules/entity/entity.module

  • 3 minor, 1 normal

modules/entity/entity.query.inc

  • 22 minor, 1 normal

2. The issues weren't itemized so I moved on to run CodeSniffer to see if this would throw anything. CodeSniffer returned nothing so I assume that means it complies.

3. Coder module on minor returned a couple warnings:

Line 46: global variables should start with a single underscore followed by the module and another underscore

  global $language_interface;

severity: minorLine 227: @see should always be followed by a filename, a URL, class/interface name (optionally including method), or a function name including ().

 * @see DrupalEntityControllerInterface

severity: minorLine 228: @see should always be followed by a filename, a URL, class/interface name (optionally including method), or a function name including ().

 * @see DrupalDefaultEntityController

severity: minorLine 229: @see should always be followed by a filename, a URL, class/interface name (optionally including method), or a function name including ().

 * @see EntityFieldQuery

Are the issues returned by Coder legitimate? Should documentation be included?

NROTC_Webmaster’s picture

NROTC_Webmaster CreditAttribution: NROTC_Webmaster commented

The first issue doesn't apply to core apparently. That is only for contrib modules.
The @see issue has already been filed and should not be changed to pass coder.

Regarding the codesniffer I am surprised it didn't return anything. Normally it will return more things than coder not less. You should probably check your settings to make sure it is actually validating based on the drupal standards.

infiniteluke’s picture

infiniteluke CreditAttribution: infiniteluke commented

From drupal/core/modules I ran
phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme entity > result.txt

result.txt is empty.

I ran the same command against the help module and got a ton of results.

NROTC_Webmaster’s picture

NROTC_Webmaster CreditAttribution: NROTC_Webmaster commented

If that's the case I'm OK with closing this but I would like to check with TravisCarden first.

NROTC_Webmaster’s picture

NROTC_Webmaster CreditAttribution: NROTC_Webmaster commented

Here are the issues that I saw when I ran codesniffer.

  1. Arguments with default values must be at the end of the argument list /modules/entity/entity.class.inc line 219
  2. Expected "foreach (...) {\n"; found "foreach (...) {\n" /modules/entity/entity.class.inc line 331
  3. Doc comment for var $update does not match actual variable name $entity at position 1 /modules/entity/entity.controller.inc line 558
  4. Opening class brace must be on a line by itself /modules/entity/entity.controller.inc line 449
  5. Expected 1 space after "=>"; 0 found /modules/entity/entity.query.inc line 604
  6. Inline comments must end in full-stops, exclamation marks, or question marks /modules/entity/entity.query.inc line 603
  7. Missing file doc comment /modules/entity/entity.query.inc line 2
  8. Line exceeds 80 characters; contains 91 characters /modules/entity/entity.class.inc line 380

In addition you should get a lot of missing param/return dataypes missing which we are not fixing.
EDIT: The foreach has two spaces between the ) and {

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Assigned: Unassigned » infiniteluke

Postponing till feature freeze. If you want to help in the meantime, please work on the blockers on the meta issue. Thanks!

infiniteluke’s picture

infiniteluke CreditAttribution: infiniteluke commented
Assigned: infiniteluke » Unassigned
Status: Active » Postponed
TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Issue summary: View changes
Status: Postponed » Active
cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Status: Active » Needs review
FileSize
entity-coder_review-1533230-11.patch17.74 KB

Fixed most of the codesniffer issues. Couple of issues remain where its asks to document functions getInfo() and setUp() but i guess we can skip those. Any suggestions are welcome.

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Assigned: infiniteluke » cs_shadow
cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented

Remaining output from codesniffer:

FILE: ...tml/drupal8/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
--------------------------------------------------------------------------------
FOUND 0 ERROR(S) AND 2 WARNING(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 116 | WARNING | Line exceeds 80 characters; contains 105 characters
 117 | WARNING | Line exceeds 80 characters; contains 109 characters
--------------------------------------------------------------------------------


FILE: ...8/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayModeTest.php
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 24 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------


FILE: ...upal8/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 26 | ERROR | Missing function doc comment
 34 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------


FILE: ...8/core/modules/entity/lib/Drupal/entity/Tests/EntityFormDisplayTest.php
--------------------------------------------------------------------------------
FOUND 2 ERROR(S) AFFECTING 2 LINE(S)
--------------------------------------------------------------------------------
 24 | ERROR | Missing function doc comment
 32 | ERROR | Missing function doc comment
--------------------------------------------------------------------------------
Jalandhar’s picture

Jalandhar CreditAttribution: Jalandhar commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

Patch #11 needs reroll.

cs_shadow’s picture

cs_shadow CreditAttribution: cs_shadow commented
Assigned: cs_shadow » Unassigned

Status: Needs work » Needs review

cjoy queued 11: entity-coder_review-1533230-11.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 11: entity-coder_review-1533230-11.patch, failed testing.

littledynamo’s picture

littledynamo CreditAttribution: littledynamo commented
Issue tags: +Amsterdam2014

I'll have a go at re-rolling this one ...

littledynamo’s picture

littledynamo CreditAttribution: littledynamo commented
FileSize
entity-coder_review-1533230-19.patch5.27 KB

Patch attached. Looks like Entity has been moved from /modules to /lib. Some of the files in the original patch don't seem to exist any more (entity.module and a few others).

The code should probably be ran though code sniffer again, but not sure how to do that for core? I usually use pareview.sh for contrib.

littledynamo’s picture

littledynamo CreditAttribution: littledynamo commented
Issue tags: -Needs reroll
littledynamo’s picture

littledynamo CreditAttribution: littledynamo commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: entity-coder_review-1533230-19.patch, failed testing.

Status: Needs work » Needs review

littledynamo queued 19: entity-coder_review-1533230-19.patch for re-testing.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm commented
Version: 8.0.x-dev » 8.1.x-dev
Priority: Normal » Minor
Status: Needs review » 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.

pfrenssen’s picture

pfrenssen
Location Sofia
CreditAttribution: pfrenssen commented
Status: Postponed » Closed (duplicate)

Closing in favor of #2571965: [meta] Fix PHP coding standards in core. In this issue the coding standards will be fixed on a sniff-per-sniff basis rather than a module-per-module basis.