Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Related Issues
#731724: Convert comment settings into a field to make them work with CMI and non-node entities
Comment | File | Size | Author |
---|---|---|---|
#35 | image.png | 12.16 KB | larowlan |
#35 | dont-credit-me-for-this-trivial-shit.patch | 615 bytes | larowlan |
#31 | comment-docs.30.patch | 579 bytes | larowlan |
#26 | drupal8.comment-module.2083895-26.patch | 5.67 KB | andypost |
Comments
Comment #1
andypostThat's would be enough
Comment #2
larowlanwow line 82!
Missing a comment for the @param and @return? can't be sure, might be just missing the context
Comment #3
andypostand more fixes
Comment #4
larowlanunless bot argues
Comment #5
alexpottPatch no longer applies.
Comment #6
larowlanStraight re-roll
Comment #7
webchickNo longer applies again, sorry. :(
Comment #8
andypostand again
Comment #9
webchickPatch no longer applies.
Comment #10
andypostComment #11
andypostProbably we should update patch after #1772420: [policy only] Documentation standard for arrays of a certain type (param/return types)
Comment #12
jibranFixed #11
Comment #13
larowlanFound 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.
Comment #14
andypostFixed
Comment #15
larowlanunless bot disagrees
Comment #17
jibranReroll of #12 same intediff as #14.
Comment #18
andypostSo back to RTBC
Comment #19
andypostComment #20
David Hernández CreditAttribution: David Hernández commentedWorking on this.
Comment #21
David Hernández CreditAttribution: David Hernández commentedRe-rolled. Looks like a big parts of the change are already there.
Comment #22
andypostAwesome! just 2 nitpicks
should also remove 'use Drupal\comment\Entity\Comment;'
s/node/entity - comment decoupled from nodes.
Comment #23
andypostSecond hunk is not valid while history module works for nodes only
Comment #24
jibran23: drupal8.comment-module.2083895-23.patch queued for re-testing.
Comment #26
andypostre-roll
Comment #27
larowlanstill good
Comment #28
webchickI'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!
Comment #29
jhodgdonint[] is actually a standard:
https://drupal.org/node/1354#types
This isn't right though:
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...
Comment #30
larowlanWill do
Comment #31
larowlantrivial patch
Comment #32
andypostNappy new year! ;)
Comment #33
webchickHooray! :)
Committed and pushed to 8.x. Thanks!
Comment #34
jhodgdonThe patch was too trivial -- it didn't go through the previous patch and find all the mistakes. Besides the FALSE, there was also 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.
Comment #35
larowlanha
Comment #36
jhodgdon:) Thanks
Comment #37
webchickLOL :) 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