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
      --------------------------------------------------------------------------------
Files: 
CommentFileSizeAuthor
#8 translation_standardized-1533440-5918390.patch8.49 KBFluxSauce
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch translation_standardized-1533440-5918390.patch. Unable to apply patch. See the log in the details link for more information. View
#2 translation_standardized-1533440.patch15.5 KBFluxSauce
PASSED: [[SimpleTest]]: [MySQL] 35,421 pass(es). View

Comments

FluxSauce’s picture

Assigned: Unassigned » FluxSauce
FluxSauce’s picture

Status: Active » Needs review
FileSize
15.5 KB
PASSED: [[SimpleTest]]: [MySQL] 35,421 pass(es). View

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

Issue summary: View changes

Report of completed work and false positives

dinakaran.ilango’s picture

Status: Needs review » Reviewed & tested by the community

patch at #2 works for me

lotyrin’s picture

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

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

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

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

Issue summary: View changes

Minor markup cleanup

FluxSauce’s picture

Status: Needs work » Needs review
FileSize
8.49 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch translation_standardized-1533440-5918390.patch. Unable to apply patch. See the log in the details link for more information. View

Resubmitting without type hinting.

NROTC_Webmaster’s picture

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

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

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

Assigned: FluxSauce » Unassigned
FluxSauce’s picture

Issue summary: View changes

Updating with revised patch results.

TravisCarden’s picture

Status: Postponed » Active
valthebald’s picture

Issue tags: +dcwroc2014

We are going to work on this issue during DCWroclaw 2014

Status: Active » Needs review

Status: Needs review » Needs work

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

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.

tatarbj’s picture

Status: Postponed » Closed (duplicate)

Closing in favor of #2571965: [meta] Fix 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.