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

  • Failed Drupal 8 branch test code review tab (one critical reported in core/modules/translation/translation.module)
  • No errors or warnings reported using Coder 8
    • Minor: Drupal Coding Standards, Drupal Commenting Standards, Drupal SQL Standards, Drupal Security Checks, Internationalization
  • Multiple false positives reported using Drupal Code Sniffer
    • phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme
    • The "Missing parameter type" and "Data type of return value" errors are considered out of scope as per #1518116: [meta] Make Core pass Coder Review. The "Missing function doc comment" errors are currently considered a false positive, reported in #1545476: false positive "Missing function doc comment" error on Simple Test inherited methods.
    • FILE: /media/sf_sandbox/core/modules/translation/translation.module
--------------------------------------------------------------------------------
FOUND 8 ERROR(S) AFFECTING 8 LINE(S)
--------------------------------------------------------------------------------
  80 | ERROR | Missing parameter type at position 1
  83 | ERROR | Data type of return value is missing
 400 | ERROR | Missing parameter type at position 1
 437 | ERROR | Missing parameter type at position 1
 441 | ERROR | Data type of return value is missing
 471 | ERROR | Data type of return value is missing
 481 | ERROR | Missing parameter type at position 1
 484 | ERROR | Data type of return value is missing
--------------------------------------------------------------------------------

FILE: /media/sf_sandbox/core/modules/translation/translation.pages.inc
--------------------------------------------------------------------------------
FOUND 1 ERROR(S) AFFECTING 1 LINE(S)
--------------------------------------------------------------------------------
 16 | ERROR | Data type of return value is missing
--------------------------------------------------------------------------------

FILE: /media/sf_sandbox/core/modules/translation/translation.test
--------------------------------------------------------------------------------
FOUND 26 ERROR(S) AFFECTING 26 LINE(S)
--------------------------------------------------------------------------------
  18 | ERROR | Missing function doc comment
  26 | ERROR | Missing function doc comment
 268 | ERROR | Missing parameter type at position 1
 271 | ERROR | Data type of return value is missing
 281 | ERROR | Missing parameter type at position 1
 312 | ERROR | Missing parameter type at position 1
 314 | ERROR | Missing parameter type at position 2
 316 | ERROR | Missing parameter type at position 3
 319 | ERROR | Data type of return value is missing
 345 | ERROR | Missing parameter type at position 2
 347 | ERROR | Missing parameter type at position 3
 349 | ERROR | Missing parameter type at position 4
 352 | ERROR | Data type of return value is missing
 380 | ERROR | Missing parameter type at position 1
 386 | ERROR | Missing parameter type at position 3
 388 | ERROR | Missing parameter type at position 4
 390 | ERROR | Missing parameter type at position 5
 393 | ERROR | Data type of return value is missing
 404 | ERROR | Missing parameter type at position 1
 406 | ERROR | Missing parameter type at position 2
 408 | ERROR | Missing parameter type at position 3
 410 | ERROR | Missing parameter type at position 4
 413 | ERROR | Data type of return value is missing
 462 | ERROR | Missing parameter type at position 1
 468 | ERROR | Missing parameter type at position 3
 471 | ERROR | Data type of return value is missing
--------------------------------------------------------------------------------
CommentFileSizeAuthor
#8 translation_standardized-1533440-5918390.patch8.49 KBfluxsauce
#2 translation_standardized-1533440.patch15.5 KBfluxsauce

Comments

fluxsauce’s picture

fluxsauce commented
Assigned: Unassigned » fluxsauce
fluxsauce’s picture

fluxsauce commented
Status: Active » Needs review
StatusFileSize
new translation_standardized-1533440.patch15.5 KB

Completed.

In case there's any question, in translation.module I switched COUNT(*) to COUNT(nid), the primary key because ">coder correctly flagged it as a warning.

fluxsauce’s picture

fluxsauce commented
Issue summary: View changes

Report of completed work and false positives

dinakaran.ilango’s picture

dinakaran.ilango commented
Status: Needs review » Reviewed & tested by the community

patch at #2 works for me

lotyrin’s picture

lotyrin commented
Status: Reviewed & tested by the community » Needs work

This patch is adding types to documentation blocks. AFAIK, that's supposed to be part of a separate meta issue.

lars toomre’s picture

lars toomre commented

Yup.. Adding type hinting is too hard for this sprint of patches. The committer is too concerned about enough time to correctly review all of the functions in this module.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented

We're not including the param/return types in this effort any more -- see #1518116: [meta] Make Core pass Coder Review -- too difficult to review for accuracy.

fluxsauce’s picture

fluxsauce commented

I will re-do the patch without the type hinting.

For the record, the stipulation that type hinting was to be excluded was made after I wrote the patch (24/04/2012 - 21:33).

fluxsauce’s picture

fluxsauce commented
Issue summary: View changes

Minor markup cleanup

fluxsauce’s picture

fluxsauce commented
Status: Needs work » Needs review
StatusFileSize
new translation_standardized-1533440-5918390.patch8.49 KB

Resubmitting without type hinting.

NROTC_Webmaster’s picture

NROTC_Webmaster commented

All of the changes look good but I didn't apply the patch. Does it pass with the exception of the datatype errors produced? If it doesn't can you say what errors are still produced.

fluxsauce’s picture

fluxsauce commented

Does it pass with the exception of the datatype errors produced? If it doesn't can you say what errors are still produced.

Yes. I listed the errors that are still reported in the issue description. To summarize:

traviscarden’s picture

traviscarden commented
Status: Needs review » Postponed

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

fluxsauce’s picture

fluxsauce commented
Assigned: fluxsauce » Unassigned
fluxsauce’s picture

fluxsauce commented
Issue summary: View changes

Updating with revised patch results.

traviscarden’s picture

traviscarden commented
Status: Postponed » Active
valthebald’s picture

valthebald
Location Sofia
commented
Issue tags: +dcwroc2014

We are going to work on this issue during DCWroclaw 2014

Status: Active » Needs review

masterremaster queued 8: translation_standardized-1533440-5918390.patch for re-testing.

Status: Needs review » Needs work

The last submitted patch, 8: translation_standardized-1533440-5918390.patch, failed testing.

xjm’s picture

xjm
she/her
Primary language English
commented
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.

tatarbj’s picture

tatarbj
Primary language Hungarian
Location Brussels
commented
Status: Postponed » Closed (duplicate)

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