Updated #107

Title says it all.

Details

  • The recent comments block had a very simple setting that allowed to configure the number of comments to show. Because one can easily configure the number of items (and more) in Views, this is "dropped" from the Block configuration screen.
  • This patch reproduces an inconsistency that existed with the old block and should be fixed in a follow-up: The comments are sorted by their creation date, but their modification/updated date is displayed. Although the use-case of editing comments is not common (although, *cough* Drupal.org *cough*) the block may show, e.g. "sadf 2 hours ago", "asdf 1 second ago" and that (wrong) order. See #1938664: Recent comments block sorts by creation date but displays updated date for fixing this.
  • The Views configuration files are very inconsistent in terms of their explicit-ness. Some settings are left out of the default config and, thus, saving a view without changing anything will result in a non-empty "diff" of the resulting configuration. Explicitly declaring all keys would make the config files *very* large, but I think there is no way around that, ultimately. Either way, this is not for this issue to solve. Because I generated the view by clicking it together in the UI, it is explicit in some parts and not in others (just like all other default views currently). See #1938654: Export all properties of all views handlers and plugins for fixing this properly.

Discussion items

  • As shown in the screenshot, the "3 hours" part of "3 hours ago" is wrapped in an < em > element by the view, whereas previously this was not the case. Because the view simply uses the default "Time ago (with "ago" appended)" display plugin, if we want to retain the previous behavior, I think we should change the output of that plugin in general (i.e. for every view that uses it) instead of overriding the markup specifically for this view. I have no opinion on whether we want to remove the < em > or keep it, though.
  • The plugin name is kept as comments_recent as it was before. Because it belongs to comment module (without s) it might be better to name it comment_recent. I guess we should just leave this as is for now and wait for #1862600: Entity type names/IDs are not properly namespaced by owner (e.g., taxonomy.term vs. taxonomy_term) to happen.

Steps to test

  1. sudo rm -r sites; git checkout sites;
  2. git pull --rebase
  3. drush am 1938062
  4. drush -y si --account-pass=admin --db-url="mysql://root:root@localhost/d8-patch" --site-name=1938062
  5. get devel (git clone --branch 8.x-1.x http://git.drupal.org/project/devel.git)
  6. under Extend, enable the Devel generate module
  7. generate nodes and comments admin/config/development/generate/content be sure to change maximum number of comment per node (I used 4)
  8. ?

Oops. devel gets an error to. Can use filtered html instead of wysiwyg formatter to get around error when making comments.

Remaining Tasks

These are blocking this issue.

UI Changes

After screenshots

placing the block
num_items.png
editing the block
editing-recentcommentsblock.png
the block
block-recentcomments.png
hovering on the block, shows contextual edit. both the configure block and edit view links work.
hover-recentcomments.png

Follow-up issues

Related Issues

Files: 
CommentFileSizeAuthor
#144 comment_recent-fu.patch448 bytesandypost
PASSED: [[SimpleTest]]: [MySQL] 59,397 pass(es).
[ View ]
#141 vdc-1938062-141.patch32.45 KBolli
PASSED: [[SimpleTest]]: [MySQL] 59,471 pass(es).
[ View ]
#136 Screen Shot 2013-12-11 at 5.33.36 PM.png88.56 KBwebchick

Comments

tstoeckler’s picture

Status:Active» Needs review
StatusFileSize
new35.81 KB
new31.17 KB
new14.72 KB

Here we go. I also updated the issue summary. I have not updated the comment block tests, and thus this is not ready for testbot-primetime. It should be ready for (human) review, though.

See also the issue summary for questions/discussion items.

Screenshots of the old and new block attached. (They were made before ripping out the legacy code as this patch does.)

tstoeckler’s picture

Issue summary:View changes

Updated issue summary.

tstoeckler’s picture

StatusFileSize
new14.72 KB

Damn, the previous patch didn't pick up the deletion of the old config that views module had.

Even though I used that as a starting point for the "new" view, I could not get Git to pick this up as a rename... Don't know why.

tstoeckler’s picture

StatusFileSize
new5.73 KB
new20.04 KB
FAILED: [[SimpleTest]]: [MySQL] 52,728 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

All right, this passes the comment block tests. Let's see what else I broke. Go, testbot, go!

tstoeckler’s picture

Tagging.

tstoeckler’s picture

Regarding upgrade path tests. I'm currently not able to upgrade due to #1784312-84: Stop doing so much pre-kernel bootstrapping. Also we already have #1871700: Provide an upgrade path to the new Block architecture from Drupal 7 anyway, which means that this part of the upgrade path is missing anyway. I would therefore suggest handling that in a follow-up. But I'm obviously not an authority on that.

tstoeckler’s picture

Issue tags:+VDC
damiankloip’s picture

+++ 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!

larowlan’s picture

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.

damiankloip’s picture

Seems reasonable, that is a pretty big patch!

dawehner’s picture

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

+++ 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.

Status:Needs review» Needs work

The last submitted patch, 1938062-3.patch, failed testing.

tstoeckler’s picture

Replying to some of the comments. The other ones I will fix in the re-roll I'll do now.

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

I did re-arrange some of the lines. In the re-roll I will recreate the view from scratch and include it unmodified. I opened #1938654: Export all properties of all views handlers and plugins for the larger issue.

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

I just kept it as it currently is. I personally think it's fine that way, but if not, let's debate that in a follow-up.

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

Yes it felt strange to write that. Do you have concrete suggestions, though? I couldn't figure out a nicer way to write this.

I also don't mind at all this being postponed for now, this is pretty trivial to re-roll.

tstoeckler’s picture

Status:Needs work» Needs review
StatusFileSize
new8.71 KB
new24.52 KB
FAILED: [[SimpleTest]]: [MySQL] 52,945 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Opened #1938664: Recent comments block sorts by creation date but displays updated date for the issue mentioned in the OP.

Attached patch should pass tests. As promised I recreated the view from scratch. The only change that I had previously missed is that with this the administrative label of the created block is set to "Recent comments". Before it was using the default "Views: Recent comments". Also the view now contains an explicit relationship for the comment.nid <-> node.nid join, which I think was left out before.

Setting to needs review for the bot, if someone wants to set it to postponed per #8 that's fine. (I also wouldn't mind hashing this out further, though, even if we don't commit it now.)

Status:Needs review» Needs work

The last submitted patch, 1938062-13.patch, failed testing.

tstoeckler’s picture

Status:Needs work» Needs review
+++ b/core/modules/comment/config/views.view.comments_recent.yml
@@ -0,0 +1,218 @@
+human_name: sdsfda
...
+id: sdsfda

...and I'm an idiot. Sorry

tstoeckler’s picture

Status:Needs review» Needs work

Ahhhrrrgggg, Drupal.org is not my friend today...

tstoeckler’s picture

Status:Needs work» Needs review
Issue tags:-Needs upgrade path, -Needs upgrade path tests
StatusFileSize
new1.35 KB
new24.52 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1938062-17-3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Here's a re-roll. I also removed the UUID, which I had previously forgotten. I hope I didn't mess up anything this time.

Also removing the upgrade path tags for now.

Status:Needs review» Needs work

The last submitted patch, 1938062-17-3.patch, failed testing.

damiankloip’s picture

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.

tstoeckler’s picture

Status:Needs work» Needs review

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

Actually I think we do. Because the view provides a block type (but not a block instance) this only means that one can actually add a "Recent comments" block (just like before). By default no block instance is actually shown anywhere.

damiankloip’s picture

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

xjm’s picture

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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.

tstoeckler’s picture

Right, the only actual assertion that is removed is that the changing of the number of comments to show works. But since that is just a simple view setting it seems redundant to test that here.

Just a note to committers that we should check #1938664: Recent comments block sorts by creation date but displays updated date whether it conflicts with this. I plan to do that (and potentially see how hard it would be reroll that) in the next few days, but we shouldn't commit this if we break that, IMO. This one is pretty easy to re-roll.

xjm’s picture

+++ 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. :)

damiankloip’s picture

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

dawehner’s picture

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

dawehner’s picture

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.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 1938062-17-3.patch, failed testing.

xjm’s picture

YesCT’s picture

#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.)

YesCT’s picture

Issue summary:View changes

Added screenie and link to plugin namespacing issue.

YesCT’s picture

Issue summary:View changes

added related issue about multilingual filter of comments

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new15.74 KB
FAILED: [[SimpleTest]]: [MySQL] 53,259 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Just a rerole.

dawehner’s picture

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"?

Status:Needs review» Needs work

The last submitted patch, drupal-1938062-32.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new19.04 KB
FAILED: [[SimpleTest]]: [MySQL] 53,457 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

There we go.

Status:Needs review» Needs work

The last submitted patch, drupal-1938062-35.patch, failed testing.

xjm’s picture

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.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new24.01 KB
PASSED: [[SimpleTest]]: [MySQL] 53,378 pass(es).
[ View ]

Tobias removed this test from his patch.

tstoeckler’s picture

Assigned:tstoeckler» Unassigned

Right, as I explained in #24:

Right, the only actual assertion that is removed is that the changing of the number of comments to show works. But since that is just a simple view setting it seems redundant to test that here.

I just noticed that I'm still assigned, which is no longer accurate. I do plan do bring this home, in case @dawehner doesn't beat me to it ;-), but I certainly don't want to hold people off from helping out. Thanks also @dawehner for the reroll.

tim.plunkett’s picture

Issue tags:-Needs reroll
+++ 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!

dawehner’s picture

StatusFileSize
new24.15 KB
PASSED: [[SimpleTest]]: [MySQL] 53,681 pass(es).
[ View ]

There we go.

dawehner’s picture

StatusFileSize
new1.62 KB

ups.

jibran’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new117.11 KB
new116.95 KB
new240.77 KB
new236.28 KB
new2.8 KB
new2.76 KB

Done some testing. Looks great.
Comment block without patch.
rc_block_without-patch _20130328-015446.png
Comment block with patch.
rc_block_with-patch _20130328-015446.png
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.

xjm’s picture

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

YesCT’s picture

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?

YesCT’s picture

Issue summary:View changes

Updated issue summary. Added links to related issues.

YesCT’s picture

Issue summary:View changes

added follow-up section

YesCT’s picture

updated the issue summary with follow-up issues section.

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

YesCT’s picture

Issue summary:View changes

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

jibran’s picture

StatusFileSize
new58.69 KB
new67.28 KB
new44.78 KB

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
Bartik D7 Comments.png
In chrome D8 bartik shows comments like this
Bartik D8 Comments chrome.png
In firefox D8 bartik shows comments like this
Bartik D8 Comments firefox.png

jibran’s picture

xjm’s picture

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?

xjm’s picture

StatusFileSize
new33.58 KB

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".

block_library.png

YesCT’s picture

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)

xjm’s picture

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.)

xjm’s picture

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.

xjm’s picture

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. :)

dawehner’s picture

webchick’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs profiling

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

Copy/pasting "after" screenshot from above:

New configuration form for Recent Comments block

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.

xjm’s picture

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.

webchick’s picture

"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.

dawehner’s picture

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 different blocks (menu, views 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.

jibran’s picture

Status:Needs work» Reviewed & tested by the community

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.

xjm’s picture

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.

xjm’s picture

Status:Reviewed & tested by the community» Needs review

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. :)

damiankloip’s picture

+++ 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.

dawehner’s picture

StatusFileSize
new428 bytes
new24.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1938062-66.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Good point!

dawehner’s picture

Issue summary:View changes

with attempt at steps to reproduce

dawehner’s picture

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +Needs profiling, +VDC, +SprintWeekend2013

The last submitted patch, drupal-1938062-66.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new6.63 KB
new24.16 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal-1938062-69_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Rerolled.

xjm’s picture

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.

xjm’s picture

Issue summary:View changes

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

jibran’s picture

Status:Needs review» Needs work
Issue tags:+Needs manual testing, +Needs profiling, +VDC, +SprintWeekend2013

The last submitted patch, drupal-1938062-69.patch, failed testing.

damiankloip’s picture

Status:Needs work» Needs review
StatusFileSize
new24.14 KB
PASSED: [[SimpleTest]]: [MySQL] 55,757 pass(es).
[ View ]

Rerolled.

slashrsm’s picture

StatusFileSize
new27.62 KB
FAILED: [[SimpleTest]]: [MySQL] 57,970 pass(es), 0 fail(s), and 2 exception(s).
[ View ]

Reroll

slashrsm’s picture

StatusFileSize
new33.38 KB
FAILED: [[SimpleTest]]: [MySQL] 56,432 pass(es), 124 fail(s), and 20 exception(s).
[ View ]

Add files that I missed in previous patch.

slashrsm’s picture

StatusFileSize
new37.83 KB
new592.47 KB
new754.31 KB
new36.5 KB

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:

Before After
Exec. time 295 ms 422 ms
Memory usage ~19MB ~9MB
Function calls ~50,000 ~70,000
Query exec. time ~0.20s ~0.10s

-----

In-depth results

Before

before.png

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

after.png

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)

Status:Needs review» Needs work

The last submitted patch, 1938062-75.patch, failed testing.

andypost’s picture

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

andypost’s picture

andypost’s picture

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

dawehner’s picture

-function comment_get_recent($number = 10) {

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

xjm’s picture

@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.

dawehner’s picture

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.

xjm’s picture

@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...

dawehner’s picture

I am sorry about my comment.

xjm’s picture

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.

slashrsm’s picture

Status:Needs work» Needs review
StatusFileSize
new1.51 KB
new34.05 KB
PASSED: [[SimpleTest]]: [MySQL] 57,961 pass(es).
[ View ]

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

tim.plunkett’s picture

@@ -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?

dawehner’s picture

Status:Needs review» Needs work

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

slashrsm’s picture

StatusFileSize
new1.86 KB
new26.63 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
slashrsm’s picture

Status:Needs work» Needs review

Here we go.

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

tim.plunkett’s picture

StatusFileSize
new32.38 KB
PASSED: [[SimpleTest]]: [MySQL] 57,853 pass(es).
[ View ]

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.

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

Dries’s picture

Assigned:Unassigned» webchick

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.

Dries’s picture

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

tim.plunkett’s picture

Issue tags:-Needs manual testing
StatusFileSize
new32.39 KB
PASSED: [[SimpleTest]]: [MySQL] 58,354 pass(es).
[ View ]

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()).

webchick’s picture

Status:Reviewed & tested by the community» Needs work
StatusFileSize
new81.76 KB
new88.24 KB
new9.96 KB

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.

Block categorized as Views -- Recent comments

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.

Machine name field fully expanded in place blocks page.

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

D7 configure blocks screen

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

tim.plunkett’s picture

dawehner’s picture

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

tim.plunkett’s picture

andypost’s picture

Status:Needs review» Needs work
  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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new32.23 KB
PASSED: [[SimpleTest]]: [MySQL] 58,643 pass(es).
[ View ]

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.

andypost’s picture

Status:Needs work» Needs review

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

dawehner’s picture

StatusFileSize
new511 bytes
new32.53 KB
PASSED: [[SimpleTest]]: [MySQL] 59,145 pass(es).
[ View ]

Well, the non existing

YesCT’s picture

StatusFileSize
new598 bytes
new32.52 KB
PASSED: [[SimpleTest]]: [MySQL] 59,063 pass(es).
[ View ]
+++ 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.

YesCT’s picture

Issue tags:-Needs profiling
StatusFileSize
new36.22 KB
new108.45 KB
new116.79 KB
new246.44 KB

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.

YesCT’s picture

Issue summary:View changes

Updated issue summary.

YesCT’s picture

Issue summary:View changes

adding tasks and issues, screenshots

YesCT’s picture

Issue summary:View changes

reorganizing related, follow-up, etc

tim.plunkett’s picture

StatusFileSize
new9.27 KB
new27.28 KB
FAILED: [[SimpleTest]]: [MySQL] 58,465 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

This is a straight reroll. No changes yet.

Status:Needs review» Needs work

The last submitted patch, comment-1938062-108.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new2.55 KB
new29.01 KB
FAILED: [[SimpleTest]]: [MySQL] 58,460 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, vdc-1938062-110.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new29.68 KB
PASSED: [[SimpleTest]]: [MySQL] 58,986 pass(es).
[ View ]
new1.66 KB

And fixed.

Status:Needs review» Needs work
Issue tags:-VDC

The last submitted patch, vdc-1938062-112.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

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

dawehner’s picture

Issue summary:View changes

sizing img

dawehner’s picture

Issue summary:View changes
Status:Needs review» Reviewed & tested by the community
StatusFileSize
new29.75 KB
PASSED: [[SimpleTest]]: [MySQL] 60,360 pass(es).
[ View ]
new10.53 KB

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.

tstoeckler’s picture

Status:Reviewed & tested by the community» Needs work

Sorry, but #102 in this issue is wrong. We definitely need to sort by created. Otherwise, if an admin changes the created of a comment to be in the past it will not actually result in the comment to show up lower in the block. This was pointed out by @alexpott in #1059608-37: Fix comment_get_recent ordering and other comments in that issue. With migration this is can also be an issue. Although it might be most common to import starting with the lowest ID (and thus, the oldest comment) that is by no means a requirement. It's perfectly valid to do it the other way around, in which case this block would show the comments in the wrong order.

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new30.3 KB
FAILED: [[SimpleTest]]: [MySQL] 60,450 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.71 KB

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.

Status:Needs review» Needs work

The last submitted patch, 117: vdc-1938062.patch, failed testing.

tstoeckler’s picture

Well it was requested in #102 and then implemented in the following patch, I think.

pcambra’s picture

Status:Needs work» Needs review
StatusFileSize
new31.12 KB
FAILED: [[SimpleTest]]: [MySQL] 59,448 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
new1.53 KB

Adapted DefaultViewRecentComments test to created descending order instead of changed.

webchick’s picture

Status:Needs review» Reviewed & tested by the community
Issue tags:+VDC, +alpha target

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.

dawehner’s picture

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.

andypost’s picture

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()

webchick’s picture

Status:Reviewed & tested by the community» Fixed

@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

alexpott’s picture

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.

alexpott’s picture

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

The last submitted patch, 120: vdc-1938062-120.patch, failed testing.

alexpott’s picture

Status:Fixed» Needs work

Failed... so needs work

dawehner’s picture

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

The last submitted patch, 120: vdc-1938062-120.patch, failed testing.

webchick’s picture

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

dawehner’s picture

Status:Needs work» Needs review
StatusFileSize
new31.12 KB
FAILED: [[SimpleTest]]: [MySQL] 59,482 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

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

Status:Needs review» Needs work

The last submitted patch, 132: vdc-1938062.patch, failed testing.

dawehner’s picture

Status:Needs work» Needs review

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

Status:Needs review» Needs work

The last submitted patch, 132: vdc-1938062.patch, failed testing.

webchick’s picture

StatusFileSize
new88.56 KB

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

Recent comment fails.

dawehner’s picture

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

webchick’s picture

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

olli’s picture

Status:Needs work» Needs review
StatusFileSize
new31.96 KB
PASSED: [[SimpleTest]]: [MySQL] 59,512 pass(es).
[ View ]
new1.29 KB

This fixes run-tests.sh for me.

olli’s picture

olli’s picture

Status:Needs work» Needs review
StatusFileSize
new32.45 KB
PASSED: [[SimpleTest]]: [MySQL] 59,471 pass(es).
[ View ]
new1.85 KB
dawehner’s picture

Status:Needs review» Reviewed & tested by the community

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

webchick’s picture

Status:Reviewed & tested by the community» Fixed

Yeehaw!! Thanks, olli! :D

Committed and pushed to 8.x. Thanks!

andypost’s picture

Status:Fixed» Needs review
StatusFileSize
new448 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,397 pass(es).
[ View ]

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

dawehner’s picture

Status:Needs review» Reviewed & tested by the community

This looks good

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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 :)

Status:Fixed» Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.