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

  • Yellow warning from Drupal 8 branch test code review (one normal reported, not enumerated)
  • No errors or warnings reported using Coder 8
    • Minor: Drupal Coding Standards, Drupal Commenting Standards, Drupal SQL Standards, Drupal Security Checks, Internationalization
  • No errors or warnings reported using Drupal Code Sniffer
    • phpcs --standard=Drupal --extensions=php,module,inc,install,test,profile,theme
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

FluxSauce’s picture

Assigned: Unassigned » FluxSauce
FluxSauce’s picture

Status: Needs review » Active
FileSize
8.51 KB

Completed.

FluxSauce’s picture

Status: Active » Needs review
TravisCarden’s picture

Per the instructions in the meta issue, @FluxSauce, everything you have in comment #2 should be moved to the issue summary, and you don't need to document what you fixed—only reported errors that you didn't fix. See an example at #1512434: Make Aggregator module pass Coder Review.

FluxSauce’s picture

Sorry about that @TravisCarden, I'll reconcile shortly. Thanks for the feedback.

FluxSauce’s picture

Status: Active » Needs review

Summarized.

FluxSauce’s picture

Issue summary: View changes

Summarizing work done

FluxSauce’s picture

Issue summary: View changes

Bum HTML

NROTC_Webmaster’s picture

Status: Needs review » Needs work

A couple of things.

According, to http://drupal.org/node/811254 getInfo() should not be translated with t().
While it is still being debated #338403: Use {@inheritdoc} on all class methods (including tests) for now we are not adding docblocks to getInfo(), setUp(), and tearDown().

Refer to http://drupal.org/node/1354 for all documentation standards, and http://drupal.org/node/325974 for SimpleTest specific standards.

FluxSauce’s picture

Status: Needs work » Needs review
FileSize
6.02 KB

@NROTC_Webmaster thanks for the review, I've corrected the issues as described. I made the getInfo() translate error elsewhere as well; glad I know about it now.

sun’s picture

Status: Needs review » Needs work
Issue tags: +Configuration system

Patch no longer applies.

FluxSauce’s picture

Not updating until #1518116: [meta] Make Core pass Coder Review is no longer postponed.

tomogden’s picture

tomogden’s picture

Should we postpone this then?

jhodgdon’s picture

Status: Needs work » Postponed

Yes.

Lars Toomre’s picture

When a patch for this issue is next re-rolled, we need to ensure that a blank line is added after the last method and closing brace of the class for test classes like LocaleConfigOverride.php.

FluxSauce’s picture

Assigned: FluxSauce » Unassigned
FluxSauce’s picture

Issue summary: View changes

Cleanup

TravisCarden’s picture

Issue summary: View changes
Status: Postponed » Active
visabhishek’s picture

Assigned: Unassigned » visabhishek
visabhishek’s picture

Status: Active » Needs review
FileSize
62.08 KB

I have Created a patch. Please review.

Status: Needs review » Needs work

The last submitted patch, 19: drupal-make_config_module_pass_coder_review-1533202-19.patch, failed testing.

visabhishek’s picture

Status: Needs work » Needs review
FileSize
62.18 KB

Patch updated.

Status: Needs review » Needs work

The last submitted patch, 21: drupal-make_config_module_pass_coder_review-1533202-21.patch, failed testing.

visabhishek’s picture

Assigned: visabhishek » Unassigned
kadimi’s picture

The last submitted patch, 8: config_standardized-1533202-5900836.patch, failed testing.

mhaamann’s picture

The last submitted patch, 2: config_standardized-1533202.patch, failed testing.

mhaamann’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 21: drupal-make_config_module_pass_coder_review-1533202-21.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.

jhodgdon’s picture

Component: configuration system » config.module
pfrenssen’s picture

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.