#362 | convert_the_comments-1986606-362.patch | 88.57 KB | jibran |
|
#352 | convert_the_comments-1986606-352.patch | 88.57 KB | jibran |
- 8.4.x:
- PHP 5.6 & MySQL 5.5 22,114 pass
- PHP 5.6 & PostgreSQL 9.1 22,104 pass, 3 fail
- PHP 5.6 & SQLite 3.8 22,105 pass, 1 fail
- PHP 7.1 & MySQL 5.5 22,114 pass
- PHP 7 & MySQL 5.5 22,114 pass
- PHP 7 & PostgreSQL 9.1 22,104 pass, 3 fail
- PHP 7 & SQLite 3.14 22,103 pass, 3 fail
- PHP 5.5 & PostgreSQL 9.1 22,104 pass, 1 fail
- PHP 5.5 & SQLite 3.8 22,101 pass, 3 fail
|
#352 | interdiff-b248d8.txt | 5.73 KB | jibran |
#350 | convert_the_comments-1986606-350.patch | 84.88 KB | jibran |
|
#350 | interdiff-42c1fd.txt | 8.47 KB | jibran |
#347 | screenshot-d8.dev-2017-07-12-00-34-00.png | 63.3 KB | jibran |
#347 | screenshot-d8.dev-2017-07-12-00-30-49.png | 62.19 KB | jibran |
#347 | screenshot-d8.dev-2017-07-12-00-23-50.png | 78.54 KB | jibran |
#347 | screenshot-d8.dev-2017-07-12-00-23-05.png | 78.25 KB | jibran |
#347 | screenshot-d8.dev-2017-07-12-00-23-37.png | 56.49 KB | jibran |
#347 | screenshot-d8.dev-2017-07-12-00-22-45.png | 54.78 KB | jibran |
#347 | screenshot-dcd5l.ply_.st-2017-07-12-00-35-13.png | 78.27 KB | jibran |
#347 | screenshot-dcd5l.ply_.st-2017-07-12-00-34-44.png | 82.61 KB | jibran |
#347 | screenshot-dcd5l.ply_.st-2017-07-12-00-22-09.png | 79.49 KB | jibran |
#347 | screenshot-dcd5l.ply_.st-2017-07-12-00-16-02.png | 78.81 KB | jibran |
#347 | screenshot-dcd5l.ply_.st-2017-07-12-00-13-03.png | 76.7 KB | jibran |
#347 | screenshot-dcd5l.ply_.st-2017-07-12-00-12-30.png | 76.55 KB | jibran |
#347 | Screenshot from 2017-07-12 00-38-58.png | 11.82 KB | jibran |
#347 | Screenshot from 2017-07-12 00-38-32.png | 11.79 KB | jibran |
#347 | Screenshot from 2017-07-12 00-13-44.png | 21.2 KB | jibran |
#347 | Screenshot from 2017-07-12 00-13-30.png | 20.22 KB | jibran |
#347 | convert_the_comments-1986606-347.patch | 83.04 KB | jibran |
|
#347 | interdiff-cd1cde.txt | 1.89 KB | jibran |
#346 | convert_the_comments-1986606-346.patch | 82.95 KB | jibran |
|
#346 | interdiff-7ea53d.txt | 2.78 KB | jibran |
#345 | convert_the_comments-1986606-345.patch | 80.58 KB | jibran |
|
#345 | interdiff-ea2d01.txt | 713 bytes | jibran |
#343 | convert_the_comments-1986606-343.patch | 80.58 KB | jibran |
|
#343 | interdiff-b37866.txt | 7.92 KB | jibran |
#340 | convert_the_comments-1986606-340.patch | 80.61 KB | jibran |
|
#340 | interdiff-c780e5.txt | 16.53 KB | jibran |
#335 | convert_the_comments-1986606-335.patch | 79.82 KB | vprocessor |
|
#335 | convert_the_comments-1986606-335.interdiff.txt | 454 bytes | vprocessor |
#332 | convert_the_comments-1986606-332.patch | 79.64 KB | vprocessor |
|
#323 | convert_the_comments-1986606-322.patch | 80.42 KB | jibran |
|
#322 | interdiff.txt | 4.12 KB | jibran |
#319 | convert_the_comments-1986606-319.patch | 78.93 KB | jibran |
|
#319 | interdiff.txt | 1.67 KB | jibran |
#318 | convert_the_comments-1986606-318.patch | 78.71 KB | jibran |
|
#318 | interdiff.txt | 8.26 KB | jibran |
#317 | convert_the_comments-1986606-317.patch | 75.35 KB | jibran |
|
#317 | interdiff.txt | 3.59 KB | jibran |
#315 | convert_the_comments-1986606-315.patch | 73.86 KB | jibran |
|
#315 | interdiff.txt | 10.47 KB | jibran |
#311 | convert_the_comments-1986606-307.patch | 73.1 KB | andypost |
|
#311 | interdiff.txt | 680 bytes | andypost |
#306 | convert_the_comments-1986606-306.patch | 73.1 KB | andypost |
|
#306 | interdiff.txt | 8.26 KB | andypost |
#303 | convert_the_comments-1986606-303.patch | 74.95 KB | andypost |
|
#303 | interdiff.txt | 7.56 KB | andypost |
#301 | convert_the_comments-1986606-300.patch | 74.13 KB | xjm |
|
#300 | convert_the_comments-1986606-300.txt | 74.13 KB | jibran |
#300 | interdiff.txt | 1.33 KB | jibran |
#299 | convert_the_comments-1986606-299-interdiff.txt | 4.05 KB | mbovan |
#299 | convert_the_comments-1986606-299.patch | 74.05 KB | mbovan |
|
#297 | convert_the_comments-1986606-297.patch | 73.59 KB | jibran |
|
#297 | interdiff.txt | 1.14 KB | jibran |
#295 | convert_the_comments-1986606-295.patch | 72.7 KB | jibran |
|
#295 | interdiff.txt | 14.74 KB | jibran |
#292 | convert_the_comments-1986606-292.patch | 79.01 KB | jibran |
|
#292 | interdiff.txt | 3.95 KB | jibran |
#291 | convert_the_comments-1986606-291.patch | 75.06 KB | jibran |
|
#291 | interdiff.txt | 3.95 KB | jibran |
#289 | convert_the_comments-1986606-289-interdiff.txt | 695 bytes | mbovan |
#289 | convert_the_comments-1986606-289.patch | 71.11 KB | mbovan |
|
#287 | convert_the_comments-1986606-287-interdiff.txt | 610 bytes | mbovan |
#287 | convert_the_comments-1986606-287.patch | 71.12 KB | mbovan |
|
#285 | comments-admin-view-1986606-285.interdiff.txt | 2.44 KB | Arla |
#285 | comments-admin-view-1986606-285.patch | 71.07 KB | Arla |
|
#283 | comments-admin-view-1986606-283.patch | 70.95 KB | Arla |
|
#273 | convert_the_comments-1986606-273.patch | 70.53 KB | dobe |
|
#271 | convert_the_comments-1986606-271.patch | 70.58 KB | jibran |
|
#271 | interdiff.txt | 2.02 KB | jibran |
#265 | convert_the_comments-1986606-265.patch | 69.76 KB | jibran |
|
#265 | interdiff.txt | 4.09 KB | jibran |
#264 | convert_the_comments-1986606-264.patch | 69.28 KB | jibran |
|
#264 | interdiff.txt | 786 bytes | jibran |
#259 | convert_the_comments-1986606-259.patch | 69.26 KB | jibran |
|
#259 | interdiff.txt | 4.26 KB | jibran |
#256 | convert_the_comments-1986606-256.patch | 66.73 KB | jibran |
|
#256 | interdiff.txt | 537 bytes | jibran |
#255 | convert_the_comments-1986606-255.patch | 73.35 KB | jibran |
|
#255 | interdiff.txt | 2.46 KB | jibran |
#253 | convert_the_comments-1986606-253.patch | 66.91 KB | jibran |
|
#253 | interdiff.txt | 1.3 KB | jibran |
#249 | convert_the_comments-1986606-247.patch | 66.79 KB | jibran |
|
#249 | interdiff.txt | 612 bytes | jibran |
#247 | provide_a_relationship-2321721-64.patch | 25.66 KB | jibran |
|
#247 | interdiff.txt | 19 KB | jibran |
#246 | convert_the_comments-1986606-246.patch | 66.76 KB | jibran |
|
#246 | interdiff.txt | 4.62 KB | jibran |
#243 | interdiff.txt | 12.22 KB | jibran |
#242 | convert_the_comments-1986606-242.patch | 66.59 KB | jibran |
|
#242 | interdiff.txt | 0 bytes | jibran |
#240 | convert_the_comments-1986606-240.patch | 68.76 KB | jibran |
|
#240 | interdiff.txt | 590 bytes | jibran |
#238 | convert_the_comments-1986606-238.patch | 68.68 KB | jibran |
|
#238 | interdiff.txt | 1.29 KB | jibran |
#238 | interdiff.txt | 5.05 KB | jibran |
#232 | convert_the_comments-1986606-232.patch | 66.41 KB | jibran |
|
#232 | interdiff.txt | 33.96 KB | jibran |
#228 | convert_the_comments-1986606-228.patch | 47.43 KB | jibran |
|
#228 | interdiff.txt | 4.71 KB | jibran |
#220 | convert_the_comments-1986606-220.patch | 49.94 KB | Arla |
|
#220 | convert_the_comments-1986606-220.interdiff.txt | 545 bytes | Arla |
#218 | Screen Shot 2015-05-24 at 19.26.23 .png | 9.39 KB | miro_dietiker |
#218 | Screen Shot 2015-05-24 at 19.25.52 .png | 16.13 KB | miro_dietiker |
#216 | convert_the_comments-1986606-216.patch | 49.94 KB | jibran |
|
#216 | interdiff.txt | 1.62 KB | jibran |
#210 | convert_the_comments-1986606-210.patch | 49.64 KB | jibran |
|
#210 | interdiff.txt | 1.86 KB | jibran |
#208 | convert_the_comments-1986606-208.patch | 49.73 KB | jibran |
|
#208 | interdiff.txt | 1.86 KB | jibran |
#206 | Screenshot 2015-05-16 17.06.53.png | 208.26 KB | larowlan |
#204 | screenshot-d8.dev 2015-05-16 08-09-47.png | 66.35 KB | jibran |
#204 | screenshot-d8.dev 2015-05-16 08-08-54.png | 65.29 KB | jibran |
#204 | screenshot-d8.dev 2015-05-16 08-10-34.png | 48.17 KB | jibran |
#204 | screenshot-d8.dev 2015-05-16 08-11-25.png | 60.55 KB | jibran |
#204 | convert_the_comments-1986606-204.patch | 48.92 KB | jibran |
|
#204 | interdiff.txt | 8.64 KB | jibran |
#201 | convert_the_comments-1986606-201.patch | 42.74 KB | jibran |
|
#201 | interdiff.txt | 15.18 KB | jibran |
#199 | convert_the_comments-1986606-199.patch | 34.88 KB | jibran |
|
#199 | interdiff.txt | 1.96 KB | jibran |
#195 | interdiff.txt | 27.63 KB | kim.pepper |
#195 | 1986606-comment-view-195.patch | 34.47 KB | kim.pepper |
|
#192 | convert_the_comments-1986606-192.patch | 34.19 KB | jibran |
|
#188 | 1986606-188.patch | 35.27 KB | jibran |
|
#188 | interdiff.txt | 777 bytes | jibran |
#187 | 1986606-187.patch | 35.39 KB | jibran |
|
#187 | interdiff.txt | 788 bytes | jibran |
#185 | 1986606-185.patch | 35.01 KB | jibran |
|
#177 | 1986606-comment-admin-view-177.patch | 35.12 KB | pcambra |
|
#177 | interdiff.txt | 3.46 KB | pcambra |
#174 | Screenshot 2014-12-30 08.15.58.png | 29.79 KB | larowlan |
#173 | Screenshot 2014-12-30 08.12.59.png | 38.53 KB | larowlan |
#173 | Screenshot 2014-12-30 08.11.58.png | 52.7 KB | larowlan |
#172 | Screenshot 2014-12-30 07.38.43.png | 47.68 KB | larowlan |
#169 | 1986606-comment-admin-view-169.patch | 34.84 KB | pcambra |
|
#169 | interdiff.txt | 5.01 KB | pcambra |
#166 | 1986606-comment-admin-view-166.patch | 36.17 KB | pcambra |
|
#166 | interdiff.txt | 5.5 KB | pcambra |
#158 | 1986606-comment-admin-view-158.patch | 38.86 KB | pcambra |
|
#158 | interdiff.txt | 2.53 KB | pcambra |
#156 | 1986606-comment-admin-view-156.patch | 38.65 KB | pcambra |
|
#156 | interdiff.txt | 2.24 KB | pcambra |
#154 | 1986606-comment-admin-view-154.patch | 38.7 KB | pcambra |
|
#154 | interdiff.txt | 31.01 KB | pcambra |
#151 | 1986606-comment-admin-view-151.patch | 61.61 KB | pcambra |
|
#151 | interdiff.txt | 15.77 KB | pcambra |
#149 | 1986606-comment-admin-view-149.patch | 63.19 KB | pcambra |
|
#146 | 1986606-diff-145-146.txt | 538 bytes | vijaycs85 |
#146 | 1986606-comment-admin-view-146.patch | 63.18 KB | vijaycs85 |
|
#145 | 1986606-diff-142-145.txt | 1.74 KB | vijaycs85 |
#145 | 1986606-comment-admin-view-145.patch | 63.71 KB | vijaycs85 |
|
#142 | 1986606-diff-140-142.txt | 1.56 KB | vijaycs85 |
#142 | 1986606-comment-admin-veiew-142.patch | 65.17 KB | vijaycs85 |
|
#140 | 1986606-comment-admin-veiew-140.patch | 65.16 KB | vijaycs85 |
|
#129 | interdiff.txt | 5.64 KB | dawehner |
#129 | vdc-1986606-129.patch | 67.24 KB | dawehner |
|
#128 | before_approved.png | 72.18 KB | xjm |
| after_approved.png | 56.42 KB | xjm |
#125 | after_unapproved.png | 49.35 KB | xjm |
#125 | after_approved.png | 56.42 KB | xjm |
#125 | before_unapproved.png | 318.43 KB | xjm |
#125 | before_approved.png | 328.55 KB | xjm |
#118 | interdiff.txt | 502 bytes | dawehner |
#118 | vdc-1986606-118.patch | 66.59 KB | dawehner |
|
#116 | interdiff.txt | 1.97 KB | dawehner |
#116 | vdc-1986606-116.patch | 66.59 KB | dawehner |
|
#113 | interdiff.txt | 15.69 KB | dawehner |
#113 | vdc-1986606-113.patch | 66.67 KB | dawehner |
|
#111 | 1986606-comment_admin_view-109.patch | 98.9 KB | tkuldeep17 |
|
#109 | 1986606-comment_admin_view-109.patch | 98.71 KB | tkuldeep17 |
|
#103 | 1986606-diff-100-103.txt | 1.46 KB | vijaycs85 |
#103 | 1986606-comment_admin_view-103.patch | 61.18 KB | vijaycs85 |
|
#100 | 1986606-diff-97-100.txt | 6.71 KB | vijaycs85 |
#100 | 1986606-taxonomy-views-100.patch | 61.25 KB | vijaycs85 |
|
#97 | comment-admin-screen.png | 84.69 KB | vijaycs85 |
#97 | 1986606-diff-80-97.txt | 1.31 KB | vijaycs85 |
#97 | 1986606-taxonomy-views-97.patch | 58.71 KB | vijaycs85 |
|
#85 | 80-1986606.patch | 1.34 MB | adnen |
|
#84 | 80-1986606.patch | 1.34 MB | adnen |
|
#84 | 80-1986606.patch | 1.34 MB | adnen |
|
#79 | comments_administration_view-1986606-79.patch | 58.88 KB | snig |
|
#76 | interdiff.txt | 751 bytes | dawehner |
#76 | vdc-1986606.patch | 59.09 KB | dawehner |
|
#73 | interdiff.txt | 2.59 KB | jibran |
#73 | d8-comment-view-1986606-73.patch | 58.35 KB | jibran |
|
#71 | interdiff.txt | 15.01 KB | jibran |
#71 | d8-comment-view-1986606-71.patch | 55.76 KB | jibran |
|
#62 | d8-comment-view-1986606-61.patch | 41.97 KB | jibran |
|
#45 | 1986606-comments_admin_views-45.patch | 91.11 KB | pcambra |
|
#45 | interdiff.txt | 41.57 KB | pcambra |
#42 | interdiff.txt | 7.73 KB | pcambra |
#42 | 1986606-comments_admin_views-42.patch | 85.66 KB | pcambra |
|
#38 | interdiff.txt | 3.3 KB | pcambra |
#38 | 1986606-comments_admin_views-37.patch | 85.38 KB | pcambra |
|
#34 | 1986606-comments_admin_views-34.patch | 86.12 KB | pcambra |
|
#33 | Comments drupal8.local 2013-06-29 10-16-46.png | 56.13 KB | pcambra |
#32 | Comments drupal8.local 2013-06-29 10-10-27.png | 126.3 KB | pcambra |
#29 | comments-before.png | 83.57 KB | kim.pepper |
#29 | comments-after.png | 77.39 KB | kim.pepper |
#26 | 1986606-comments_admin_views-26.patch | 85.67 KB | pcambra |
|
#24 | interdiff.txt | 20.04 KB | pcambra |
#24 | 1986606-comments_admin_views-24.patch | 85.77 KB | pcambra |
|
#21 | interdiff.txt | 32.14 KB | pcambra |
#21 | 1986606-comments_admin_views-21.patch | 66.71 KB | pcambra |
|
#20 | interdiff.txt | 40.22 KB | pcambra |
#20 | 1986606-comments_admin_views-20.patch | 67.93 KB | pcambra |
|
#19 | interdiff.txt | 10.42 KB | pcambra |
#19 | 1986606-comments_admin_views-19.patch | 57.44 KB | pcambra |
|
#12 | interdiff.txt | 934 bytes | pcambra |
#12 | 1986606-comments_admin_views-12.patch | 53.2 KB | pcambra |
|
#11 | 1986606-comments_admin_views-11.patch | 53.2 KB | pcambra |
|
#11 | interdiff.txt | 5.3 KB | pcambra |
#9 | 1986606-comments_admin_views-9.patch | 48.99 KB | pcambra |
|
#9 | interdiff.txt | 48.77 KB | pcambra |
#5 | 1986606-comments_admin_views-5.patch | 31.48 KB | pcambra |
|
#5 | interdiff.txt | 2.26 KB | pcambra |
#3 | 1986606-comments_admin_views-3.patch | 31.38 KB | pcambra |
|
#3 | interdiff.txt | 17.61 KB | pcambra |
#2 | 1986606-comments_admin_views-2.patch | 28.9 KB | pcambra |
|
Comments
Comment #1
pcambraComment #2
pcambraHere's a very initial take, listing has been exported to views and added some bulk operations.
TODO's
Comment #3
pcambraHere's a new version of the patch, added a delete action and tests for it.
Also discussed with dawehner, we're delaying adding the number of comments to the "Unapproved comments" menu item as it could be a performance risk and actually there's no "right" way to do it apparently.
Comment #5
pcambraThe tab menus are not working good enough, I'd say that there's a conflict with the parent menu so the tabs are displayed but the parent is always the original screen, we could embed the view in case views is enabled but there are also other options such as replacing the secondary tabs by a filter.
Also, actions module is not a views dependency, resulting into a Broken/Missing handler in the view by default which I don't think is a good thing to display, we might want to add a custom form just the way as admin/content and admin/people are doing for other reasons.
Comment #7
tim.plunkett#1846172: Replace the actions API will change how the actions stuff works.
The tab issues are caused by #2011006: Default local tasks provided by Views are broken
Comment #8
pcambraResuming work in this
Comment #9
pcambraHere's the view, I guess that we can work in the entity list controller in #1978904: Convert comment_admin() to a Controller and deal with the number of unread comments in a follow up.
Comment #11
pcambraComment #12
pcambraA little leftover regarding excluded fields
Comment #15
pcambraI guess we need better testing support for the comment admin view, sort of NodeAdminTest, but not sure what makes sense to test here.
Comment #16
damiankloip CreditAttribution: damiankloip commentedThis should use the area_text_custom ('Unfiltered text') handler instead. Otherwise we are tied to a format, and if that is not available nothing will be displayed at all.
At the moment I thought we were still removing UUIDs but not sure if that patch/issue to add UUIDs to default config got resolved yet?
public function, while we're there :)
Can we get a docblock for these (and public)? again, while we're there...
public
Maybe we should be trying to use the storage controller instead? Would be good to get a definitive answer on this, because I'm not actually sure myself.
@return needed
Comment #17
pcambraFrom a conversation with @damiankloip the test we might need would be in the lines of FrontPageTest:
"We shouldn;t need to test too much as view tests should cover a lot of these things"
Comment #18
dawehnerSome additional feedback.
I don't think that this is longer true in Drupal 8 world and at least we should not assume it.
Then let's typehint this with CommentInterface
Let's better do Unicode::strtolower
Comment #19
pcambraThanks @dawehner & @damiankloip
Done.
As for discussed on IRC, uuids should be there.
Fixed those as well :). Also "typehinted" with CommentInterface
Replaced, let's see how it looks.
Grabbed that from the content one, is just the comment what is wrong or it's the actual thing?
We've got a test for the action.
Attaching a new patch covering the above, still need to create a new test.
Comment #20
pcambraAdded tests and fixed the optional/broken links
Comment #21
pcambraRemoved the views_entity_comment references from the data hooks and added a test for the empty text
Comment #22
pcambraAfter discussing with @dawehner, this and #1978904: Convert comment_admin() to a Controller needs to happen at the same time, so I'll include the fallback conversion of comment_admin() to a controller in this patch.
Comment #23
pcambraRelated: #2029453: Move views node links back to node table
Comment #24
pcambraAdded support for multiple comment deletion using tempstorage as nodes do and menu page callback cleanup.
It's not converted to wscci yet, but that could go to a followup just as the node is.
Comment #26
pcambraIt had a conflict with the comment approve issue, here's a re-roll to HEAD
Comment #28
dawehnerIt would be great to have some screenshots of before and after in the summary.
This change seems out of scope, especially because the actual code did not changed. Let's try to have the smallest change as possible.
So if we are build up a fallback we should do it properly and use EQ, and maybe even a fallback
Comment #29
kim.pepperBefore:
After:
Comment #30
klonosSo, we're still missing the bulk operations part.
Comment #31
tim.plunkettSee core/modules/node/lib/Drupal/node/Plugin/views/field/NodeBulkForm.php and core/modules/node/node.views.inc line 230 for how comment should go about implementing a bulk form handler
Comment #32
pcambraThis is with views module enabled, implementing the actions, it's the fallback what doesn't have the operations anymore.
Comment #33
pcambraOperations are there too, had a cache issue.
Summing up, with views and action modules enabled, we get the bulk operations in the comment screen which is a view. Without views enabled, we get a fallback that doesn't show the bulk operations, just lists comments with status.
Comment #34
pcambraHere's a fix for the actions test that checks tempstore instead of deletion, also converted the query into an entity query as for #28, but then we're losing the ability to order by the node posted in as it's in a different table. Is there any way to overcome this or is ok to drop that for the fallback?
Regarding the views + action dependency, I think it's fine to require action module for having the VBO feature enabled in the view, otherwise, we'd need to provide a form and that one won't be configurable with the actions desired, I'm not sure why the content listing took this path.
This patch will still fail in the operations part. Let's see what we agree about the bulk operations before fixing that one.
Comment #36
dawehnerIt is just amazing how big views config is.
Let's skip this part of the patch.
As people can replace the access controller for comments, having an access to a link has to be checked via $comment->access all the time, so we should skip the basic access() method.
Is this really needed after drupalLogin?
Let's just make an empty line here.
Comment #37
R.Hendel CreditAttribution: R.Hendel commentedLast patch #34 is no longer compatible with current 8.x
(warning: "Reversed (or previously applied) patch detected!")
Comment #38
pcambraAddressing @dawehner comments in #36
Yes, as we have the published/unpublished thing, we need to keep several displays, we could agree in dropping the local subtabs in favor of an exposed filter for published / unpublished.
Ok
Dropped access method in all Link* views field plugins.
Yes, as the action is executed with the user 2 (admin_user) but the tempstore is retrieved from uid 1 so that's the only way I had to access the right tempstore, it's not super nice but it's used normally in tests.
Done
As #37 mentions, re-rolled the patch against latest head.
Back to CNR, we still need to agree if action module dependency and fallback.
Comment #40
pcambraOpened this followup I found while doing this #2031599: Add lock set/delete tempstore tests
Comment #41
tim.plunkettAction plugins are not dependent on the action module.
The action_bulk_form plugin used here is a generic handler, but node and user each subclass \Drupal\system\Plugin\views\field\BulkFormBase, and comment should too.
Look at UserBulkForm and NodeBulkForm; they're relatively simple classes.
Comment #42
pcambraRebased to latest head and also added a CommentBulkForm as for #41
Comment #43
tim.plunkettThis looks very brittle.
Comment #45
pcambraSo here's another round, the tests will still fail on the anonymous and maybe approval as I've found that any of the actions are displaying any confirmation message (i.e. "Deleted 1 comment"), but neither node actions do so I don't know if we should include them here or not.
This includes a reroll as many things have happened since #42.
As for an IRC conversation with @tim.plunkett and @dawehner, I've replaced the code in #43 by a options settings form for excluding/including the operations (i.e. not show the unpublish action for unpublished comments) but at this point the code is identical to the one in Drupal\action\Plugin\views\field\BulkForm, it might be good to move that one out of action to views as the action plugins are available even if the action module is not.
An alternative would be to make available the action form in views regardless the status of the action module.
Also fixed some updating issues related to #2011018: Reconcile entity forms and confirm forms.
Comment #47
pcambraRelated: #2049573: Move most or all of the ActionBulkForm class to BulkFormBase
Comment #48
xjmSo, I am 300% in favor of converting this to a view, but I'm not sure we can get away with closing #1978904: Convert comment_admin() to a Controller as a duplicate, at least not yet. That, as part of the routing meta, is release-blocking; and Dries has stated several times that View conversions do not block release.
Maybe we can reopen the other at postponed, under the hope that this patch lands in time?
Also, this and #1907960: Helper issue for "Comment field" are going to seriously punch each other in the face. :) Might be worth considering waiting until that one is in (for either this or a rote form conversion), since that (or rather the main issue, sorry) is currently marked critical.
Comment #49
pcambraSetting status as for ##4
Comment #50
andypostI think it makes no sense to mark duplicate #1978904: Convert comment_admin() to a Controller
It much easy to convert and fallback is needed anyway
Comment #51
andypostThis issue also postponed on #2027115: Allow views to override existing routing items
Moved back to work #1978904-13: Convert comment_admin() to a Controller
Comment #52
andypostAlso related #2111357: Get rid of comment_count_unpublished() in favor of CommentStorage method
Comment #53
pcambraI think this is not postponed anymore
Comment #54
dawehnerLet's better wait until https://drupal.org/comment/7894541#comment-7894541 is in, given that it conflicts in a lot of places with this patch.
Views conversions are soooo novice issues!
Comment #55
pcambraComment #56
andypostStill postponed on RTBC'ed #1978904: Convert comment_admin() to a Controller
Comment #57
andypostComment #58
daffie CreditAttribution: daffie commentedComment #61
jibranReroll
Caution: Delete multiple action is not working. For this we should add comment list controller. View empty area is also not rendered properly I think we already have an issue for that.
Comment #62
jibranNow with patch
Comment #66
andypostuse 2 spaces for indent
seems uuid still needed for shipped config
Comment #67
andypostseems we really need list controller for fallback
Comment #68
tim.plunkettNope. The only fallbacks needed are node and user, as decided by Dries. If they want a list, use a view.
Comment #69
jibranCurrently we don't have a
comment.delete_multipe
route in core which is required by comment delete action. Deletion of multiple comments are done usingcomment.admin
route.I am purposing here that if we have to add
comment.delete_multipe
route to fix comment delete action then why not follow the same pattern as node list controller because comment list controller is almost similar to node list controller unlike taxonomy terms listing page.Comment #70
jibranJust had a discussion with @tim.plunkett. We have to add
comment.delete_multiple
route to complete this patch and we can move the discussion of replacing/movingcomment.admin_overview
route to list controller to the follow up issue because it is related to comment module not views conversion.Comment #71
jibranFixed #66. Comment delete is working now. Re-factor some code for delete multiple.
Fun Fact: Can someone translate uuid in comment delete action?
Comment #73
jibranFixed the comment tests. But I don't know how to fix
DefaultViewsTest
. It ready for review.Comment #75
klonos#2130059: Issue status change not consistently saved in node revisions
Comment #76
dawehnerThank you for working on that issue! The bulk form would profit from #2049573: Move most or all of the ActionBulkForm class to BulkFormBase though this is certainly no blocker
I had a look at the test and realized that it is 100% bullshit. We cannot just test all default views and expect that there is never any result ... I removed the part of the test which does not make sense
though kept the bits which do: checking which display is actually executed.
Comment #77
olli CreditAttribution: olli commentedShould we use the "Show the empty text in the table" with this one too? That is enabled on people, content and files.
Re #76 We could just create few unpublished comments here and remove that part (or test only views' views) in a separate issue?
Comment #78
jibranNeeds reroll after #2049573: Move most or all of the ActionBulkForm class to BulkFormBase
Comment #79
snig CreditAttribution: snig commentedre-rolled
Comment #81
dawehnerThis issue is a clear sign that we don't have 100% schema coverage.
Comment #82
willieseabrook CreditAttribution: willieseabrook commentedWill tackle this in Sprint 29 March.
Comment #83
adnen CreditAttribution: adnen commentedComment #84
adnen CreditAttribution: adnen commentedComment #85
adnen CreditAttribution: adnen commentedComment #93
tim.plunkettYou've included what looks to be your entires /sites directory. Please don't do that :)
Comment #94
dawehner@adnen
Good to see that someone continues to work on that!
I have to dissapoint you, but your fill sites directory is still smaller as the original VDC patch :P #1805996: [META] Views in Drupal Core
Comment #95
YesCT CreditAttribution: YesCT commented:) https://drupal.org/contributor-tasks/create-patch has links to docs with examples and git information on creating patches that might help. And also ask anything in #drupal-contribute (https://drupal.org/irc).
Comment #96
xjmAlso see the git configuration instructions for how to ignore the files dir when creating patches.
Comment #97
vijaycs85Updated patch has:
1. Removed files dir and install specific changes.
2. Fixed an argument issue in controller
did a manual testing and all looks function at /admin/content/comment.
Comment #98
jibranThanks for working on this. Here is some minor feedback
I think we have to move this in config/install folder.
we can remove this. and the other uuid in
views.view.comment.yml
Can we change $object to $entity?
form_set_error is @deprecated.
Comment #100
vijaycs85Thanks for the quick review @jibran. Fixed all 4 review items in #98
Comment #101
xjmComment #103
vijaycs85This would fix few failures.
Comment #104
YesCT CreditAttribution: YesCT commentedComment #105
vijaycs85@YesCT, added screenshot in #97
Comment #107
xjmComment #108
YesCT CreditAttribution: YesCT commented@vijaycs85 ah, please update the issue summary. there should be a section for UI changes. And in that section should be a before and after screenshot. :)
Comment #109
tkuldeep17 CreditAttribution: tkuldeep17 commentedComment #111
tkuldeep17 CreditAttribution: tkuldeep17 commentedAfter updating latest code, patch is submitting..
Comment #113
dawehnerEither #109 and #111 seemed to be a completly mixed up reroll as it changes views.content.yml as well. Let me help you, this one installs without a problem.
All the fixes are required for config schema stuff.
Comment #114
jibranThis can be removed now.
Comment #116
dawehner@jibran
Good catch!
Comment #117
vijaycs85Might be my mistake setting it as views_field. but it might not be a string. By the key name 'text', it sounds it is translatable. Can we make it 'label', if it is textfield, or 'text' if it is textarea?
Comment #118
dawehnerYeah label seems to make much more sense.
Comment #119
vijaycs85Comment #120
YesCT CreditAttribution: YesCT commentedbefore and after screenshots of this admin page are a novice task. tagging.
Comment #121
dawehnerYeah let's apply rules everywhere instead of just open our eyes.
Comment #122
YesCT CreditAttribution: YesCT commentedSo is it still true, per earlier comments, that action module needs to be enabled to get the same bulk operations as exists before?
And it looks like the count of unapproved comments isn't there in the new one.
Comment #123
xjmI'll supply the screenshots (edit: there are some in the summary) and do some manual testing. Thanks @YesCT and @dawehner.
Comment #124
tim.plunkettThe action entity is in the system module. See #2083649: Rename action module to action_ui for resolving that confusion.
Comment #125
xjmSo, the earlier screenshots are no longer accurate. I tested the latest patch manually. The action checkboxes are available, as is the count of unapproved comments in the menu tab title.
Before
After
The one regression from HEAD is that -- because of the content relationship in the view -- we can no longer list comments on all entity types. Notice that there is no comment on my custom block entity listed in the after screenshots. This is not a feature of D7, so maybe it's okay for it to only be a view of node comments; on the other hand, moderation seems to be an intrinsic feature of comments regardless of what entity they're on. I could go either way. We could also spin that off into a followup issue.
Interestingly, simply unchecking "require this relationship" does not seem to be enough to get the comments on other entity types to show up in the view; I have to actually also remove the title field that relies on the relationship before the other comment is displayed. That feels buggy in a way that's not in scope for this issue... here's the queries.
With the content title field and "require this relationship" unchecked:
Same thing, but without the content title field:
Setting NR to decide if the loss of other entity types' comments in the listing is an acceptable regression or not, or if it could be a followup. The texts for the actions are also different: "Publish comment" versus "Publish the selected comments". We might want a quick UX check on that. (Note that admin/content itself has it both ways: "Delete selected content" but "Publish content". (Note that these labels come from the actions themselves, not the view.)
Comment #128
xjmSorry, total screenshot embed fail.
Comment #129
dawehnerSeriously good catch xjm, now we just have a views architecture problem.
As you have seen, the INNER kills it all...
Rerolled the patch.
Comment #130
xjmAgreed with just dropping the INNER for
{node_field_data}
. Edit: what I said first made no sense, but in any case, I filed #2273849: Convert INNER joins to LEFT joins for relationships without "Require this relationship" for fixing the bug generally.Comment #132
xjmComment #133
xjmComment #134
xjmComment #135
xjm@berdir in #2205215: {comment} and {comment_entity_statistics} only support integer entity ids regarding this issue:
Comment #136
dawehner.
@xjm
Do you still want to be assigned to this issue?
Comment #137
xjmOops nope, that was just to add screenshots and test. :)
Comment #138
hampercm CreditAttribution: hampercm commentedAttempting to reroll patch from #129
Comment #139
hampercm CreditAttribution: hampercm commentedSorry, I think this reroll is beyond my current abilities. Turned out to be much more involved than I had realized.
Comment #140
vijaycs85Reroll of #129
Comment #142
vijaycs85Fixing Drupal\Core\Form\FormErrorInterface => Drupal\Core\Form\FormBuilderInterface.
Comment #143
tim.plunkettShouldn't need the form builder at all anymore, that's a method on $form_state
Comment #145
vijaycs85Thanks @tim.plunkett. Updated.
Comment #146
vijaycs85Minor update...
Comment #148
larowlanComment #149
pcambraJust a plain reroll to take it from there (just one .rej file, comment.routing.yml)
Comment #151
pcambraFirst round, updated view fields, comment bulk form field and the ConfirmDeleteMultiple.php class. There should be less failures in this one (hopefully)
Comment #153
diego21 CreditAttribution: diego21 commentedI could apply the #151 patch, so no reroll is needed.
Comment #154
pcambraHere's another take, let's hope this one is better.
I've updated the comment view to do some cleanup and to remove the translation link from the dropbutton, content view doesn't have it and I think is not working properly, but I need to look that further (probably another issue, we can add it later, I guess)
Comment #155
dawehnerThank you for keeping up with this issue!
Tested the patch manually.
First thing I noticed is that we don't have added any filter ... I kinda think, given that this is some sort of advantage of views, we should leverage it here.
Maybe open up a followup?
I kinda believe that this belongs into the
CommentViewsData
...Certainly a right fix!
You should be able to use $this->currentUser() here.
This could be replaced with
$this->redirect('comment.admin');
Afaik this has to be replaced with
confirm_form_route_name
What is up with this change?
Comment #156
pcambraThanks for the review Daniel!
Regarding the filters, the one straightforward is the published/non published and we've got those as tabs, maybe the bundle of the parent entity?
Both user and nodes are in the _data_alter hook, maybe we can have a followup to move all of those?
Edit: both user and node bulk form view data additions happen in the data_alter, we're following the pattern atm.
Done.
Great catch! fixed.
This comes from #1986606-76: Convert the comments administration screen to a view it seems :))
Comment #157
dawehnerThat sounds like a wonderful idea!
Well ... there we alter other tables, not comment tables, your hunk added stuff to 'comments'.
Ha ... maybe you could try to figure out whether we can remove that hunk from the patch again?
Comment #158
pcambraMoved the bulk form to CommentViewsData
But I think we should add a followup then to move all alike, such as the node in node.views.inc:
Also added some sample data, the test removed wasn't working because there were no unapproved comments to list.
Regarding adding the filter on Node type, we've bumped into #2273731: Filters throw unrecoverable error when using relationships and/or #2322457: Error in Views Filter Nodes based on Current user that have term reference same in nodes. which mark relationships as "complicated" in views atm. Can we do this in a followup?
Comment #159
dawehnerSure!
Can we add a beta evaluation, please?
Comment #160
pcambraComment #161
pcambraAdded, feel free to improve the reasons. Not sure if anything else is required here :).
Comment #162
pcambraOoops removed one tag too many
Comment #163
dawehner+1
Comment #164
larowlanWe're limiting this to only node-based comments with a) this view and b) any follow-up. Comments can be attached to any entity type in HEAD.
Is this an unintended hunk? these were removed some time ago - in particulare getFieldUIPageTitle no longer exists on comment manager
nitpick > 80
$this->currentUser()?
We're implementing ContainerFactoryPluginInterface so we can inject this service right?
Manual review coming
Comment #165
larowlanComment #166
pcambraThanks for the review @larowlan
Fixed.
Nice one. It would appear this is a leftover, I've removed those bits, thanks!
I disagree with this one, bear in mind that we're providing the default comment listings that are placed in admin/content/comments, so the same way that you wouldn't expect not-node entities listed in admin/content, the comments listing in the content section should be bound to nodes IMHO.
Otherwise you'll start to see comments there attached to entities that are not listed in admin/content, which I think is unexpected. This change if only, allows the user to change that if desired, thing that the current comment listing doesn't.
Comment #167
jibranThanks @pcambra for making it green again. Some minor points.
These properties are not defined on the class.
Can we inject current user here?
So we are overriding $comment @var. Does it make sense?
Comment #168
jibranWell currently in head we are showing all commented entities removing that here would be a regression. So I'd suggest to leave the current scenario as is and discuss this point in a follow up.
Comment #169
pcambraFair enough. I'll open a batch of followups when we're RTBC again, I think it makes more sense to list just the node comments under an admin/content/comments route.
They are there (they were before, I'm just removing the unused ones here):
Not sure, DeleteNode.php does the same thing:
It doesn't really matter, but I've renamed the variable to avoid misunderstandings.
Comment #170
jibranThank you for the fixes. This is RTBC for me let's wait for @larowlan manual review :-)
Comment #171
larowlan'Can we inject current user here? '
Yes ContainerFactoryPluginInterface
Comment #172
larowlanManual screenshot from HEAD - two comment fields - one on users.
Comment #173
larowlanWith patch
So what we're missing against HEAD (i.e. regressions) is
1) Can't perform bulk unpublish (here is screenshot from HEAD)
2) Missing the 'posted on' link/column
Also - I would expect to see some dead code removed in the patch - but there is no such deletion?
Did we decide earlier (172 comments) that 1 or 2 are acceptable in light of the 'less custom code' approach?
Comment #174
larowlanAlso page title on the view is 'Comment' but in HEAD it is 'Comments'
Comment #175
jibranWe are also missing "posted in" field.
Comment #176
larowlanSorry, said 'posted on', meant 'posted in'
Comment #177
pcambraThank you for the manual tests @larowlan!
Let's see if I didn't miss anything.
Added this, I guess we'll need to open a followup to make the rest of places where this happens (node, user...) use the same pattern.
Fixed this.
Tried to add a comment field on the user entity but getting all sort of errors in the way in latest HEAD. Generating comments for nodes displays two actions: one to delete and one to unpublish in the published comments tab which becomes publish in the unpublished comments tab.Edit, this was due to the problem fixed in #2380071: No way to add comment field to any entity. Added a comment field to users and taxonomy terms and I see both delete/publish operations for comments on not-node entities, not sure what might be happening.
Yes. If we deattach the comment view from the node we'd need a custom handler to display the "posted in" field. It's a matter of whether we display comments only for nodes or for all entities in admin/content/comments. If we restrict to nodes, there's no need of the extra handler but then I guess we need to fix the "fallback controller" accordingly.
What 'dead code' needs to be removed that it isn't?
Comment #178
larowlanI was expecting a lot of CommentAdminOverview would go, if not all of it - I'm not seeing that in the patch - could be I'm mistaken - but I thought the view would replace that - or does it remain and views enhances it/the listing?
How much work is involved in that - and would it be performant? In HEAD we collect the entity IDs into entity type groups and then use a single call to entity_load_multiple per entity-type - is that kind of thing possible with views?
Might be a question for @dawehner and @jibran from the VDC team.
Comment #179
dawehnerMuch like the other conversions (admin/content, admin/people) we realized that some sort of admin listing is a central functionality which is part of the corresponding module. We can't just drop the basic functionality. At the same time, we should add the flexibility.
Absolutely, you could write a handler which does that. We have a method (
::preRender()
) which retrieves all results so you can act based upon it.Comment #180
andypostThe related issue should postpone this. OTOH we have no consistent method to determine is an entity use "changed" field, because
EntityChangedInterface
does not tell about a field for that.Also statistics could be disabled so no idea how to sort comments properly
Comment #181
pcambraA couple of small followups:
#2400159: Use the container for current user on all ActionBase plugins
#2400153: Move bulk form data definition to their EntityViewsData
Comment #182
dawehnerWell, I would argue that the patch for itself is some sort of progress, so we don't have to wait onto another one, maybe people disagree though.
Comment #185
jibranRe-rolled
Comment #187
jibranok
Comment #188
jibranminor clean up.
Comment #189
andypostlast interdiff shows that there's no test coverage for access
also I wondered a way to pass comments to confirm submit via tempstore that affected by race condition like #2421263: Potential data loss: concurrent (i.e. by different users) node edits leak through preview, suppose better to pass ids via form_state somehow
Comment #192
jibranReroll.
Comment #193
kim.pepperKicking the bot.
Comment #195
kim.pepperFixes install fatals. Not sure why they weren't happening in #192 ?
Comment #197
jibranI'm going to work on fails and #179.2.
Comment #198
jibranI'm going to work on fails and #179.2.
Comment #199
jibranI hope this will fix the fails.
Comment #201
jibranI have wasted so much time on these config fails I should have created a new view :P
The only remaining thing is "posted on" col. Still working on this.
Comment #202
jibran@larowlan & @andypost is it intentional that we don't have a comment list builder handler?
Comment #204
jibranAdded new screenshots also added posted in column.
Before Empty
After Empty
Before With Comment
After With Comment
Comment #205
jibranThe Query
Comment #206
larowlanI think we need to check access here. If $entity->access('view') then show the label, otherwise show Redacted or similar.
Other than that, can't fault it.
Here's a screenshot, on the top you can see admin has unpublished the node.
On the bottom is a user with 'administer comments', 'access admin theme' and 'access admin pages' but not 'administer nodes' or 'view unpublished nodes'.
They shouldn't see the node title in the comment admin screen.
Comment #207
larowlanSorry, given this passed but has access bypass - means we need tests :(
Comment #208
jibranTests are showing me some different story.
Comment #209
dawehnerDo you think its worth to try to merge those calls, so they aren't duplicated?
All that doesn't make sense! We should be able to just use the ordinary string formatter, which already has a link to entity option ... don't we?
Comment #210
jibranFixed that. We can further simplify
to
If the operation is not delete it'll either be publish or unpublish but contib can always add more operations so let's not make this FormSubmit a nightmare for contrib by oversimplifying things :)
CommentedEntityLabel
is a direct copy ofEntityLabel
without thequery()
and simplifiedrender()
&preRender()
. I did0 extend CommentedEntityLabel form EntityLabel first but using FieldPluginBase is much simpler an more cleaner.Comment #211
dawehnerBut well, do we really need it?
Comment #212
jibranHow else we can show posted in column? This is what you suggested in #179.2
Comment #214
larowlanTest looks good, can't find anything else to pick at.
Thanks @jibran, @pcambra, @dawehner and @vijaycs85 for the hard slog on this.
Comment #215
miro_dietikerSorry for nitpicking...
edit and delete are both capitalised in Content overview: Edit and Delete.
For other entity overviews we have introduced hook_entity_operations_alter(). Now it is still not supported. Does that mean that all lists that are switched to a view only allow alterations of the operations by changing the view configuration?
Comment #216
jibranFixed #215. Thank you for the nice catch.
Comment #217
miro_dietikerThx for fixing. Looking good indeed.
I'm just a little confused there is no test coverage at all depending on "Delete" and "Edit" link on the overview..
Then i guess, the related issue about comment hook_entity_operations_alter() is rejected?
Comment #218
miro_dietikerIn this comment issue, each operation is added in configuration, hidden, with a Dropbutton. Thus hook_entity_operations_alter() does not work.
In the content overview, entity operations are added through one dedicated operations field.
Fine, if we look into this as a followup, but there need to be consistency in how we maintain operations...
Comment #219
jibranHave you seen #202? This problem is not introduced here. It is inherited from D7/D6 and we never corrected it. This issue always came up. We don't have a sperate link for deleting the comment like node has
node/nid/delete
there is nothing for the comment like this when we converted comment paths to route this question came up but that was consider out of scope because priority was route conversion at that time. When we introduced list builder for entities we ignore comments because we have to isolate between admin page and delete page which we never did. Operations links can only exist if we have a list builder and for comments and that is why we have to use dropbutton here. See #2491875-14: EntityViewsData adds Operations links to all entities, which won't work if the entity type has no list builder, leading to WSOD on some views as well. Thank you for adding the related issue.Comment #220
ArlaCorrected "Opertaions" typo. Keeping the RTBC status because this is so trivial. No need for commit attribution.
Comment #223
BerdirHm. Not sure I buy the argument about those operation links :)
* entity.comment.delete_form / comment/{comment}/delete exists, not sure why you think it doesn't? How else would it work? The normal delete link also points to the same?
* That comment currently doesn't have a list builder doesn't mean it shouldn't have one. You can just add the default, it won't be accessible directly in any way unless you have link template/route for it. I've been doing that in quite a few contrib projects.
* Pretty sure the primary reason this is using link fields + dropbutton is because this patch was started long before the other admin views were changed to use the new operations field. Not using it seem seems inconsistent?
Unlike other things, "getting a basic version in and then improve on it" doesn't work so well for views/default configuration because updating it requires update functions or you're stuck with existing sites on an old version of the view. So it might be now or never for 8.x ;)
Comment #224
damiankloip CreditAttribution: damiankloip commentedBerdir is totally correct. When this issue started we only had the dropbutton handler that would reference other link based handlers. Once we added the operations handler, we should be using that. The other admin views do. This gives us consistency through out the operations for an entity.
As well as other modules being able to add their own, like devel, for example. If we keep using the individual links, we lose that. You would have to add a new link handler to the dropbutton each time....
Comment #225
jibranThanks @alexpott for retesting it. Thank you @Berdir and @damiankloip for clearing this up. I'm +1 for both #223 and #224 but I have some concerns.
Ok that's my bad I should have done proper homework before replying.
This seems out of scope here. Should I create an issue and postpone this on that?
Ok right now I can't use operations links because there is no route template. If I'll create a separate issue for that can we make it pass beta eval?
One can always delete the existing view and re-import new one
Comment #226
BerdirIt's just one line, I don't think we need a separate issue for that?
Huh?
Comment.php:
Everything that you need is here?
Comment #227
jibran/me shuts up now and fixes the patch. :D
Thanks once again for pointing that out.
Comment #228
jibranReroll and replaced dropdown with operations links.
As it turns out links were already there I just have to add list builder.
Is it a good idea to create a follow up to replace
\Drupal\comment\Form\CommentAdminOverview
with\Drupal\comment\CommentListBuilder
? Although we are usingCommentAdminOverview
for two links/admin/content/comment
and/admin/content/comment/approval
.Comment #229
dawehnerGreat interdiff!
Feel free to try that out. I think using entity list builders for content entities is a bad idea, but this is just my personal opinion ...
Comment #230
miro_dietikerWow! Awesome to see this happen!
Comment #231
olli CreditAttribution: olli commented@jibran++
Can we remove AdminController now?
position: 0
Can we replace these with operations and commented_entity_label?
Not sure where this is coming from..
I think this is not needed anymore?
Should we use $this->getEntity($values) that also works with relationships?
Comment #232
jibranAwesome review @olli thank you very much.
admin/content/comment/approval
was missing unpublish action added that as well. Tests++'Select one or more comments to perform the update on.'
and after patch/with enabled view is'No comments selected.'
. I haven't changed that yet but we have to choose one. Test++comment.routing.yml
.DeleteComment
.NodeBulkFormTest
andUserBulkFormTest
as they are not needed.Comment #233
jibranNeeds some feedback.
Is this a proper way or do we have to create a new formatter for this?
dfac call seems wrong here. Is there any other way to do this?
Comment #235
dawehnerI'd argue its not. Better create an entire new field formatter for the usecase of comments, maybe called "Comment permalink" and make it just available for comments.
Comment #236
andypostNot sure it's good idea to introduce new formatter because permalink issues are not resolved
#2113323: Rename Comment::permalink() to not be ambiguous with ::uri()
#2198041: Comment::urlInfo no longer includes the comment-{cid} fragment
#2010202: Deprecate comment_uri()
Comment #237
Berdir2) dfac() seems definitely wrong. We need to get the node cache tag in that cached views table row so that when a node is changed, that row needs to be invalidated. This is a new field plugin right, so I'm pretty sure that can do that..
Here you need to return a render array that contains the cache tags for $entity And also the access cache metadata I think, so that two different users will get a different cache.
Comment #238
jibranThank you @dawehner, @andypost and @Berdir for the help, suggestions and replies.
CommentedEntityLabel
we are not using field formatter it's an old style views field plugin. We have to re-write it any way. Given that we have to support #178.2& #179.2
we have to come up with new plan.
@berdir also told me on IRC
I'll add it.
Meanwhile
CommentPermalinkFormatter
for #235 and updated the view.Comment #240
jibranFixed schema fails.
Comment #241
dawehnerThis class looks perfect now!
Yeah I don't think we have to still manually sanitize the value, due to autoescaping ... ?
Comment #242
jibranThis fixed everything mention in #238. After the current changes #241 is not applicable anymore. As it turns out entity_id is a base field of the comment entity so we don't need an extra views field plugin. I added one to fix preRender issue.
Does this mean that we should load all the entities for all ER field views? OR preRender is redundant here?
Other then that this is ready.
Comment #243
jibranComment #246
jibranWith the test fix. This needs multilingual approval now.
Comment #247
jibranFixed the doc block as per @andypost's review.
Comment #248
andypostwhy this hunks are commented out?
Comment #249
jibranSorry wrong patch and interdiff. Please ignore #247.
Comment #250
plach@246:
If I'm not mistaken, that's ignoring translations and acting on comments as a whole, right? Won't that cause similar issues to the ones fixed in #2484037: Make Views bulk operations entity translation aware?
Comment #251
andypostjust 5c here: historically we added language column to this table in d7 the idea behide was that comment will always have one language inherited from node
Comment #252
jibran@plach
There two reason I ignored that.
1. There is no language filter on comment admin page so it doesn't make sense to key it by language.
2. I haven't seen a practical example where we have to translate the comment. It's true that comment can have different languages but unlike nodes or terms we are not going to translate the comments or its fields.
Can you please have a look at views commented entity field plugin as well please? You have worked on views row caching and your are also familiar with ER system. PreRender for commented entity makes sense for me.
@andypost thanks for the clarity. I think inflation has increased the cost of the suggestion from 2¢ to 5¢ :D
Comment #253
jibranStill waiting for a response on #252.
Comment #254
plachMissing trailing dot.
Actually this code will always run if display cache is disabled, even if row cache is available. You may want to try and lazily execute it in ::getItems() instead. See Drupal\views\Plugin\views\field\Field::getItems().
Comment #255
jibranReroll
Thanks @plach for the review.
getItems()
, which runs on rows level, then it doesn't solve our problem so we are better off not doing that at all.Comment #256
jibranInterdiff in 255 was right patch was wrong. Re-uploading the patch and minor fix.
Comment #259
jibranFixed the schema and added update path and update path test.
Comment #261
jibranThis doesn't make sense and it passes on ci so retesting.
Comment #264
jibranThis should be green.
Comment #265
jibranI talked to @plach and @dawehner about this and they showed me how to use getItems properly. Here is the patch doing that.
Comment #266
larowlanLooks good to me, would like one last look from @plach though
Comment #267
larowlanplach: for your reference
Comment #268
larowlanAlso @jibran pointed out that the interdiff at #265 doesn't appear to execute, so asked if I could triple check that in a debugger, will do (please assign back to me @plach)
Comment #269
plachSurely translating comments is hardly as common as translating nodes, however I actually worked on a site with such a need (they were using machine translation for them). If this is committed as-is, we will get the same problems we had in #2484037: Make Views bulk operations entity translation aware, although on a fairly smaller scale. I'm afraid this could be a good argument to reject the patch, because at this stage this kind of issues need to be complete to be accepted.
I'd be fine with multiple deletion being addressed in a major follow-up, I wouldn't be fine if it were considered critical due to potential data-loss, although under very rare circumstances.
OTOH the bulk form handler is already coded to support multilingual entities and the node
DeleteMultiple
action + tests can serve as a good model to quickly put together the same functionality here. Ideally we should have a common delete multiple entity action, but that would require a bit more work.Comment #270
jibranI tested this last night
CommentedEntity::getItems()
loads all the commented entities once on each page load so it is getting executed. But it is rendering same value for each commented entity for all rows so I have to work on that.Comment #271
jibranReroll.
Comment #273
dobe CreditAttribution: dobe as a volunteer commentedI had been working on a very similar patch as for some reason this queue was hard for me to find (using "admin" in search was key). I kept searching "drupal 8 comment views" instead of "drupal 8 comment admin views". Here is a reroll of the patch. Some other things changed in the StringFormatter file. Should work against latest dev. Don't wanna step on any of the toes of the owners of this queue mainly just trying to get familiar with the backend of d8.
Comment #276
xjmJust discovered that this never landed. :( Sad kitties.
Also seems to now be blocked by this bug, which also affects comments: #2587551: Selecting Operation Links for File Type View throws an Exception
Comment #277
xjm@alexpott and I agreed that this is worth making an RC target.
Comment #278
miro_dietikerIs @larowlan working on the remaining items?
Otherwise please state what are the missing steps so someone of our team can complete the work based on the agreed approach.. I lost track after #229 when it was RTBC.
Comment #279
larowlanWe need to address #269 (can't do it in a follow-up now - has to go in with this).
So that is this
Comment #280
xjmI separated #2604618: Views operations dropbuttons do not work with Comment because it does not specify a list builder out into its own patch since it is a separate bug. Technically this issue is dependent on that one, but since it's just one line I think work can proceed here as well.
Comment #281
xjmWe could also possibly proceed by adding the new comment delete action and the permalink formatter in their own issues to make this issue's patch more manageable.
Comment #282
xjm#2604618: Views operations dropbuttons do not work with Comment because it does not specify a list builder and #2491875: EntityViewsData adds Operations links to all entities, which won't work if the entity type has no list builder, leading to WSOD on some views are both in now.
Comment #283
ArlaRerolled the patch in #273. Still needs work (#279) but I'm switching to Needs review to check tests.
Comment #285
ArlaThe view config had to be updated to match changed schema.
Comment #287
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedFixed failing CommentAdminTest.
Comment #289
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedImplemented abstract setDatabaseDumpFiles method in CommentAdminViewUpdateTest class.
Comment #291
jibranFrom #270
> it is rendering same value for each commented entity for all rows
Added test for that.
Comment #292
jibranForgot to add the test file.
Comment #295
jibranConverted BTB to webtest. Fixed the config schema fail.
Comment #297
jibranWith fetal fixes.
Comment #299
mbovan CreditAttribution: mbovan at MD Systems GmbH commentedFixing tests, docs and some code style issues.
Comment #300
jibranThanks @mbovan for the fixes.
This test should fail unless I'm doing something wrong. Can someone manually test this case. Meanwhile let me try something else.
Comment #301
xjmReuploading @jibran's patch with a patch extension for the testrunner.
Comment #303
andypostList builder and
comment_update_8001()
already added in #2604618: Views operations dropbuttons do not work with Comment because it does not specify a list builderAlso fixed code style and usage of deprecated entity manager
Still broken views test, looks because of loading of commented entities
Comment #304
jibranGood #300 failed so I was not wrong. Now we need to debug that test I suspect it is related to cache tags. Maybe @dawehner can have a look at it. Other then that fail I think this patch is ready to go.
@andypost why do we need to import comment delete action? Isn't it imported when view get enabled by update? Anyway if we have to import it anyway then the action should be imported before view because view is using that action.
Comment #306
andypost@jibran thanx! nice catch.
Removed out of scope changes in
\Drupal\Core\Field\Plugin\Field\FieldFormatter\StringFormatter
Added test for import of action.
Reverted changes in
\Drupal\comment\Tests\CommentTestBase
because we can use this for users so todo for #1972242: User compact view mode is not configured in minimalSo for user we need to create comment manually
Removed debug introduced in previous patch
Suppose RTBC
Comment #307
jibranThis will not reproduce the same situation we have to post comment using ui. I think it will not fail.
Comment #308
xjmI tested the latest patch manually with a few comments and users. The author name disappears when a comment is edited. Steps to reproduce:
/admin/content/comment
and confirm that the username is displayed./admin/content/comment
. The username disappears.This does not happen on the admin comments page in HEAD.
Comment #309
xjm@andypost confirmed that #308 is a bug in HEAD with comment views. Unfortunately, I think it's a blocking bug. :( We can't regress this so that edited comments have no author name listed on the admin comments page. That probably means this will have to get postponed to 8.1.x unless the fix for that other issue (I didn't check whether it exists somewhere yet) is trivial to fix.
Comment #310
xjmFiled #2614504: Values of 'name' & 'email' fields should be NULL when comment has author (uid > 0) which likely blocks this issue. :(
Comment #311
andypostuncommented test
@jibran while #1972242: User compact view mode is not configured in minimal is open we can't use comments for user in integration tests, need to change profile to standard or properly setup user displays
Comment #312
larowlanPretty sure this is the cause of #2614504: Values of 'name' & 'email' fields should be NULL when comment has author (uid > 0) and xjm's issues. The name field is the wrong field. We should be using uid which has the AuthorFormatter which handles using usernames of registered users and falls back to the entered name value for anonymous users.
The name field *does not* support that level of flexibility.
Comment #313
catchMoving to 8.1.x.
Comment #314
dobe CreditAttribution: dobe as a volunteer commentedIf your setting this to 8.1.x where does that leave: #1823450: [Meta] Convert core listings to Views?
Comment #315
jibranFixed #312.
Comment #317
jibranFixed the tests. Working on a test for #308
Comment #318
jibranFixed #279.
Comment #319
jibranGreen tests.
Comment #321
jibranLeaving minor review to self.
Use
$module_handler->getModule('comment')->getPath();
Improve the desc.
Comment #322
jibranAdded tests for #308 after implementing #312 the bug described in #308 is not an issue anymore.
Also fixed the feedback from #321.
This is ready for review now also update IS.
Comment #323
jibranComment #324
jibranComment #325
jibranAnyone care to review.
Comment #326
andypostany reason for the additional method call?
I pretty sure this changes should be moved to separate issue!
1) get rid of AdminController
2) Details about comments and translations deletion
Also separate issue, new action and proper delete implementation
new formatter, maybe better fits into #2227503: Apply formatters and widgets to Comment base fields
this actually not good idea, the rest of code supposes "node" as entity.
Comment #327
jibranCommentPermalinkFormatter
for that.Comment #328
dawehnerMh, so a maintainer decided its good to have those changes, so let's accept that. In general it feels counterintuitive that we have to change parts of the code which is overridden by views anyway though.
I totally agree with @jibran here.
Sure, but its more stuff to review or rather it removes time to review the actual important changes.
IMHO we should check for the entity type here as well. This really just makes sense on comments.
Ideally we would also go to the comment page and check whether something is rendered, just to be sure.
Comment #330
andypostComment #332
vprocessor CreditAttribution: vprocessor at Skilld commentedreroll is ready
Comment #335
vprocessor CreditAttribution: vprocessor at Skilld commentedCI error had been fixed
Comment #338
xjmWhen the Views maintainers and core committers discussed this issue at DrupalCon Dublin, we agreed that this is still a major task.
It looks like the Migrate tests might need an update for this issue -- retesting #335 to be sure.
Comment #340
jibranComment #341
jibranThis is ready for the review again.
This can be removed after next review.
Comment #342
LendudeApplied the patch, ran the update, and tada! a View, Nice!
Reviewing this makes me wish this had been split up into multiple issues with a meta or something, what a monster :)
some things I saw:
Lets use setUnpublished(), this use is deprecated
And lose the TRUE here.
$node should be $comment and that should fatal, so I think we lack test coverage for this
$current_user is missing from @params
This should extend EntityField
Lets lose the FALSE
lets lose the FALSE
Comment #343
jibranFixed #341.
Re: #342 Thanks, for the review.
Comment #345
jibranWrong substitution.
Comment #346
jibranAdded tests for #342.3
Comment #347
jibranI had a chat with @Lendude on IRC here is the new patch fixes his feedback.
Fixed it.
The reason for this is because we have different operations on both pages.
Comment #348
Lendude#347.2 Ah yeah that makes sense.
@jibran sorry about the fractured feedback but every time I look through this I see some new stuff, its big, there is a lot to look at :)
I think this is really close, one (hopefully) last thing: We seem to be missing explicit test coverage for
\Drupal\comment\Plugin\Field\FieldFormatter\CommentPermalinkFormatter
. Can't really see any implicit test coverage either, or am I overlooking that?Comment #349
jibranThanks, @Lendude for the reviews all good as long as we are moving along and improving the patch I'm fine with the changes.
Nice catch, I'll add some asserts for implicit tests.
Comment #350
jibranAdded some asserts around
AuthorNameFormatter
andCommentPermalinkFormatter
.Comment #352
jibranCommentEditTest++
Because of this convoluted logic 'name' field of comment can be empty, at that point we can show the uid of the comment so I have updated the view like this:
Comment #353
Lendude@jibran great work on this.
The extra test coverage looks good and great to see we have coverage to catch stuff like #350.
Comment #354
larowlanIs this still blocked on #2614504: Values of 'name' & 'email' fields should be NULL when comment has author (uid > 0)?
Comment #355
jibranRE: #354 No, I have worked around that in #352 and added some asserts for that as well.
Comment #356
andypost@larowlan This is no longer blocker, admin view will use
uid
field to display author in "comment manner"So I changed title and explained in #2614504-79: Values of 'name' & 'email' fields should be NULL when comment has author (uid > 0) - this is not blocker but about data integrity (name&mail should be cleared if comment gets author)
Comment #357
larowlanUpdating issue credits
Adding myself for reviews/manual testing
Adding @miro_dietiker for manual reviews/suggestions
Adding @tim.plunkett for mentoring and reviews
Adding @Berdir for mentoring and reviews
Adding @Lendude for reviews
Adding @plach for providing guidance on translation/language and reviews
Adding @olli for reviews
Adding @damiankloip for reviews
Comment #358
larowlanReviewed code again, looks good.
Queued tests for other DB backends in case we have a sort issue.
Manually testing upgrade path and patch.
Comment #359
larowlanManually tested, works well.
Confirmed that #310 is resolved
We just need a change record here.
Comment #360
larowlanAlso, some follow up feature requests to add some filters and other shiny things that weren't previously possible would be awesome.
Comment #361
jibranHere we go https://www.drupal.org/node/2898013. Thanks, for reviewing it.
Comment #362
jibranMaybe I'll get lucky by re-uploading.
Comment #365
larowlanWoohoo!
Thanks to everyone for the marathon effort here - particularly @jibran and @pcambra with 45 and 23 patches respectively.
Committed as 04d6992 and pushed to 8.5.x.
Cherry-picked as 9c443f8 and pushed to 8.4.x.
Published changed record.
Can someone open a follow-up to add some shiny filters like we have on admin/content.
Once again - thank you so much to everyone who has helped get this one over the line.
Comment #366
larowlanFixing release notes tag
Comment #367
larowlanNext stop #2207263: Try and build /forum and /forum/{tid} with views?
Comment #368
jibran> Can someone open a follow-up to add some shiny filters like we have on admin/content.
> Also, some follow up feature requests to add some filters and other shiny things that weren't previously possible would be awesome.
On super powerful public demand added #2898344: Add filters to the comments administration pages.
Thanks for the commits @larowlan.
Comment #369
xjmSo I don't see any profiling documented on this issue. Usually when we add new listing queries we at least should add an explain. See: https://www.drupal.org/core/gates#performance
Not necessarily a reason to roll it back and hopefully the view is already being optimized correctly, but we should confirm with an EXPLAIN on the view query and possibly compare it to the 8.3.x query for the old hardcoded page.
Comment #370
jibran\Drupal\comment\Form\CommentAdminOverview
handles is it in three steps and we load user entity for each row.The view query is
And the exlplain of this is
We have never discussed views render queries when replacing the admin page. Do you want me to share those as well?
Comment #371
andypost@jibran I bet @xjm called to measure performance because views load each single comment while rendering, so removal of
$comments = $this->commentStorage->loadMultiple($cids);
should bring serious impact on performance particularly on cold caches.Explaining main query will not show actual impact of the change, so checking perf here is tricky
Probably this cases are most needed
1) cold entity cache - the most issue I bet
2) cold render & views cache
3) posting new comment (must invalidate "comment_list" cache tag) so affects both
Comment #372
jibranCan we please create a follow-up issue for this?
Comment #373
xjm@jibran, well, the core gates are supposed to be addressed before commit, so we should confirm them in this issue.
Comment #374
larowlanHere's a call graph between 8.3 and 8.4 with warm caches https://blackfire.io/profiles/compare/c8b886bc-a6b4-49b4-a0da-7cdc8ea5dc....
Net take away is that building the form was slower than the view.
Tested with previousnext.com which has lots of comments.
Disabled breakpoint, admin toolbar, toolbar, responsive image which make the page 6x slower (looking at you admin toolbar).
The table display (8.3) took 131ms, the view display (8.4) took 135, but the form (8.3) took 97ms whereas the views pre-render (form stuff) took 93ms, so its swings and roundabouts.
Additional query checks/EXPLAIN etc are in #2898344: Add filters to the comments administration pages
Will ping catch for sign-off.
Comment #375
larowlanCold caches https://blackfire.io/profiles/compare/11203d6d-d566-4d61-afee-216467eb74... not as big a difference, but still in favour of this patch
Comment #376
jibran@catch posted in #2898344-48: Add filters to the comments administration pages