Remove comment_get_thread() in favour of method on CommentStorage

Should this go on the storage controller because its related to storage?

Yes, you are right

There's an ORDER BY a certain expression in comment_get_thread(), to make the speed bearable. So it goes into swappable storage afaict.

This is going to be part of CommentStorageInterface (which still needs work) in #2068331: Convert comment SQL queries to the Entity Query API - so I guess this one can be closed.

Actually - #2068331: Convert comment SQL queries to the Entity Query API grew way too large, so I'll reverse strategy, and split out comment_get_thread() into here.

For your review. I reshuffled the code/arguments a bit to allow for the possibility of not applying PagerSelectExtender.

One possibly outstanding issue is the call to \Drupal::currentUser() that's inside CommentStorage::loadThread() now.
Could we just make that into an argument 'has admin rights' to pass into the method?
Maybe even sneak that as a bitmask into the $mode argument?

retitling

Reroll after PSR-4

Looks good. Just one minor comment.

+++ b/core/modules/comment/comment.module
@@ -974,8 +880,9 @@ function comment_node_update_index(EntityInterface $node, $langcode) {
+      if ($node->get($field_name)->status
+          && $comments = \Drupal::entityManager()->getStorage('comment')
+               ->loadThread($node, $field_name, $mode, $comments_per_page)) {
         comment_prepare_thread($comments);

Let's move $comments assignment out of condition.

Alright, no contest here.
(Spotted it but had no opinion so left as-is)

Looks good.

+++ b/core/modules/comment/comment.module
@@ -611,107 +611,13 @@ function comment_add(EntityInterface $entity, $field_name = 'comment', $pid = NU
+  $nodes = \Drupal::entityManager()->getStorage('comment')->loadThread($entity, $field_name, $mode, $comments_per_page, $pager_id);
+  return array_keys($nodes);

These are not nodes.

Hummm they're very much not.

Back to rtbc

"Wait, what was I thinking... This is not even true."

/me strips off part of an earlier addition to comments

crosspost, sorry larowlan discovered another minimal thing :)

Reroll for #2228763 (replaced field_id by field_name occurrences on 2 lines).

Still having the feeling that using a Drupal::currentUser() call inside CommentStorage will/should be shot down. See #5.

Here's a suggested patch: don't check user access inside loadThread() but require a extra flag to display unpublished comments. (This would open up the possibility for using 'non admin' displays for admin users too. And other flags like 'include to-be-moderated comments'.)

By the way, the patch doesn't look like it but I did check for other uses of COMMENT_MODE_FLAT / COMMENT_MODE_THREADED. I don't think there are any other places (yet) that should assume COMMENT_ACCESS_MODE_ADMIN could even be an input value.
(Except a possible future change to comment_new_page_count() - but not yet. I think there may be a bug in there that potentially returns the wrong page number for admin users, but getting into that is way out of scope for this issue.)

Oops, forgot to check my own loadThread() calls. Interdiff from 17 included.

Side effect of this patch: comment_node_update_index can never read unpublished comments by mistake. I guess that's a good thing.

20: 2156089-20-comment-get-thread.patch queued for re-testing.

The change to add COMMENT_ACCESS_MODE_ADMIN and not make comment_get_thread() determine if the current user has the administer comments permission needs a change notice.

Change record need to be created before or after this goes in?

Before and saved as a draft

Rerolled:
changed some comments for COMMENT_MODE_FLAT -> CommentManagerInterface::COMMENT_MODE_FLAT;
moved COMMENT_ACCESS_MODE_ADMIN into CommentManagerInterface because that's where the other constants are now.

This means that now, CommentStorage is dependent on CommentManagerInterface where it wasn't before... ('use' statement added during reroll).

Change notice will be added to https://www.drupal.org/node/2299799 soon. This means the change notice will document more than one not-yet-committed issue.

Fixing my own include...

#2281475: Deprecate comment_new_page_count() and move it functionality into Comment storage controller got in first.

Was very simple re-roll.

+++ b/core/modules/comment/src/CommentManagerInterface.php
@@ -27,6 +27,13 @@
   /**
+   * All comments are displayed, not only published ones (which is the default).
+   * Some functions accept this mode in addition to (OR-ed with) COMMENT_MODE
+   * constants, but it is not a 'setting' like the other modes.
+   */
+  const COMMENT_ACCESS_MODE_ADMIN = 2;

I'm not sure about this. Using bitwise operator to join this together with the threaded and flat mode is kinda weird because you can't join them together... why are we doing this? The service already has the current user service. If we need to do this then lets add an argument to loadThread to include admin comments if the user has access.

Done.

Meh... broken diff. Should be fine now. Interdiff from #35 is still valid.

Fixes 40

doh

Looks good

Interdiff #1: backed out references to COMMENT_ACCESS_MODE_ADMIN in code comments.

interdiff #2: sorry for causing the confusion guys, but I'm backing out the whole part referring to this flag/function argument. I promise to post the reverse interdiff-2.txt in a followup patch with attribution.

Answers:
#34/alexpott

why are we doing this? The service already has the current user service.

We didn't have the current user service at the time I added this into the patch, and I (still newbie) mistakenly thought we could not have it. (From the combination of an old andypost comment and me agreeing that including $currentuser is weird.)

It seems that assumption is wrong**. So now that we have the current user service, this part is indeed not necessary. And shouldn't be included in this issue.

#38/andypost

getDisplayOrdinal() method does not pass any additional parameters, and both functions should be unified.

You're right.
On top of that, I think getNewCommentPageNumber() should be unified with those functions, even though it isn't now. (In other words: since it doesn't do the same permission check, I think that Drupal contains a bug where a link to the wrong page number for new comments could be output for admin users.)

I'll take care of that in the followup issue, and extend the tests to... test my suspicion.

--

** I can understand it if $currentuser doesn't actually contain a 'static' user but a 'proxy object' that always gets the active user. That's still unclear to me, but w/e.

OK done.

Also needs follow-up to get rid of the function comment_view_multiple()

Doesn't ring a bell here. Is D8 deprecating comment_view_multiple() ( and comment_view() ) the same way we did with comment_load[_multiple]()? I'll open a follow-up when I understand.

Also: comment_prepare_thread(). Last month I didn't understand how to get rid of it but now it seems logical to me to move it into CommentViewBuilder::viewMultiple(). In the same follow-up. Or did you mean that?

Preempting testbot fail now #1498662: Refactor comment entity properties to multilingual is in

Thanks for fixing. (I will keep falling away for some days because of work.)

why new implementation uses default argument with value 0?

Per #5: comment_get_thread() always applied paging (PagerSelectExtender). While we are nailing down interfaces here,

• it seemed strange to me to make it impossible to not apply PagerSelectExtender ...even though I know of no current practical use for it...
• in this case 'not applying PagerSelectExtender' (default argument = 0) feels to me like the most sensible default.

Opinions on that, welcome. I do appreciate these detailed reviews because it makes me feel safer to actually experiment with changes like this :)

1. +++ b/core/modules/comment/comment.module
   @@ -456,109 +457,13 @@ function comment_node_view_alter(array &$build, EntityInterface $node, EntityVie
   -  if ($mode == CommentManagerInterface::COMMENT_MODE_FLAT) {
   -    $query->orderBy('c.cid', 'ASC');
   -  }
   -  else {
   -    // See comment above. Analysis reveals that this doesn't cost too
   -    // much. It scales much much better than having the whole comment
   -    // structure.
   -    $query->addExpression('SUBSTRING(c.thread, 1, (LENGTH(c.thread) - 1))', 'torder');
   -    $query->orderBy('torder', 'ASC');
   -  }
   
   +++ b/core/modules/comment/src/CommentStorage.php
   @@ -216,6 +217,124 @@ public function getChildCids(array $comments) {
   +    if (($mode & CommentManagerInterface::COMMENT_MODE_THREADED)
   +        == CommentManagerInterface::COMMENT_MODE_THREADED) {
   +      // See comment above. Analysis reveals that this doesn't cost too
   +      // much. It scales much much better than having the whole comment
   +      // structure.
   +      $query->addExpression('SUBSTRING(c.thread, 1, (LENGTH(c.thread) - 1))', 'torder');
   +      $query->orderBy('torder', 'ASC');
   +    }
   +    else {
   +      $query->orderBy('c.cid', 'ASC');
   +    }
   

   Why have we swapped this around? Plus we don't need the bitwise operations anymore.

2. +++ b/core/modules/comment/src/CommentStorageInterface.php
   @@ -85,6 +86,28 @@ public function getDisplayOrdinal(CommentInterface $comment, $comment_mode, $div
   +   *   Ordered array of comment objects.
   

   Lets add the fact that they are keyed by ID.

1. True
2. OK
3. I didn't know of #2205673: [META] Remove all @deprecated functions marked "remove before 8.0" until now. So we're not marking functions as deprecated, but removing them. *remove* Will adjust wording in https://www.drupal.org/node/2299799.

Re-roll.

Looks good

re #54.3 Well, normally we deprecate and then remove in separate patches - since this gives a chance for module developers to convert but I can not imagine this is a widely used function.

Committed 6e6f2c6 and pushed to 8.x. Thanks!

+1 for the British spelling of favour in the issue title :)