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
StatusFileSize
new15.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

dinathecool’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
StatusFileSize
new8.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.