Convert the recent_comments block to a view

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentBlockTest.phpundefined
@@ -62,47 +65,36 @@ function testRecentCommentBlock() {
+debug($comments[0]->subject->value);

:)

Otherwise, I think this is looking good!

Any chance we could postpone this in hope of getting #731724: Convert comment settings into a field to make them work with CMI and non-node entities in first? It's a hell of a re-roll.

Seems reasonable, that is a pretty big patch!

Oh yeah more queries converted to views!

The Views configuration files are very inconsistent in terms of their explicit-ness.

Are you talking about the views files provided by core, or also the ones which are saved new? Many of the default views, which are part of core, has been saved in a time when only needed values where exported. Afaik nowadays every value get in there. If you wonder about the test files, ... there are people which write views config by hand. Let's talk about that tomorrow if you want.

e. I *think* the view does that as well (at least it joins on {node}), but I'm not sure.

It totally does that, see #1222324: Fix query access control on relationships (comments)

+++ b/core/modules/comment/comment.moduleundefined
--- /dev/null
+++ b/core/modules/comment/config/views.view.comments_recent.ymlundefined

Just wondering, have you saved the view or also rearranged some of the lines?

+++ b/core/modules/comment/config/views.view.comments_recent.ymlundefined
@@ -0,0 +1,240 @@
+      fields:
+      fields:

These lines look odd.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentBlockTest.phpundefined
@@ -62,47 +65,36 @@ function testRecentCommentBlock() {
     $label = $block->label();

$label is an unused variable here.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentBlockTest.phpundefined
@@ -62,47 +65,36 @@ function testRecentCommentBlock() {
+    for ($i = 1; $i < 11; $i++) {
+      $this->assertText($comments[$i]->subject->value, format_string('Comment @number found in block.', array('@number' => 11 - $i)));
...
+        $position = strpos($this->drupalGetContent(), $comments[$i]->subject->value);
+        $this->assertTrue($position < $previous_position, format_string('Comment @a appears before comment @b', array('@a' => 11 - $i, '@b' => 12 - $i)));

I'm wondering whether we should use xpath() instead of assertText() to determine the printed comments.

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentBlockTest.phpundefined
@@ -62,47 +65,36 @@ function testRecentCommentBlock() {
+        $previous_position = $position;

This code ordering is odd, kind of similar to _filter_xss_attributes, but yeah it's technical right.

+++ b/core/modules/comment/comment.moduleundefined
--- /dev/null
+++ b/core/modules/comment/config/views.view.comments_recent.ymlundefined

Just wondering, have you saved the view or also rearranged some of the lines?

+++ b/core/modules/comment/config/views.view.comments_recent.ymlundefined
@@ -0,0 +1,240 @@
+      fields:
+      fields:

These lines look odd.

Looks like a random failure there.

+++ b/core/modules/comment/config/views.view.comments_recent.ymlundefined
@@ -0,0 +1,217 @@
+status: '1'

Sorry if I missed something, but do we want this enabled by default? I would say maybe not.

+++ b/core/modules/comment/config/views.view.comments_recent.ymlundefined
@@ -0,0 +1,217 @@
+        area_text_custom:
...
+          plugin_id: text_custom

Awesome! I'm glad that we are using this :) The right empty handler to use with default views.

That's a good point. Sounds good to me.

#17: 1938062-17-3.patch queued for re-testing.

This looks ready for me. There might be some discussion that DefaultRecentCommentsView.php tests got removed, but hey we test the block already in the other test, so this seems fine.

+++ b/core/modules/comment/config/views.view.comments_recent.ymlundefined
@@ -0,0 +1,217 @@
+          admin_label: ''
+          label: ''
+          exclude: '0'
+          alter:
+            alter_text: '0'
+            text: ''
+            make_link: '0'
+            path: ''
+            absolute: '0'
+            external: '0'
+            replace_spaces: '0'
+            path_case: none
+            trim_whitespace: '0'
+            alt: ''
+            rel: ''
+            link_class: ''
+            prefix: ''
+            suffix: ''
+            target: ''
+            nl2br: '0'
+            max_length: ''
+            word_boundary: '0'
+            ellipsis: '0'
+            more_link: '0'
+            more_link_text: ''
+            more_link_path: ''
+            strip_tags: '0'
+            trim: '0'
+            preserve_tags: ''
+            html: '0'
+          element_type: ''
+          element_class: ''
+          element_label_type: ''
+          element_label_class: ''
+          element_label_colon: '0'
+          element_wrapper_type: ''
+          element_wrapper_class: ''

I think we removed the empty exported values from /node?

Patch looks great to me other than that. However, it would be good to have confirmation that someone has actually manually tested this. :)

Hmm, not sure, those values are for fields, and we don't have fields on /node.

#17: 1938062-17-3.patch queued for re-testing.

Yeah damian is right about this.

This god remove on the frontpage view, because when you actually add a view during the UI you always end up with at least one field, even you configured it to be a node-view one. This not-needed fields then got removed.

#1807322: Filter comments taking into account the current content language might not be needed when we get this issue done. (I'll add it to the summary.)

Added screenie and link to plugin namespacing issue.

added related issue about multilingual filter of comments

Just a rerole.

I'm confused about the size of the patch but it seems to be that DefaultViewRecentComments.php landed? Why is this file actually tested, as it doesn't edn with "Test"?

There we go.

Hm? Test classes are tested based on whether they live in the module's PSR-0 Tests/ directory. Ending the file names in Test is just a naming convention for clarity. However, I'm not seeing the file you're referring to? The fail is in CommentBlockTest and sounds like it might be caused by needing to rebuild the derivative cache.

Edit: I posted in reply to an earlier comment, disregard. This is what happens when you open all your issues in tabs at once.

Tobias removed this test from his patch.

+++ b/core/modules/comment/config/views.view.comments_recent.ymlundefined
@@ -0,0 +1,217 @@
+      relationships:
+        nid:
+          id: nid
+          table: comment
+          field: nid
+          required: '1'
+      fields:
+        subject:
+          id: subject
+          table: comment
+          field: subject
+          relationship: none
+          group_type: group
+          admin_label: ''
+          label: ''
+          exclude: '0'
+          alter:
...
+      filters:
+        status:
+          value: '1'
+          table: comment
+          field: status
+          id: status
+          expose:
+            operator: '0'
+          group: '1'
+        status_node:
+          value: '1'
+          table: node
+          field: status
+          relationship: nid
+          id: status_node
+          expose:
+            operator: '0'
+          group: '1'

These are missing plugin_id.

Otherwise, this looks awesome!

There we go.

ups.

Done some testing. Looks great.
Comment block without patch.

Comment block with patch.

Visible no difference only block config page is changed. Number of recent comments field is moved to views pager settings.
Code is verified by @tim.plunkett in #40 and interdiff looks good in #42. It is RTBC if it'll came back green.

Non related to issues

• While adding comment I am unable to press submit or preview button due JS error.

An invalid form control with name='comment_body[und][0][value]' is not focusable.
Dono about any active issue related this.

• There are some major styling issues when I added the comments. I am attaching the chorme(Version 25.0.1364.172 m) and firefox(19.0.2) sceenshots.

Dono about any active issue related this as well.

Can we also add before/after screenshots for the block admin workflow for adding and configuring this block?

with regards to #43

#1954968: Required CKEditor fields always fail HTML5 validation is the follow-up for not being able to submit to add a comment.

@jibran
What were the major styling issues exactly?

Updated issue summary. Added links to related issues.

added follow-up section

updated the issue summary with follow-up issues section.

next: steps to test
going to try using devel to get some comments.

added follow-up todo for time ago em and plugin name

Just to be clear #1954968: Required CKEditor fields always fail HTML5 validation is not due to this patch.
@YesCT from #45
In D7 bartik shows comments like this

In chrome D8 bartik shows comments like this

In firefox D8 bartik shows comments like this

@xjm #44
Before

After

Hmm, the last screenshot in #48 makes it look to me like Views blocks aren't working properly with the new block title behavior from #1875260: Make the block title required and allow it to be hidden. (That's not introduced by this patch, but it is a concern.) Can we get a followup issue for that?

Here's the block on the block library page. Mostly unchanged other than that it of course shows up under the "Views" category now rather than "Comments".

in #47
1.
It looks like before the patch, the body and the login/edit-delete links line was inside the area with the boarder.
after the patch, the body and the login/edit-delete links line is outside of that area with the boarder.

2.
and the difference between firefox and chrome is if the boardered area is taking up the full width, or if it is scaled in the case of a narrow comment.

Also in #48
3.
Before the patch it has a 'number of recent comments' select which defaults to 10.
after the patch it does not. (I did not check to see what the view defaults to, if it's 10, but just is not showing the select, or if there is no limit)

after the patch it does not. (I did not check to see what the view defaults to, if it's 10, but just is not showing the select, or if there is no limit)

That's because this configuration is moved to the view. (The same is true of the title actually.)

Also, #47 is out of scope for this issue; it looks to be a bug in HEAD if I understand the screenshots. Let's open a followup for that and for #49.

after the patch it does not. (I did not check to see what the view defaults to, if it's 10, but just is not showing the select, or if there is no limit)

That's because this configuration is moved to the view. (The same is true of the title actually.)

This does actually raise a good point -- it's not obvious to end users how to configure this block (using Views). Maybe we need a link on the views' block form (or in the dropbutton in the block library?) to edit the View display. (This also could extend to menus, custom blocks, etc.--anything with a derivative based on an entity.) Another followup. :)

@xjm
This is indeed a nice idea! Opened a follow up for that: #1956134: Provide helpful editing links on "admin/structure/block" for deriver blocks (menu, views, block content, etc.)

Note: Haven't read the issue yet, just tested before/after the patch.

Copy/pasting "after" screenshot from above:

So as much as I'm all about this movement of converting dumb one-off listings to views, the current patch represents a significant reduction in the UX around both placing and editing this block. :(

Most notably:

1. Now, you must fill in a machine name in order to place the block (a field most users have never seen before on any other blocks since it's a mere footnote). Why? It should be auto-generated from the block's title like other blocks.
2. There is no way from this screen to change "80% use case" settings like the block title and the number of records to show in a block. To do this, you'd currently have to go to a completely separate interface, namely Views, which a) many (most?) of the people dealing with this page have never used before, and b) is in no way remotely apparent from anything on this screen.

If we were before feature freeze, I might be inclined to let this in and do some kind of nice UX wrapper as a follow-up, but we're not before feature freeze, and we already have one critical task to make the block UI shippable, which is a sprawling morass of sub-issues, so I'm very disinclined to add more to it. So I really think these two problems need to be worked on before this is committable. Obviously, we can discuss it if people feel very strongly, but then I think my next move is assigning this to Dries to make the call. This is one of those things that spans the chasm between site builder and content creator, and it doesn't make sense to spend tons of time trying to make the content creator experience better in one place if we keep significantly chipping away at it in others.

Additionally, since this is the first such block we've converted, makes sense to get some before/after profiling data here.

There is no way from this screen to change "80% use case" settings like the block title and the number of records to show in a block

I have mixed feelings about this point. On the one hand, I can see the point of "I just want to change the number of items listed" and that there is a (IMO not too serious) UX regression from D7 here. On the other, one of the goals of converting core listings is to make the Views module's functionality discoverable. If we hide away the Views UI from people, we're losing some of that value, which I almost think is more important than just removing legacy code.

Edit: Integrating the Views UI with the Blocks UI was, back in October, a fairytale dream of mine for D8, and the original reason I reviewed the BAP patch. Now it's D9 material and then some, and there's no avoiding that.

On the other other hand, do people really care about the number of items that much?

Edit: The title/machine name thing I consider a bug, and I suggested filing it separately above in #49. I'm fine with addressing that before this goes in.

"If we hide away the Views UI from people, we're losing some of that value, which I almost think is more important than just removing legacy code."

The problem with applying that logic to "# of things to show here" is that it's perfectly conceivable that a completely non-technical person wants to change that value for a particular block placement because they're putting together some landing page for mobile phones and that block that normally shows 10 things is going to take up the whole screen, so they want to restrict it to 3 things instead. That, in fact, is basically one of the only gains we got from SCOTCH at this point: the possibility to alter things like titles and "number of things to show here" on a per-instance basis.

Pushing Views UI at non-technical people with such a use case is not only a disaster waiting to happen, it's actually potentially a tremendous security risk, because with access to "administer views" permissions they can (accidentally or otherwise) pull up a list of content their role shouldn't necessarily be able to see.

So I really do think we need some kind of workaround for Views blocks that's maybe not the crazy "fairytale dream" we were originally hoping for, but still doesn't require giving administer views access to everyone with administer blocks access.

Filed #1957276: Let users set the block instance title for Views blocks in the Block UI.

Reading the post in #58 let's me think of a system like ctools views integration has. You can configure in your view, what settings are available to be changed on the panels configuration. The following options have been available in D7:

• use a pager
• items per page
• offset of the pager
• link to the view
• should there be a more link
• Override the internal path of the view
• Override the title of the view
• Should the exposed form be shown
• Display just certain fields

What do you think about adding such a system to the block integration?

Pushing Views UI at non-technical people with such a use case is not only a disaster waiting to happen, it's actually potentially a tremendous security risk, because with access to "administer views" permissions they can (accidentally or otherwise) pull up a list of content their role shouldn't necessarily be able to see.

This would be then solved with this kind of override system.

#1956134: Provide helpful editing links on "admin/structure/block" for deriver blocks (menu, views, block content, etc.) would certainly also help us here, because we could not only place a link on the two different block listings,
but we could move this extra links to local actions as well, so you can already access them from the block configuration page.

Btw.: Isn't the actual reality of most sites, that you just have one person actually building the website, and maintaining it? Only big sites actually care about permissions for certain administration pages. Based on that and the statistics of the install-base of views you could maybe argue that most people actually have seen this page.

See #1957346: Add some settings on the block display to allow overrides on the block instance configuration for the follow up.

I think #59 and #61 addressed the issues raised in #56and #58. We can fix these issues in followups. So RTBC form me. I don't think we have to postpone this on above issues.

What do you think about adding such a system to the block integration?

I would be +1 on that, or maybe a somewhat simplified subset of the options.

Since it's during feature freeze, we need to fix these related issues before commit, not in followups. Usability is a gate, after all. This is okay. We will get it done, and Drupal will be better for it. :)

+++ b/core/modules/comment/config/views.view.comments_recent.ymlundefined
@@ -0,0 +1,222 @@
+tag: ''

We should give this the 'default' tag, like other default views, I think.

Good point!

with attempt at steps to reproduce

#66: drupal-1938062-66.patch queued for re-testing.

Rerolled.

I filed a new, re-scoped issue to handle the title UX problem: #1967460: Display the block title and populate the machine name for views blocks.

added remaining tasks section which should clarify what is blocking this.

#69: drupal-1938062-69.patch queued for re-testing.

Rerolled.

Reroll

Add files that I missed in previous patch.

I did some profiling on this.

Tests were done on a fresh install (8.x vs. 8.x+#75). Recent comments block was the only view on the page. I displayed it in the sidebar while loading "Basic page" node. Before testing I generated 1000 nodes using devel_generate. Every node had at most 100 comments.

Request was much longer after I applied the patch. Query itself was much faster but it looks like time increase happened also due to entity loads that happen inside Views.

Memory footprint also increased significantly.

XHProf data is attached.

Results summary:

-----

In-depth results

Before

mysql> SELECT SQL_NO_CACHE c.* FROM comment c INNER JOIN node_field_data n ON n.nid = c.nid WHERE (c.status = 1) AND (n.status = 1) AND (n.default_langcode = 1) ORDER BY c.created DESC, c.cid DESC LIMIT 10 OFFSET 0;
+-------+--------------------------------------+-------+------+-----+---------------------------------+-----------+------------+------------+--------+-----------+----------------+----------------------------+----------+----------+
| cid   | uuid                                 | pid   | nid  | uid | subject                         | hostname  | created    | changed    | status | thread    | name           | mail                       | homepage | langcode |
+-------+--------------------------------------+-------+------+-----+---------------------------------+-----------+------------+------------+--------+-----------+----------------+----------------------------+----------+----------+
| 50621 | f0eeed86-ae1d-4f84-8300-dbadbffa8ad3 | 50602 | 1000 |   0 | Iriure Jus Paratus              | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 02.00.00/ | devel generate | devel_generate@example.com | NULL     | und      |
| 50620 | a53903d8-05d7-428a-822c-44d58e72f766 | 50596 | 1000 |   0 | Huic Valde                      | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 01.01/    | devel generate | devel_generate@example.com | NULL     | und      |
| 50619 | c29b72f2-45d3-4991-a300-c365e16aec9e |     0 | 1000 |   1 | Abico Ad Erat Praemitto         | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 0a/       | admin          | devel_generate@example.com | NULL     | und      |
| 50618 | 03519a61-1b23-4b58-be00-b9f3dc9a3b3b | 50599 | 1000 |   0 | Conventio Duis Iaceo Uxor       | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 03.00.04/ | devel generate | devel_generate@example.com | NULL     | und      |
| 50617 | 447d3dbe-6c45-49e9-b900-cfccb7ab4a9b | 50597 | 1000 |   1 | Dolus Oppeto Te Vindico         | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 02.01/    | admin          | devel_generate@example.com | NULL     | und      |
| 50616 | 0464367a-5f3e-4288-9e29-41558a723275 |     0 | 1000 |   0 | Adipiscing Esca Iriure Plaga    | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 09/       | devel generate | devel_generate@example.com | NULL     | und      |
| 50615 | 4fe0fe63-72ee-4d41-b235-534bcd37c897 | 50599 | 1000 |   0 | Abdo Nulla Veniam               | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 03.00.03/ | devel generate | devel_generate@example.com | NULL     | und      |
| 50614 | 0a99c7d9-550e-4afa-a300-ee1792f65a6a | 50596 | 1000 |   0 | Blandit Consectetuer Scisco     | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 01.00/    | devel generate | devel_generate@example.com | NULL     | und      |
| 50613 | e1dbc713-0ccd-49f9-b107-7fd96e1b2746 |     0 | 1000 |   0 | Consectetuer Inhibeo Jugis Quia | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 08/       | devel generate | devel_generate@example.com | NULL     | und      |
| 50612 | 8976d77c-4027-4915-9c00-b4a45f323c6e | 50599 | 1000 |   0 | Consectetuer Singularis         | 127.0.0.1 | 1372162126 | 1372162126 |      1 | 03.00.02/ | devel generate | devel_generate@example.com | NULL     | und      |
+-------+--------------------------------------+-------+------+-----+---------------------------------+-----------+------------+------------+--------+-----------+----------------+----------------------------+----------+----------+
10 rows in set (0.21 sec)

mysql> explain SELECT c.* FROM comment c INNER JOIN node_field_data n ON n.nid = c.nid WHERE (c.status = 1) AND (n.status = 1) AND (n.default_langcode = 1) ORDER BY c.created DESC, c.cid DESC LIMIT 10 OFFSET 0;
+----+-------------+-------+-------+----------------------------------------------------+-----------------------+---------+---------------------+------+----------------------------------------------+
| id | select_type | table | type  | possible_keys                                      | key                   | key_len | ref                 | rows | Extra                                        |
+----+-------------+-------+-------+----------------------------------------------------+-----------------------+---------+---------------------+------+----------------------------------------------+
|  1 | SIMPLE      | n     | range | PRIMARY,node_default_langcode,node_status_type,nid | node_default_langcode | 4       | NULL                | 1000 | Using where; Using temporary; Using filesort |
|  1 | SIMPLE      | c     | ref   | comment_num_new,comment_nid_langcode               | comment_num_new       | 5       | drupal8.n.nid,const |    1 |                                              |
+----+-------------+-------+-------+----------------------------------------------------+-----------------------+---------+---------------------+------+----------------------------------------------+
2 rows in set (0.00 sec)

After

mysql> SELECT SQL_NO_CACHE comment.subject AS comment_subject, comment.cid AS cid, comment.nid AS comment_nid, comment.changed AS comment_changed, comment.created AS comment_created, comment.cid AS comment_cid, node_comment.nid AS node_comment_nid FROM comment comment INNER JOIN node node_comment ON comment.nid = node_comment.nid INNER JOIN node_field_data node_comment__node_field_data ON node_comment.nid = node_comment__node_field_data.nid WHERE (( (comment.status <> 0) AND (node_comment__node_field_data.status = 1) )) ORDER BY comment_created DESC, comment_cid DESC LIMIT 10 OFFSET 0;
+---------------------------------+-------+-------------+-----------------+-----------------+-------------+------------------+
| comment_subject                 | cid   | comment_nid | comment_changed | comment_created | comment_cid | node_comment_nid |
+---------------------------------+-------+-------------+-----------------+-----------------+-------------+------------------+
| Iriure Jus Paratus              | 50621 |        1000 |      1372162126 |      1372162126 |       50621 |             1000 |
| Huic Valde                      | 50620 |        1000 |      1372162126 |      1372162126 |       50620 |             1000 |
| Abico Ad Erat Praemitto         | 50619 |        1000 |      1372162126 |      1372162126 |       50619 |             1000 |
| Conventio Duis Iaceo Uxor       | 50618 |        1000 |      1372162126 |      1372162126 |       50618 |             1000 |
| Dolus Oppeto Te Vindico         | 50617 |        1000 |      1372162126 |      1372162126 |       50617 |             1000 |
| Adipiscing Esca Iriure Plaga    | 50616 |        1000 |      1372162126 |      1372162126 |       50616 |             1000 |
| Abdo Nulla Veniam               | 50615 |        1000 |      1372162126 |      1372162126 |       50615 |             1000 |
| Blandit Consectetuer Scisco     | 50614 |        1000 |      1372162126 |      1372162126 |       50614 |             1000 |
| Consectetuer Inhibeo Jugis Quia | 50613 |        1000 |      1372162126 |      1372162126 |       50613 |             1000 |
| Consectetuer Singularis         | 50612 |        1000 |      1372162126 |      1372162126 |       50612 |             1000 |
+---------------------------------+-------+-------------+-----------------+-----------------+-------------+------------------+
10 rows in set (0.10 sec)

mysql> EXPLAIN SELECT comment.subject AS comment_subject, comment.cid AS cid, comment.nid AS comment_nid, comment.changed AS comment_changed, comment.created AS comment_created, comment.cid AS comment_cid, node_comment.nid AS node_comment_nid FROM comment comment INNER JOIN node node_comment ON comment.nid = node_comment.nid INNER JOIN node_field_data node_comment__node_field_data ON node_comment.nid = node_comment__node_field_data.nid WHERE (( (comment.status <> 0) AND (node_comment__node_field_data.status = 1) )) ORDER BY comment_created DESC, comment_cid DESC LIMIT 10 OFFSET 0;
+----+-------------+-------------------------------+-------+--------------------------------------+-----------------+---------+-------------------------------------------+------+----------------------------------------------+
| id | select_type | table                         | type  | possible_keys                        | key             | key_len | ref                                       | rows | Extra                                        |
+----+-------------+-------------------------------+-------+--------------------------------------+-----------------+---------+-------------------------------------------+------+----------------------------------------------+
|  1 | SIMPLE      | node_comment                  | index | PRIMARY                              | tnid            | 4       | NULL                                      |  731 | Using index; Using temporary; Using filesort |
|  1 | SIMPLE      | node_comment__node_field_data | ref   | PRIMARY,node_status_type,nid         | PRIMARY         | 4       | drupal8.node_comment.nid                  |    1 | Using where                                  |
|  1 | SIMPLE      | comment                       | ref   | comment_num_new,comment_nid_langcode | comment_num_new | 4       | drupal8.node_comment__node_field_data.nid |    1 | Using where                                  |
+----+-------------+-------------------------------+-------+--------------------------------------+-----------------+---------+-------------------------------------------+------+----------------------------------------------+
3 rows in set (0.00 sec)

This have collision with twig in #1987396: Refactor & Clean-up comment.module theme functions

+++ b/core/modules/comment/comment.module
@@ -441,39 +438,6 @@ function comment_permalink(Comment $comment) {
-function comment_get_recent($number = 10) {

This is a API change and now needs approve

Also related #19734: add 'Pending Comments' block #1059608: Fix comment_get_recent ordering

Separate issue to remove comment_get_recent() - #2054993: Remove (untested, unused, broken) comment_get_recent()

-function comment_get_recent($number = 10) {

People can just use entity query instead, so I don't think this is a problem at all.

@andypost, I have tentative approval for Dries on minor API changes for these conversions generally, but he of course will also need to look at them on a case-by-case basis.

#1957346: Add some settings on the block display to allow overrides on the block instance configuration has also landed, so let's incorporate that here.

I am wondering whether it is a good investment of time for dries to look at these minor API changes, because seriously, if someone ports a module now, the person will, at least from my perspective, not expect that the api is frozen. It seems to be the case that we communicate that different, but maybe it is better to be honest.

@dawehner, the core maintainers have stated that this is what they prefer. And yeah, I think it's actually not unreasonable to expect APIs to be frozen after... API freeze...

I am sorry about my comment.

Let's not touch comment_get_recent() here, and discuss it in #2054993: Remove (untested, unused, broken) comment_get_recent() instead. IMO this issue should only remove the block plugin since that's what the Views block deprecates.

Re-roll and added comment_get_recent() as suggested in #86.

@@ -386,6 +383,39 @@ function comment_permission() {
+function comment_permalink(Comment $comment) {

This was removed in #1978914: Convert comment_permalink() to a Controller, why is it readded?

Also did you roll this patch with git copies turned on?

Yeah this needs to be fixed but then let's get it in, finally.

Here we go.

@tim.plunkett: I don't think so. Should I use it?

You're missing the new view file there. This is #87 + the interdiff from #90, which was correct.

Turns out its not similar enough for copies to kick in.

Thank you very much. I did another simplytest check and it worked how I would expect a view block to work.

Given that webchick reported UX issues in #56 and that we're not addressing them in this patch, it would be good for her to sign off on the proposal to do this later.

We fixed that in #1957346: Add some settings on the block display to allow overrides on the block instance configuration.

Oh neat. I hadn't seen that yet. Based on that, this look pretty darn committable. ;-)

Manually tested after rerolling to be sure.

The conflict was just in a file we were outright deleting ($this->node->type vs $this->node->getType()).

Rambly review coming up! Summary below.

So first order of feedback is I couldn't find the block at all. This is because it is showing up categorized as "Views"; I would expect it to be under "Comments," or else "Content" if we want to rename "Node" to Content and make it a general bucket for all content-related thingies.

Second order of feedback is the "Machine name" field is expanded for some reason on the place block screen. I would expect this to be collapsed. This looks like it's not the fault of this patch though, as other blocks are doing the same thing. I also can't close the dialog because the X box keeps moving back up under the Toolbar which is "special." ;) Also weird that the button label is "Save" and not "Place." However, none of these are the fault of this patch, and might be revised in the new UI anyway.

Here's that same screen in D7 btw, for comparison:

- The block title isn't shown here. Therefore, how do I know if I want to display it or not? (the checkbox to do this is not pictured, because it annoyingly keeps scrolling below the toolbar where I can't reach it. This is not introduced by this patch, though.)
- We lost the ability to set a custom block title here. :( But yet we have this on all other blocks. A user is not going to understand why this block is special.
- Similarly, what does "Use default settings" mean in terms of number of things in a block? We should tell people what the value is. (e.g. "10 (default settings)")
- "Items per page" is a misnomer; it's "Items in block." Maybe "Items to show" or something more generic that could cover both cases?
- The labels "Show pages" / "Show content types" / etc. don't really make any sense. I think we mean "Show on pages" / "Show for content types / roles" etc. Once again, not the fault of this patch.
- Even though I checked "Show title," no title is in fact shown. This is because I had the block in the "header" region of the Bartik theme, which must do some magic to strip this out. I don't know that we can actually do anything about that, but figured I'd mention it because it caused me to think this was buggy until I tried "Sidebar first" instead.

----

So, to sum up:

- We need to put this block in a smarter category than "Views." Requiring people caring for the content of the site to understand what modules things come from is unrealistic.
- We need to expose the block title in order for the "Display title" checkbox to make any sense.
- We should also give the user the ability to override the title, for feature compatibility with D7.
- "Items per page" should be renamed to something that makes sense for blocks.
- Rather than saying "Default settings" (how do I know what the default settings are if I don't have access to Views UI?) it should say what the setting is and say (default) next to it.
- Follow-up issue for the bug about the machine name being auto-expanded.
- Follow-up issue to fix the &@^*#ing scrolling.
- We still have some more work to do on polishing the updated block UI. :) Waiting for updated specs on those though, which may address some of the other stuff identified here.

I realize most of this wasn't introduced by this patch, but was introduced in whatever the issue was that enabled overrides of views settings from this screen. I also realize that some of this was discussed before, but it's been a few months and I forget all the details. Since this is the first view to actually be converted to a block so feels like we ought to clean some of these views block-specific issues up here, too. I'm open to pushback on that, though.

Code-wise, this looks very yummy, and "7 files changed, 251 insertions, 768 deletions." is a wonderful diffstat. If we can just do a few final tweaks, I think this is good to go.

Followup #1: #2071019: Allow the block category for Views block displays to be edited

- Similarly, what does "Use default settings" mean in terms of number of things in a block? We should tell people what the value is. (e.g. "10 (default settings)")

Here is a follow up: #2072181: Change title in blocks form for items_per_page

- We should also give the user the ability to override the title, for feature compatibility with D7.

Talking about feature compability, it would be really important to get #2051917: Test contextual links feature of page displays in, so we ensure this feature in views. I know that there are plans to remove it.
Yeah this comment was 100% offtopic :p

Since this is the first view to actually be converted to a block so feels like we ought to clean some of these views block-specific issues up here, too. I'm open to pushback on that, though.

If you want to motivate people to work on things during the night, go the other way round, just saying. If people experiment with the new blocks provided by default, they will have ideas how to improve it.

Code-wise, this looks very yummy, and "7 files changed, 251 insertions, 768 deletions." is a wonderful diffstat. If we can just do a few final tweaks, I think this is good to go.

Well, it is kind of unfair because the configured view existed before, so some additions aren't really counted. We should look more in terms of code, which is for sure even better.

#2072181: Change title in blocks form for items_per_page is in.

1. +++ b/core/modules/comment/config/views.view.comments_recent.yml
   @@ -0,0 +1,222 @@
   +      sorts:
   +        created:
   

   sort by created is not needed according #1059608-7: Fix comment_get_recent ordering

Just to be sure: views adds both a node_access and comment_access tag, so there is a relationship to node in that view.

Thanks for providing the order criteria.

What's about comment_access and node_access - the second one was used in comment_get_recent()

If it is a comment view, the comment_access tag is automatically added.

Waiting for the fifth RTBC ... I don't think that the block issues should block to let this patch going in,
simply because once we have views in core, people will be motivated to continue to work. Believe me or not but rerolling patches for the sake of rerolling
is just nice from a academical point of view.

When block UI would be ready, just a small nitpick

+++ b/core/modules/comment/comment.install
@@ -10,7 +10,6 @@
-  variable_del('comment_block_count');

please add comment_update_N() function to properly get rid of it

Well, the non existing

+++ b/core/modules/comment/comment.install
@@ -392,6 +391,14 @@ function comment_update_8004() {
+ * Removes the comment_block_count variable.
+ */
+function comment_update_8005() {

fixing this small thing.
https://drupal.org/node/1354#functions

"These use a different syntax, because the documentation summary (first line) is displayed to someone running update.php to tell them which updates need to run. ...
Note that the first line should start with an imperative verb, so it will make sense to people running update.php."

----
also, there was a newline added by accident that I took out.

profiling was done in #76

adding some screenshots of the most recent patch.

I'll update the issue summary with them, and try and itemize the concerns and what needs new issues.

Updated issue summary.

adding tasks and issues, screenshots

reorganizing related, follow-up, etc

This is a straight reroll. No changes yet.

No wonder the tests fail ... we remove the page display in this patch and sort by CID instead of created.

And fixed.

#112: vdc-1938062-112.patch queued for re-testing.

sizing img

Ran into a related issue: #2135335: Adding a views block results in an empty block title on the admin screen

Nevertheless this patch works fine, see the screenshot.

I totally agree. Have you seen the comment which changed that?

Let's be consistent with the way how comment_get_recent() work at the moment.

Thank you to provide such an easy to adapt test.

Adapted DefaultViewRecentComments test to created descending order instead of changed.

Still applies, and looks good here. Tagging for an alpha target. The only thing we'll want to do is not commit the update_N function in favour of migrate module, but I can easily just remove that hunk from the patch on commit.

Still applies, and looks good here. Tagging for an alpha target. The only thing we'll want to do is not commit the update_N function in favour of migrate module, but I can easily just remove that hunk from the patch on commit.

It feels wrong to not add update functionality for things which cannot be done by migrate yet. If we don't add the update function here I fear that we really easy forget all those usecases. I did another review of #120 and everything was fine in there.

I'd like to point to Alex #1059608-37: Fix comment_get_recent ordering comment about order and do we really wanna to remove the fallback implementation in #2054993: Remove (untested, unused, broken) comment_get_recent()

@dawehner: Fair point, I guess it's not hurting anything.

@andypost: I believe this patch supports Alex's comment about comment ordering:

+      sorts:
+        created:
+          id: created
+          table: comment
+          field: created
+          relationship: none
+          group_type: group
+          admin_label: ''
+          order: DESC
+          exposed: false
+          expose:
+            label: ''
+          plugin_id: date
+        cid:
+          id: cid
+          table: comment
+          field: cid
+          relationship: none
+          group_type: group
+          admin_label: ''
+          order: DESC
+          exposed: false
+          expose:
+            label: ''
+          plugin_id: standard

So if an editor was is inclined to change the created date after the fact, the comment would moves to the top of the list, which is exactly how the node front page works, so IMO it is fine (that seems to also support Alex's stance). If I'm wrong about that, hey, it's a view, it's super easy to change the sort. :)

#2054993: Remove (untested, unused, broken) comment_get_recent() presumably should just move to a call to whatever views_build_view() is these days. But I guess we can talk over there. It seems like a 2/10 in the importance scale though, so I think there are far more important, especially beta-blocking things we can focus on than that, personally.

Just gave this another run-through and does what it's supposed to do, including the comment-publish-date-editing use case, soooo!

Committed and pushed to 8.x. YEAH. :D

This was committed in http://drupalcode.org/project/drupal.git/commit/f7e75db as part of #2020395: Convert "Who's new" block to a View. Unfortunately this resulted in fails in Drupal\comment\Tests\Views\DefaultViewRecentComments and Drupal\views\Tests\DefaultViewsTest. Going to press retest now I've reverted the commit.

120: vdc-1938062-120.patch queued for re-testing.

Failed... so needs work

120: vdc-1938062-120.patch queued for re-testing.

&!^@^!@*^&*!^@ Damn it. :( Twice in two days. :( Sorry once again. :(

Reuploading the patch to see whether it still fails (both fails do work locally).

132: vdc-1938062.patch queued for re-testing.

Hm. 4 fails each time. These at least look like they could conceivably be related, rather than random.

Well sure, I am just desperated because this just runs fine locally via the UI/run-tests.sh

Is it a PHP 5.3 vs. PHP 5.4 thing maybe?

This fixes run-tests.sh for me.

#2151427: Convert COMMENT_NOT_PUBLISHED & COMMENT_PUBLISHED to a constant on the comment interface

Oh nice, this remember myself alot when we worked on using the current user service.

Yeehaw!! Thanks, olli! :D

Committed and pushed to 8.x. Thanks!

As I mentioned in #104 the function was not fixed, so deprecated code is commited

This looks good

Committed a274193 and pushed to 8.x. Thanks!

Thanks for the quick turnaround - we don't need new calls to variable_del() at this stage :)