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.
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Meta issue: #1971384: [META] Convert page callbacks to controllers
Blocked by#1986606: Convert the comments administration screen to a view
Blocks #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface
UX Changes
No longer possible to sort the results by 'posted in' however in HEAD that is broken since #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Comment | File | Size | Author |
---|---|---|---|
#94 | comment-admin-1978904-94.patch | 29.81 KB | andypost |
#94 | interdiff.txt | 3.04 KB | andypost |
Comments
Comment #1
shanethehat CreditAttribution: shanethehat commentedFirst attempt at one of these. Not sure if I should do anything with the drupal_get_form calls?
Comment #2
shanethehat CreditAttribution: shanethehat commentedComment #4
shanethehat CreditAttribution: shanethehat commentedComment #6
shanethehat CreditAttribution: shanethehat commentedI believe the remaining fails are the fault of the delete form not being found. That form is being converted as part of #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface, so this should probably wait for that.
Comment #7
tim.plunkettFixing tags
Comment #8
kim.pepperWe need to sort out the crufty handling of comment_admin() in this issue, before we can fix #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface
Comment #9
kim.pepperWe could just make this a _form router item, and then create a separate one for confirming deleting over at #1946348: Convert all of confirm_form() in comment.admin.inc to the new form interface.
Can we pass multiple comment ids in the url to a the delete multiple form?
There must be an example of where we're doing this somewhere else.
Comment #10
tim.plunkettAh yes, I've done this in both #1895160: Convert admin/content to a View, keep a non-views fallback with no bulk operations and #1851086: Replace admin/people with a View by using tempstore.
From the admin/people issue:
Or similar.
Basically, you store the array of objects, redirect to the confirm form, retrieve them, and then go about as usual. Just don't forget to clear them out.
Comment #10.0
tim.plunkettAdded link to blocked issue
Comment #11
kim.pepperAfter discussion with @tim.plunkett in IRC, we've agreed to convert the
admin/comments page to a view with bulk operations #1986606: Convert the comments administration screen to a view, which should help (or
remove the need for) this issue.
Comment #12
pcambraAfter discussing with @dawehner, we need to do the fallback conversion of comment_admin in #1986606: Convert the comments administration screen to a view, marking this one as dupe
Comment #13
andypostDiscussed with @dawehner in IRC
EDIT: #2027115: Allow views to override existing routing items
Comment #14
jibranWorking on it.
Comment #15
andypostPlease separate this to own
AdminController.php
orCommentAdminController.php
because #731724: Convert comment settings into a field to make them work with CMI and non-node entities uses separateAdminController
for administrative tasksComment #16
jibranHere is the reroll. Yay!!
core/modules/comment/comment.admin.inc
is deleted.Comment #17
larowlanLooks great!
Extra line (nitpick)
Unicode::truncate()
Comment #19
jibranFixed #17 and tests. I have done some manually testing and it is working fine.
Comment #20
andypostGreat clean-up!
As I said before, please move administrative task to own AdminController.php!
Not sure it's right to add 'node_access' tag to other base table. see #2054993-19: Remove (untested, unused, broken) comment_get_recent()
Comment #21
jibranThanks for the review @andypost. Fixed #20.
Comment #22
jibranThe patch against #731724-503: Convert comment settings into a field to make them work with CMI and non-node entities. So that @larowlan don't have to merge :)
Comment #23
andypost#21 is RTBC, probably we could get #22 after comment-field patch
Comment #24
andypost#22 needs re-roll
Comment #25
jibranWorking on the reroll.
Comment #26
jibranReroll of #22.
Comment #28
jibranblahhh same fail as #16
Comment #29
andypostProbably defaults for route makes no sense here, so please add default argument
should be 'new' by default
Comment #30
jibranFixed #29.
Comment #32
andypostUh, last nitpick ;)
Needs - Defaults to 'new'.
Comment #33
andypost#30: drupal8.comment-module.1978904-30.patch queued for re-testing.
Comment #34
jibranFixed #32.
Comment #35
andypost@jibran thank you!
Comment #36
jibran#34: drupal8.comment-module.1978904-34.patch queued for re-testing.
Comment #37
jibranReroll after #2101155: Add CommentManagerInterface and minor doc fixes.
Comment #39
jibranSorry for the messed up reroll. Please find some bounce doc fixes. :)
Comment #40
larowlanIs this still correct?
Other than that looks fine
Comment #41
larowlanYeah that busts the tablesort headers on the comment-admin page, but could very well be broken in HEAD, lets fix it here.
Comment #42
jibranHow about removing it? IMO this
node_title
doesn't make sense anymore. Now it can betaxonomy_term_name
,user_name
or{$entity_type}_{$entity_info['entity_keys']['label']}
we can't do it for single table unless we make sperate table for each entity_type.Comment #43
larowlanYep, exactly like that.
For committers, this patch removes the ability to sort on 'posted in' from comment-admin screen, but since #731724: Convert comment settings into a field to make them work with CMI and non-node entities that has been broken as seen by #41
The interdiff at 42 is the wrong one but the patch itself is right.
Unless bot disagrees.
Comment #43.0
larowlanAdded link to 1986606
Comment #44
andypostAny idea why node access should be fired here? see 3.
Let's get rid of the join - we list comments!
We actually use 'cid, entity_type, entity_id' so other fields useless
Code comment makes no sense.
Here's a actual access checking so "node access" should be removed
There's
CommentManager::getParentEntityUri()
so maybe we get rid of the micro-optimization with loading commented entities?And let's change
s/$entity/$commented_entity
no reason to save comment entity when no changes made...
Also #2028025: Expand CommentInterface to provide methods will use
setPublished()
method for thatComment #45
andypostand the table 'node_field_data' have 'title' column that changed before #2057401: Make the node entity database schema sensible
Comment #46
jibranThanks for the review @andypost.
CommentManager::getParentEntityUri()
is worth using. $entity is gone and $commented_entity is in.Also added
comment_uri()
call because$comment->uri()
is not working :(.Comment #47
andypostAwesome! the
$comment->uri()
should work, so this needs debug because we need to get rid of this procedural wrapper and use https://drupal.org/node/2020491 so this hunk needs @todo with link to issueOtherwise this RTBC
Comment #48
jibranThis is beauty look at the interdiff. I love EFQ :D. Added @todo.
Comment #50
jibranNot
Drupal\Core\Entity\Query\QueryInterface
it isDrupal\Core\Entity\Query\QueryFactory
. blahh!!Comment #51
larowlanWe lost the bulk loading. Previous code grouped commented entity ids by type then loaded each type in bulk. New code calls load on each row so is far less performant. Besides that, great job on using efq.
Comment #52
jibranWell you are right about this but I EFQ will not return $entity_type or $entity_id.
We have to make a hard decision here #46 is without EFQ and green, #50 is with EFQ. So both comments maintainers are in the issue it is your call :).
Comment #53
larowlanCan't we load the comment entities using the results from efq and then loop those to create an array of commented entity ids, grouped by type, then load each group in one call to load multiple. That's basically what the old code did.
Comment #54
larowlanLike this
Comment #55
kim.pepperLooks good to me. Assuming @andypost and @larowlan are happy.
Comment #56
jibranI have the same idea but I am not at all sold out on looping
$comments
array twice. Bulk loading vs looping 50 comments twice.Comment #57
kim.pepperI vote for keeping the existing flow an optimising as a follow with real data.
Comment #58
larowlanI think the two loops is the far lesser evil than querying (and waiting for) the database over 200 times.
Consider a typical install. 90% of sites will only have comments on nodes.
They will also have menu module installed.
And path module.
And field module.
So to load one node you are looking at these queries
So minimum 4 database queries per node load.
Assuming there are no other contrib modules installed that also react to hook_node_load.
So with 50 nodes thats 200 database queries.
Each of those has the overhead of the DBTNG API and then the wait time for the query (and transfer time if separate db server).
With the patch at #54 there's only 4 queries - one call to node load (multiple).
I'll take 4 queries and two foreach loops through a 50-item array over 200 queries and one foreach loop any day.
Comment #59
jibranThanks for the detail explanation @larowlan. RTBC +1 for
Comment #60
andypost+1 RTBC
Not sure about 50&200 because most of times we get entities are loaded from cache, anyway this out of scope and this tuning could be done latter and suppose for views implementation
The only thing that left here is 'title callback' that uses
comment_count_unpublished()
so filed follow-up #2111357: Get rid of comment_count_unpublished() in favor of CommentStorage methodComment #61
andypostRe-roll after #2080563: Remove Unused local variable $cid from /core/modules/comment/comment.admin.inc
Comment #62
jibranMinor fix after #2010202-14: Deprecate comment_uri()
Comment #64
andypostNow I see what's wrong here:
1)
$comment->uri()
have no fragment and does not count proper page2)
comment_uri()
the same as above but adds fragment3)
CommentManager::getParentEntityUri()
is similar to$comment->permalink()
but have fragmentSo I recommend leave the issue as proper conversion and fix this URIs in #2010202: Deprecate comment_uri()
this change is not valid
Comment #65
andypost@jibran was right, we need 2 different URIs here
PS: related issue #2111419: Remove CommentManager::getParentEntityUri() in favor of Comment::permalink()
Comment #66
jibran#65: drupal8.comment-module.1978904-65.patch queued for re-testing.
Comment #68
jibran:/ patch no longer applies.
Comment #68.0
jibranadded note about sorting
Comment #69
andypostRe-roll
Simplified logic (delete without confirmation is not allowed)
Use form builder service
Comment #70
andypostThat's primary change
Comment #71
jibranWhat? Why are we unpublishing here? We should delete here.
Comment #72
jibranok I got it now. Let's improve the comment by adding @see \Drupal....::adminPage().
Comment #73
andyposthere it is
Comment #74
jibranIt is RTBC for me. Let's wait @larowlan for RTBC.
Comment #75
larowlanComment #76
Xano73: 1978904-comment_admin-71.patch queued for re-testing.
Comment #78
XanoRe-roll.
Comment #79
jibranThanks for the reroll @Xano. Back to RTBC.
Comment #80
andypost@Xano you forget to remove
comment.admin.inc
Patch just adds this hunk so leaving RTBC
Comment #81
XanoSharp, @andypost! I admit I just checked the reject files and forgot to go through Git's verbose output.
Comment #82
andypostRe-roll after #2102777: Allow theme_links to use routes as well as href
Comment #83
jibran82: 1978904-comment_admin-82.patch queued for re-testing.
Comment #85
larowlanreroll
Comment #86
jibranBack to RTBC.
Comment #87
pcambraPlain reroll, trying to unblock #1986606: Convert the comments administration screen to a view
Comment #88
andypostRTBC again
Comment #89
tim.plunkettWe must not have coverage for this at all. form_set_error now requires that $form_state be passed to it. (And you can use $this->setFormError now)
Comment #90
h3rj4n CreditAttribution: h3rj4n commentedChanged form_set_error to setFormError().
Comment #91
tim.plunkettThat was a 0 byte patch :)
Also it still needs tests.
Comment #92
h3rj4n CreditAttribution: h3rj4n commentedWhat kind of test does it need because some things are already checked in Drupal\comment\Tests\CommentAdminTest::testApprovalAdminInterface.
What should the test check? I've writed / experimented with some test so I'm able to write it but I'm not sure what to test. Should it just check (like CommentAdminTest::testCommentAdmin) that it returns 200? And check if the comment popups up when you add a comment? And after removing the content, check if it displays 'No comments available' again?
Reuploaded the patch WITH content this time ;)
Comment #93
andypostI think Tim means to add a test that validation fails with message
'Select one or more comments to perform the update on.'
OTOH the scope of the issue to convert things, not to add new tests
EDIT: There's #1847540: [META] Clean up comment module tests and decouple from node to clean-up comment module and its tests
Comment #94
andypostAdded test and re-rolled after #2151427: Convert COMMENT_NOT_PUBLISHED & COMMENT_PUBLISHED to a constant on the comment interface
Comment #95
tim.plunkettThanks andypost, that's exactly what I meant.
Looks good!
Comment #96
xjm94: comment-admin-1978904-94.patch queued for re-testing.
Comment #98
jibran94: comment-admin-1978904-94.patch queued for re-testing.
Comment #99
andypostStill applies cleanly
Comment #100
webchickCommitted and pushed to 8.x. Thanks!
Comment #101
larowlanYeah!
Comment #102
jibranThank you @webchick for committing this and thank you @andypost, @h3rj4n, @larowlan, @shanethehat, @pcambra and @Xano for the patches :)