Updated: Comment #0

Problem/Motivation

This is a prerequisite for #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Currently patch is big because fixes a lot of doc blocks and other code-comments

Proposed resolution

Incorporate changes from current sandbox

Remaining tasks

file patch

User interface changes

no

API changes

no

#731724: Convert comment settings into a field to make them work with CMI and non-node entities

Comments

andypost’s picture

Status:Active» Needs review
StatusFileSize
new12.65 KB
PASSED: [[SimpleTest]]: [MySQL] 58,470 pass(es).
[ View ]

That's would be enough

larowlan’s picture

  1. +++ b/core/modules/comment/comment.module
    @@ -78,8 +76,6 @@
    -use Drupal\comment\Entity\Comment;

    wow line 82!

  2. +++ b/core/modules/comment/comment.module
    @@ -1408,9 +1403,9 @@ function comment_get_display_page($cid, $node_type) {
    + * @param \Drupal\comment\CommentInterface $comment

    Missing a comment for the @param and @return? can't be sure, might be just missing the context

andypost’s picture

StatusFileSize
new14.59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,863 pass(es).
[ View ]
new4.7 KB

and more fixes

larowlan’s picture

Status:Needs review» Reviewed & tested by the community

unless bot argues

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

larowlan’s picture

Status:Needs work» Reviewed & tested by the community
StatusFileSize
new13.62 KB
PASSED: [[SimpleTest]]: [MySQL] 58,740 pass(es).
[ View ]

Straight re-roll

webchick’s picture

Status:Reviewed & tested by the community» Needs work

No longer applies again, sorry. :(

andypost’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new14.28 KB
PASSED: [[SimpleTest]]: [MySQL] 59,324 pass(es).
[ View ]

and again

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

andypost’s picture

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new14.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,149 pass(es).
[ View ]
andypost’s picture

Status:Reviewed & tested by the community» Needs review
jibran’s picture

StatusFileSize
new1.79 KB
new14.46 KB
PASSED: [[SimpleTest]]: [MySQL] 58,952 pass(es).
[ View ]

Fixed #11

larowlan’s picture

Status:Needs review» Needs work

Found another doc-block issue:
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

// Second reply to comment #3 creating comment #4.>

But is actually in reply to comment #2.

Can we fix here to minimize confusion in #731724: Convert comment settings into a field to make them work with CMI and non-node entities (see #1907960-273: Helper issue for "Comment field" - point 5.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new15.44 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.comment-module.2083895-14.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1010 bytes

Fixed

larowlan’s picture

Status:Needs review» Reviewed & tested by the community

unless bot disagrees

Status:Reviewed & tested by the community» Needs work

The last submitted patch, drupal8.comment-module.2083895-14.patch, failed testing.

jibran’s picture

Status:Needs work» Needs review
StatusFileSize
new1010 bytes
new15.25 KB
PASSED: [[SimpleTest]]: [MySQL] 58,561 pass(es).
[ View ]

Reroll of #12 same intediff as #14.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

So back to RTBC

andypost’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll
David Hernández’s picture

Assigned:Unassigned» David Hernández

Working on this.

David Hernández’s picture

Assigned:David Hernández» Unassigned
Status:Needs work» Needs review
StatusFileSize
new6.1 KB
PASSED: [[SimpleTest]]: [MySQL] 59,046 pass(es).
[ View ]

Re-rolled. Looks like a big parts of the change are already there.

andypost’s picture

Status:Needs review» Needs work
Issue tags:-Needs reroll

Awesome! just 2 nitpicks

  1. +++ b/core/modules/comment/comment.module
    @@ -134,7 +134,7 @@ function comment_entity_bundle_info() {
    -function comment_uri(Comment $comment) {
    +function comment_uri(CommentInterface $comment) {

    should also remove 'use Drupal\comment\Entity\Comment;'

  2. +++ b/core/modules/comment/comment.module
    @@ -1259,10 +1259,9 @@ function comment_load($cid, $reset = FALSE) {
    + *   Time to count from (defaults to time of last user access to node).

    s/node/entity - comment decoupled from nodes.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new6.39 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.comment-module.2083895-23.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new499 bytes

Second hunk is not valid while history module works for nodes only

jibran’s picture

Status:Needs review» Needs work

The last submitted patch, 23: drupal8.comment-module.2083895-23.patch, failed testing.

andypost’s picture

Issue summary:View changes
Status:Needs work» Needs review
StatusFileSize
new5.67 KB
PASSED: [[SimpleTest]]: [MySQL] 59,525 pass(es).
[ View ]

re-roll

larowlan’s picture

Status:Needs review» Reviewed & tested by the community

still good

webchick’s picture

Component:comment.module» documentation
Status:Reviewed & tested by the community» Fixed
+++ b/core/modules/comment/comment.module
@@ -603,7 +602,7 @@ function comment_add(EntityInterface $entity, $field_name = 'comment', $pid = NU
- * @return
+ * @return int[]

@@ -304,10 +303,10 @@ function comment_permission() {
- * @return
+ * @return \Drupal\comment\CommentInterface[]|array

I'm not familiar with "datatype[]" as a standard, and http://manual.phpdoc.org/HTMLSmartyConverter/HandS/phpDocumentor/tutoria... doesn't seem to imply they are valid. However, I was able to find similar examples in PHPUnit and Symfony, so I guess this is fine.

Retroactively sending to the "documentation" queue in case jhodgdon wants to push back on that.

Committed and pushed to 8.x. Thanks!

jhodgdon’s picture

Status:Fixed» Needs work

int[] is actually a standard:
https://drupal.org/node/1354#types

This isn't right though:

+ * @return int|FALSE

Should have been lower-case "false". Can someone check over the patch again and make a quick follow-up or a new issue if more appropriate? Shouldn't have been committed as it was...

larowlan’s picture

Will do

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new579 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,874 pass(es).
[ View ]

trivial patch

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Nappy new year! ;)

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Hooray! :)

Committed and pushed to 8.x. Thanks!

jhodgdon’s picture

Status:Fixed» Needs work

The patch was too trivial -- it didn't go through the previous patch and find all the mistakes. Besides the FALSE, there was also NULL:

+ * @return array|NULL

which I didn't call out and I was hoping someone would check over the previous patch carefully before the cleanup was committed. Can we have another one please? I think that's it.

larowlan’s picture

Status:Needs work» Needs review
StatusFileSize
new615 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,369 pass(es).
[ View ]
new12.16 KB

ha
tell him what he's won dave

jhodgdon’s picture

Status:Needs review» Reviewed & tested by the community

:) Thanks

webchick’s picture

Status:Reviewed & tested by the community» Fixed

LOL :) no worries, i missed that too.

Committed and pushed to 8.x, and I think I'll stay out of the docs queue for awhile. :D

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.