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
CommentFileSizeAuthor
#21 drupal-make_config_module_pass_coder_review-1533202-21.patch62.18 KBvisabhishek
#19 drupal-make_config_module_pass_coder_review-1533202-19.patch62.08 KBvisabhishek
#8 config_standardized-1533202-5900836.patch6.02 KBfluxsauce
#2 config_standardized-1533202.patch8.51 KBfluxsauce

Comments

fluxsauce’s picture

fluxsauce commented
Assigned: Unassigned » fluxsauce
fluxsauce’s picture

fluxsauce commented
Status: Needs review » Active
StatusFileSize
new config_standardized-1533202.patch8.51 KB

Completed.

fluxsauce’s picture

fluxsauce commented
Status: Active » Needs review
traviscarden’s picture

traviscarden commented

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

fluxsauce commented

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

fluxsauce’s picture

fluxsauce commented
Status: Active » Needs review

Summarized.

fluxsauce’s picture

fluxsauce commented
Issue summary: View changes

Summarizing work done

fluxsauce’s picture

fluxsauce commented
Issue summary: View changes

Bum HTML

NROTC_Webmaster’s picture

NROTC_Webmaster commented
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

fluxsauce commented
Status: Needs work » Needs review
StatusFileSize
new config_standardized-1533202-5900836.patch6.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

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Needs work
Issue tags: +Configuration system

Patch no longer applies.

fluxsauce’s picture

fluxsauce commented

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

tomogden’s picture

tomogden commented
tomogden’s picture

tomogden commented

Should we postpone this then?

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Status: Needs work » Postponed

Yes.

lars toomre’s picture

lars toomre commented

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

fluxsauce commented
Assigned: fluxsauce » Unassigned
fluxsauce’s picture

fluxsauce commented
Issue summary: View changes

Cleanup

traviscarden’s picture

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

visabhishek commented
Assigned: Unassigned » visabhishek
visabhishek’s picture

visabhishek commented
Status: Active » Needs review
StatusFileSize
new drupal-make_config_module_pass_coder_review-1533202-19.patch62.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

visabhishek commented
Status: Needs work » Needs review
StatusFileSize
new drupal-make_config_module_pass_coder_review-1533202-21.patch62.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

visabhishek commented
Assigned: visabhishek » Unassigned
kadimi’s picture

kadimi commented

8: config_standardized-1533202-5900836.patch queued for re-testing.

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

mhaamann’s picture

mhaamann commented

2: config_standardized-1533202.patch queued for re-testing.

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

mhaamann’s picture

mhaamann commented
Status: Needs work » Needs review

21: drupal-make_config_module_pass_coder_review-1533202-21.patch queued for re-testing.

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

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.

jhodgdon’s picture

jhodgdon
she/her
Primary language English
commented
Component: configuration system » config.module
pfrenssen’s picture

pfrenssen
Location Sofia
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.