Deprecate comment_new_page_count() and move it functionality into Comment storage controller

coming up. I wasn't planning on separating that out but totally works for me :)

Suppose proper tag

Actually, thanks for separating this. Because I had not thought of that in #2068331. comment_new_page_count() was not deprecated. Instead, I only split out the threaded new-page-nr into a new method on CommentStorage (because that was the only thing containing SQL.

This is the split-out & rerolled part of #2068331, if only for my own overview.

From there you could theoretically go in two directions:

• put everything in CommentStorage
• say "no it's actually nicer to move the now 'mode agnostic' comment_new_page_count() into CommentManager instead.

You said the former, and I'll assume the latter is indeed overkill. Feel free to set back to Needs Work if that changes somehow.

followup patch coming up.

...after I actually post the first patch...

At the risk of being confusing I'm going to try -only once- to fix the above test error separately, in 5a.

--

All storage-specific code (DB queries) go to storage. Non DB-specific logic can live in manager.

Indeed. But is it actually worth creating two functions out of comment_new_page_count()? I don't know; the non DB-specific logic isn't much code.
(See the 5a-9 interdiff for comment_new_page_count(): that piece is what could be moved into a function in manager.)

Below patch assumes "No".

More comments:

* This patch treads on #2262407: Deprecate comment_get_display_page/ordinal()'s turf because it also moves the COMMENT_MODE_FLAT definition into the interface. So one should be rerolled after the other gets committed (or we should wait RTBC'ing this one, to not annoy the core committers?)

* Some more misc class/argument renames in unrelated functions were necessary...

* I didn't want to have getNewCommentPageNr() start returning an array|NULL. It still returns a nice and consistent int. Even though it feels like unremoving boilerplate code to its callers...

* still a bit new in this forest of interfaces, I'm not sure if I should inject the EntityManager into the NodeNewComments class. I didn't know where to get it and saw another \Drupal:: being used inside the class... So I didn't.
(Same with the tests.)

obviously should have stolen more of slashrsm's code from #2262407: Deprecate comment_get_display_page/ordinal(). I think this is enough.

assertIdentical(5,5) fails. Obviously.

Is there a better arithmetic expression than this one? *shrug* I just want to see this tested now.

OK. (Tried to see whether unifying the lines above there to your change worked... but it didn't speak to me.)

Also changed my last fix - since (int) always rounds down.

Temporary glitch? It worked locally and I could step through the test being executed without seeing a hint of what would be the problem.

15: 2281475-15-comment-new-page-count.patch queued for re-testing.

Reroll after #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form.

If someone cares: (Non-unified) diff of patches attached as a record that the only 'changes to the changes' are $field_id -> $field_name stuff.

d'oh, incomplete reroll

srsly d'oh

Despite me taking 3 uploads *sigh*, all that happened since #21 was a s/field_id/field_name/ so, still RTBC

RTBC seconded

Just two small things. I'd change them myself but they happen enough that there's a chance I'd miss one and screw up testbot.

1. +++ b/core/modules/comment/comment.module
   @@ -257,80 +248,26 @@ function comment_permission() {
   - *   The first new comment entity.
   + *   The entity that comments belong to.
   
   +++ b/core/modules/comment/src/CommentStorageInterface.php
   @@ -16,27 +16,44 @@
   +   * @param \Drupal\Core\Entity\ContentEntityInterface $entity
   +   *   The entity that comments belong to.
   

   As long as we're fixing the grammar of the one below this,

   "The entity to which the comments belong."

2. +++ b/core/modules/comment/comment.module
   @@ -257,80 +248,26 @@ function comment_permission() {
   +  $page_nr = \Drupal::entityManager()->getStorage('comment')
   +    ->getNewCommentPageNr($num_comments, $new_comments, $entity, $field_name);
   ...
   +  return $page_nr ? array('page' => $page_nr) : NULL;
   
   +++ b/core/modules/comment/src/CommentStorage.php
   @@ -119,6 +119,64 @@ public function getMaxThreadPerThread(EntityInterface $comment) {
   +  public function getNewCommentPageNr($num_comments, $new_comments, ContentEntityInterface $entity, $field_name = 'comment') {
   
   +++ b/core/modules/comment/src/CommentStorageInterface.php
   @@ -16,27 +16,44 @@
   +  public function getNewCommentPageNr($num_comments, $new_comments, ContentEntityInterface $entity, $field_name = 'comment');
   

   Let's spell out "Number" in the method name and the holding variables ($page_nr => $page_number). We don't typically abbreviate words, and when we do we definitely don't do "num" on some places and "nr" in others. ;)

Looks good

Also summary should be updated about removal of constants, suppose this API change (maybe better to file new issue or change record)

1. +++ b/core/modules/comment/comment.module
   @@ -256,81 +247,27 @@ function comment_permission() {
   + *   Use \Drupal\comment\CommentStorageInterface::getNewCommentPageNr();
   

   should be fixed

2. +++ b/core/modules/comment/comment.module
   @@ -256,81 +247,27 @@ function comment_permission() {
   +  $page_nr = \Drupal::entityManager()->getStorage('comment')
   ...
   +  return $page_nr ? array('page' => $page_nr) : NULL;
   
   +++ b/core/modules/comment/src/CommentViewBuilder.php
   @@ -336,7 +336,10 @@ public static function attachNewCommentsLinkMetadata(array $element, array $cont
   +    $page_nr = \Drupal::entityManager()
   ...
   +    $query = $page_nr ? array('page' => $page_nr) : NULL;
   
   +++ b/core/modules/comment/src/Controller/CommentController.php
   @@ -290,7 +290,9 @@ public function renderNewCommentsNodeLinks(Request $request) {
   +      $page_nr = $this->entityManager()->getStorage('comment')
   ...
   +      $query = $page_nr ? array('page' => $page_nr) : NULL;
   
   +++ b/core/modules/comment/src/Plugin/views/field/NodeNewComments.php
   @@ -151,9 +151,11 @@ protected function renderLink($data, ResultRow $values) {
   +      $this->options['alter']['query'] = $page_nr ? array('page' => $page_nr) : NULL;
   
   +++ b/core/modules/forum/forum.module
   @@ -590,8 +590,11 @@ function template_preprocess_forums(&$variables) {
   +          $page_nr = \Drupal::entityManager()->getStorage('comment')
   ...
   +          $query = $page_nr ? array('page' => $page_nr) : NULL;
   

   $page_no at least

32: 2281475_32.patch queued for re-testing.

Rerolled (was trivial) and renamed variables according to #31-2/#34-2. (And renamed $total_comments_number to $total_comments, for consistency with $new_comments.)

As for #34-1: short change record created at https://www.drupal.org/node/2295487. I can see the sense in splitting this out to a separate issue (this constants change is actually in several RTBC issues now)... but since webchick already almost-committed this and other issues are waiting for this... I'd rather not split things further.

Reroll after #2097123: Deprecate comment_num_new() in favour of method on CommentManager.

Diff-between-diffs included to prove that there's nothing to re-review. (The change from 'comment_node_forum' to 'comment_forum' is not our business; #2097123-13: Deprecate comment_num_new() in favour of method on CommentManager at bottom says it's cool.

git ac https://www.drupal.org/files/issues/2281475-40.patch
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 30032  100 30032    0     0  26107      0  0:00:01  0:00:01 --:--:-- 26624
error: patch failed: core/modules/comment/src/Controller/CommentController.php:290
error: core/modules/comment/src/Controller/CommentController.php: patch does not apply
error: patch failed: core/modules/comment/src/Tests/CommentInterfaceTest.php:8
error: core/modules/comment/src/Tests/CommentInterfaceTest.php: patch does not apply
error: patch failed: core/modules/comment/src/Tests/CommentPreviewTest.php:7
error: core/modules/comment/src/Tests/CommentPreviewTest.php: patch does not apply
error: patch failed: core/modules/comment/src/Tests/CommentStatisticsTest.php:6
error: core/modules/comment/src/Tests/CommentStatisticsTest.php: patch does not apply

1. +++ b/core/modules/comment/comment.module
   @@ -244,81 +235,27 @@ function comment_permission() {
   + * @deprecated Deprecated since Drupal 8.x-dev, to be removed in Drupal 8.0.
   + *   Use \Drupal\comment\CommentStorageInterface::getNewCommentPageNumber();
   + *   Note this returns an integer instead of array|null.
   
   +++ b/core/modules/comment/src/CommentStorageInterface.php
   @@ -16,27 +16,44 @@
   +   * @return array|null
   +   *   An array "page=X" if the page number is greater than zero; NULL otherwise.
   

   I think the docblock on the interface is incorrect.

2. +++ b/core/modules/comment/src/Tests/CommentNodeAccessTest.php
   @@ -7,6 +7,8 @@
   +use Drupal\comment\Entity\Comment;
   

   Unnecessary use statement

back to rtbc

#2262407: Deprecate comment_get_display_page/ordinal() landed which has the same constant change in.

Rerolled, and much smaller now all constant changes are already in. The only 'misc' stuff remaining is some class/argument renames in unrelated functions which were necessary.

Interdiff is small change to the @return comment, to say its 0-based.

You'll see a reference to a second change notice (https://www.drupal.org/node/2299799) appear shortly. Will set its status to Draft again, because it will contain documentation for this and other uncommitted changes.

Looks good :)

Committed d8a4d54 and pushed to 8.x. Thanks!