Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Status: Active » Needs review
FileSize
12.65 KB

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

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
FileSize
13.62 KB

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
FileSize
14.28 KB

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
FileSize
14.26 KB
andypost’s picture

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

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
FileSize
15.44 KB
1010 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
FileSize
1010 bytes
15.25 KB

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
FileSize
6.1 KB

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
FileSize
6.39 KB
499 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

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
FileSize
579 bytes

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
FileSize
615 bytes
12.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.