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

FileSize
14.59 KB
PASSED: [[SimpleTest]]: [MySQL] 58,863 pass(es). View
4.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
FileSize
13.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
FileSize
14.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
FileSize
14.26 KB
PASSED: [[SimpleTest]]: [MySQL] 59,149 pass(es). View
andypost’s picture

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

FileSize
1.79 KB
14.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
FileSize
15.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
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
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
FileSize
6.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
FileSize
6.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
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

Issue summary:View changes
Status:Needs work» Needs review
FileSize
5.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
FileSize
579 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
FileSize
615 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,369 pass(es). View
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.