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

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Reroll the patch if it no longer applies. Novice Instructions
CommentFileSizeAuthor
#25 comment-module-pass-coder-review-1532692.patch2.23 KBTomefa
#20 comment-module-pass-coder-review-1532692-20.patch11.73 KBdeepakaryan1988
#17 drupal-core-make-comment-module-pass-coder-review-1532692-11.patch12.55 KBsandergo90
#15 drupal-core-make-comment-module-pass-coder-review-1532692-11.patch12.55 KBsandergo90
#12 drupal-core-make-comment-module-pass-coder-review-1532692-11.patch12.55 KBsandergo90
#10 drupal-core-make-comment-module-pass-coder-review-1532692-11.patch12.55 KBsandergo90
#5 comment-coder-fixes-5940134.patch7.98 KBTravisCarden
#4 comment-coder-fixes-1532692-4.patch8.41 KBTravisCarden
#3 drupal-1532692-3.patch10.84 KBTravisCarden
#3 interdiff.txt1.04 KBTravisCarden
#1 drupal-1532692-1.patch10.83 KBTravisCarden
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Active » Needs review
FileSize
drupal-1532692-1.patch10.83 KB
Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Status: Needs review » Needs work 
+++ b/core/modules/comment/comment.admin.incundefined
@@ -213,7 +222,11 @@ function comment_multiple_delete_confirm($form, &$form_state) {
+        '#value' => $cid,
+        '#prefix' => '<li>', '#suffix' => check_plain($subject) . '</li>',

The #suffix key needs to be on its own line here.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.04 KB
drupal-1532692-3.patch10.84 KB

Ah! Good catch; thank you.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
FileSize
comment-coder-fixes-1532692-4.patch8.41 KB

Here's an updated patch without the directive types, per the new direction of the larger initiative.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
FileSize
comment-coder-fixes-5940134.patch7.98 KB

Had to re-roll for upstream changes. Note, this patch is unaffected by #1539712: [policy, no patch] Coding standards for breaking function calls and language constructs across lines.

Lars Toomre’s picture

Lars Toomre CreditAttribution: Lars Toomre commented
Issue tags: -Novice, -Coding standards, -coder-fixes-2012

#5: comment-coder-fixes-5940134.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Novice, +Coding standards, +coder-fixes-2012

The last submitted patch, comment-coder-fixes-5940134.patch, failed testing.

TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Needs work » Postponed

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

sphism’s picture

sphism CreditAttribution: sphism commented
Status: Postponed » Active

We have the go ahead with all these issues again, see #1518116: [meta] Make Core pass Coder Review for more details

sandergo90’s picture

sandergo90 CreditAttribution: sandergo90 commented
Status: Active » Needs review
FileSize
drupal-core-make-comment-module-pass-coder-review-1532692-11.patch12.55 KB

I've tried to create all the correct coding standards. Attached the patch.

Status: Needs review » Needs work

The last submitted patch, drupal-core-make-comment-module-pass-coder-review-1532692-11.patch, failed testing.

sandergo90’s picture

sandergo90 CreditAttribution: sandergo90 commented
FileSize
drupal-core-make-comment-module-pass-coder-review-1532692-11.patch12.55 KB

Attached new patch for coding error.

sandergo90’s picture

sandergo90 CreditAttribution: sandergo90 commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, drupal-core-make-comment-module-pass-coder-review-1532692-11.patch, failed testing.

sandergo90’s picture

sandergo90 CreditAttribution: sandergo90 commented
Status: Needs work » Needs review
FileSize
drupal-core-make-comment-module-pass-coder-review-1532692-11.patch12.55 KB

New patch again.

Status: Needs review » Needs work

The last submitted patch, drupal-core-make-comment-module-pass-coder-review-1532692-11.patch, failed testing.

sandergo90’s picture

sandergo90 CreditAttribution: sandergo90 commented
Status: Needs work » Needs review
FileSize
drupal-core-make-comment-module-pass-coder-review-1532692-11.patch12.55 KB

Sorry for all the patches. Here's one again.

sandergo90’s picture

sandergo90 CreditAttribution: sandergo90 commented
Issue summary: View changes

Updated issue summary.

parthipanramesh’s picture

parthipanramesh CreditAttribution: parthipanramesh commented
Status: Needs review » Needs work

sorry but patch does not apply..

error: patch failed: core/modules/comment/comment.module:297
error: core/modules/comment/comment.module: patch does not apply
willieseabrook’s picture

willieseabrook CreditAttribution: willieseabrook commented
Issue tags: +TUNIS_2014_JANUARY, +SprintWeekend2014

Starting as part of SprintWeekend2014

deepakaryan1988’s picture

deepakaryan1988
Primary language Hindi
CreditAttribution: deepakaryan1988 commented
Assigned: Unassigned » deepakaryan1988
Status: Needs work » Needs review
FileSize
comment-module-pass-coder-review-1532692-20.patch11.73 KB

I have created patch for it.

willieseabrook’s picture

willieseabrook CreditAttribution: willieseabrook commented
Issue tags: -TUNIS_2014_JANUARY
TravisCarden’s picture

TravisCarden CreditAttribution: TravisCarden commented
Status: Needs review » Needs work

A few things here:

Please address these issues so someone can productively perform a closer code review. Thanks!

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented
Assigned: deepakaryan1988 » Unassigned
Issue summary: View changes
Issue tags: -coder-fixes-2012, -SprintWeekend2014 +Amsterdam2014

In the meantime, many functions have been removed from comment.module so that will make this patch smaller. (In addition to taking out the "@param" things that are in #1808478: Add missing type hinting to Comment module docblocks now, as comment#22 said.) Find me if there are questions about comment.module stuff.

Rerolling this and seeing what is left of the patch after that, should be a novice issue.

Tomefa’s picture

Tomefa CreditAttribution: Tomefa commented

Rerolling the patch at Drupalcon Amsterdam

Tomefa’s picture

Tomefa CreditAttribution: Tomefa commented
Status: Needs work » Needs review
FileSize
comment-module-pass-coder-review-1532692.patch2.23 KB

Here is the reroll of the patch.
I didn't resolve any coding standard issue, just rerolling it and make it work for the actual branch 8.0.x

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented
Status: Needs review » Needs work

Thanks for the reroll. But it is still 'needs work' to address the comments in #22

xjm’s picture

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

pfrenssen’s picture

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