Deprecate comment_get_display_page/ordinal() [#2262407]

(
So yeah. I wandered into here via DDD Szeged sprinting, drupal8multilingual.org focus issues, which has #2068325: [META] Convert entity SQL queries to the Entity Query API.
I took #2068331: Convert comment SQL queries to the Entity Query API , but until yesterday I hadn't really seen that that one coincides with child issues of #2096923: [meta] Clean-up CommentManager service and deprecate procedural helper functions.
)

I'm trying to split #2068331 out into more manageable chunks. This chunk covers CommentManager as well as CommentStorage... Maybe...

Questions:

Do we want to deprecate comment_get_display_page() and comment_get_display_ordinal() too?

comment_get_display_page() has 2 callers (CommentForm::save() & CommentController::permaLink())

comment_get_display_ordinal() only gets called by comment_get_display_page(). Thing is, it must be in CommentStorage.

/**
 * Gets the display ordinal for a comment, starting from 0. [...]
 */
function comment_get_display_ordinal($cid, $instance) {

  [[ SQL gunk that belongs in CommentStorage ]]

  return $query->execute()->fetchField();
}

/**
 * Returns the page number for a comment. [...]
 */
function comment_get_display_page($cid, $instance) {
  $ordinal = comment_get_display_ordinal($cid, $instance);
  $comments_per_page = $instance->getSetting('per_page');
  return floor($ordinal / $comments_per_page);
}

If comment_get_display_ordinal() moves to CommentStorage, what do we do with comment_get_display_page()?

Stick it in CommentManager?
Delete it altogether and make getDisplayOrdinal have a third argument called 'int $divisor'? That could default to 1. If you pass it the number-of-comments-per-page, you get the output of what is now comment_get_display_page()...

Do we want to pass the new methods a Comment object instead of a $cid, as the first argument?

My first stab at this, split off from #2068331, kept $cid intact because comment_get_display_page() wants to give it a $cid. But merging comment_get_display_page() into comment_get_display_ordinal() could make it easier to replace the argument with a Comment object...

Comment	File	Size	Author
#20	interdiff.txt	2 KB	slashrsm
#20	2262407_20.patch	24.04 KB	slashrsm
#20
#17	2262407_17.patch	24.42 KB	slashrsm
#17
#13	interdiff.txt	938 bytes	slashrsm
#13	2262407_13.patch	24.44 KB	slashrsm
#13
#11	interdiff.txt	511 bytes	slashrsm
#11	2262407_11.patch	24.44 KB	slashrsm
#11
#9	2262407_9.patch	24.17 KB	slashrsm
#9
#9	interdiff.txt	18.73 KB	slashrsm
#7	interdiff.txt	616 bytes	slashrsm
#7	2262407_7.patch	8.47 KB	slashrsm
#7
#6	2262407_6.patch	8.88 KB	slashrsm
#6
#4	interdiff-2262407-0-4.txt	8.29 KB	roderik
#4	2262407-comment-display-ordinal-4.patch	8.85 KB	roderik
#4
	comment-display-ordinal.patch	4.73 KB	roderik

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

roderik

Dutch

Amsterdam,NL / Budapest,HU

CreditAttribution: roderik commented 9 May 2014 at 00:46

Status:

Active

» Needs review

Comment #2

roderik

Dutch

Amsterdam,NL / Budapest,HU

CreditAttribution: roderik commented 9 May 2014 at 00:53

Issue summary:

View changes

Comment #3

larowlan

🇦🇺🏝.au GMT+10

CreditAttribution: larowlan commented 9 May 2014 at 20:39

+1 to merging

Comment #4

roderik

Dutch

Amsterdam,NL / Budapest,HU

CreditAttribution: roderik commented 10 May 2014 at 01:05

File	Size
2262407-comment-display-ordinal-4.patch	8.85 KB

interdiff-2262407-0-4.txt	8.29 KB

1 file was hidden/shown/deleted

File	Size
comment-display-ordinal.patch	4.73 KB

Cool.

Updated patch.
(I didn't heed the 80 column boundary where surrounding code didn't.
Plus added a random coment change because I was just disciplined on https://drupal.org/node/1354#order.)

Comment #5

slashrsm CreditAttribution: slashrsm commented 5 June 2014 at 23:12

Assigned:

roderik

» slashrsm

Comment #6

slashrsm CreditAttribution: slashrsm commented 5 June 2014 at 23:22

File	Size
2262407_6.patch	8.88 KB

Reroll.

Comment #7

slashrsm CreditAttribution: slashrsm commented 5 June 2014 at 23:25

File	Size
2262407_7.patch	8.47 KB

interdiff.txt	616 bytes

Merge garbage removed.

Comment #8

marcingy CreditAttribution: marcingy commented 6 June 2014 at 16:38

Status:

Needs review

» Needs work

+  $comment = \Drupal::entityManager()->getStorage('comment')->load($cid);
+  return $comment ?
+    \Drupal::entityManager()->getStorage('comment')
+      ->getDisplayOrdinal($comment, $field_definition->getSetting('default_mode'))
+    : 0;

Why not

$comment_manager = \Drupal::entityManager()->getStorage('comment');
$comment = $comment_manager->load($cid);
....

Just means the return can be tidied up.

Similar in comment_get_display_page.

if (!user_access('administer comments')) {
+      $query->condition('c1.status', CommentInterface::PUBLISHED);
+    }

This maybe scope creep but user_access is deprecated and we should be using the user service. (\Drupal\Core\Session\AccountInterface::hasPermission().)

+    if ($comment_mode == COMMENT_MODE_FLAT) {

Should COMMENT_MODE_FLAT become a defines for the interface?

Comment #9

slashrsm CreditAttribution: slashrsm commented 6 June 2014 at 17:46

Status:

Needs work

» Needs review

File	Size
interdiff.txt	18.73 KB
2262407_9.patch	24.17 KB

Here we go.

Comment #10

6 June 2014 at 17:47

Status:

Needs review

» Needs work

The last submitted patch, 9: 2262407_9.patch, failed testing.

Comment #11

slashrsm CreditAttribution: slashrsm commented 6 June 2014 at 18:55

Status:

Needs work

» Needs review

File	Size
2262407_11.patch	24.44 KB

interdiff.txt	511 bytes

Meh!

Comment #12

Primsi CreditAttribution: Primsi commented 20 June 2014 at 13:12

Status:

Needs review

» Needs work

+++ b/core/modules/comment/src/CommentStorage.php
@@ -119,6 +131,38 @@ public function getMaxThreadPerThread(EntityInterface $comment) {
+    // Count how many comments (c1) are before $comment (c2) in display order. This

Comment longer than 80 chars. Apart from that looks ok to me.

Comment #13

slashrsm CreditAttribution: slashrsm commented 20 June 2014 at 16:44

Status:

Needs work

» Needs review

File	Size
2262407_13.patch	24.44 KB

interdiff.txt	938 bytes

Fixed and re-rolled.

Comment #14

marcingy CreditAttribution: marcingy commented 20 June 2014 at 18:33

Status:

Needs review

» Reviewed & tested by the community

Looks good

Comment #15

marcingy CreditAttribution: marcingy commented 27 June 2014 at 02:11

13: 2262407_13.patch queued for re-testing.

Comment #16

30 June 2014 at 04:37

Status:

Reviewed & tested by the community

» Needs work

The last submitted patch, 13: 2262407_13.patch, failed testing.

Comment #17

slashrsm CreditAttribution: slashrsm commented 30 June 2014 at 10:19

Status:

Needs work

» Needs review

File	Size
2262407_17.patch	24.42 KB

Reroll.

Comment #18

slashrsm CreditAttribution: slashrsm commented 30 June 2014 at 12:47

Status:

Needs review

» Reviewed & tested by the community

Comment #19

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 7 July 2014 at 11:23

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

This needs a change notice to announce the deprecation and the moving of the constants.

diff --git a/core/modules/comment/src/Tests/CommentNodeAccessTest.php b/core/modules/comment/src/Tests/CommentNodeAccessTest.php
index 7e4525c..9882371 100644
--- a/core/modules/comment/src/Tests/CommentNodeAccessTest.php
+++ b/core/modules/comment/src/Tests/CommentNodeAccessTest.php
@@ -8,7 +8,6 @@
 namespace Drupal\comment\Tests;
 
 use Drupal\comment\CommentManagerInterface;
-use Drupal\comment\Entity\Comment;
 use Drupal\simpletest\WebTestBase;
 
 /**
diff --git a/core/modules/comment/src/Tests/CommentPagerTest.php b/core/modules/comment/src/Tests/CommentPagerTest.php
index 295c5e7..a880a2b 100644
--- a/core/modules/comment/src/Tests/CommentPagerTest.php
+++ b/core/modules/comment/src/Tests/CommentPagerTest.php
@@ -6,6 +6,7 @@
  */
 
 namespace Drupal\comment\Tests;
+
 use Drupal\comment\CommentManagerInterface;
 
 /**
diff --git a/core/modules/comment/src/Tests/CommentStatisticsTest.php b/core/modules/comment/src/Tests/CommentStatisticsTest.php
index 7172298..8a9f321 100644
--- a/core/modules/comment/src/Tests/CommentStatisticsTest.php
+++ b/core/modules/comment/src/Tests/CommentStatisticsTest.php
@@ -6,8 +6,8 @@
  */
 
 namespace Drupal\comment\Tests;
+
 use Drupal\comment\CommentManagerInterface;
-use Drupal\comment\Entity\Comment;
 
 /**
  * Tests the comment module administrative and end-user-facing interfaces.

Unnecessary use statements and incorrect spacing.

Comment #20

slashrsm CreditAttribution: slashrsm commented 8 July 2014 at 09:02

Status:

Needs work

» Needs review

File	Size
2262407_20.patch	24.04 KB

interdiff.txt	2 KB

Reroll and fixed.

Comment #21

slashrsm CreditAttribution: slashrsm commented 8 July 2014 at 19:07

Change record draft: https://www.drupal.org/node/2299799

Comment #22

roderik

Dutch

Amsterdam,NL / Budapest,HU

CreditAttribution: roderik commented 8 July 2014 at 21:32

Status:

Needs review

» Reviewed & tested by the community

@ #19/alexpott: Change record for moving of constants is at https://www.drupal.org/node/2295487

(This was made for #2281475-37: Deprecate comment_new_page_count() and move it functionality into Comment storage controller. Either #228147 or this issue will need a reroll when the other gets committed. I know this isn't ideal but it's better than splitting the constants move out to a separate issue at this point in time...)

Many functions in comment.module have been marked @deprecated but are not 'record'ed yet. I think it makes sense (to the reader) to have all these in one change record. (Also all of the issues have seen RTBC already, so I believe the downside for committers to have these in one change record is minimal.)
So I'll go edit slashrsm's change record to add those, and you can tell me to revert/split the text if it must be so.

Meanwhile, this is back to RTBC. (I believe the last interdiff.)

Comment #23

alexpott

he/they

English

🇪🇺🌍

CreditAttribution: alexpott commented 8 July 2014 at 22:31

Status:	Reviewed & tested by the community	» Fixed
Issue tags:	-Needs change record

@roderik yep I agree that once change record for all the removed functions in comment.module is preferable. But each issue needs to be added to this change record - so there is no downside for committers - we just need to see the change record referenced.

Committed 47d6bc0 and pushed to 8.x. Thanks!

diff --git a/core/modules/comment/src/CommentStorageInterface.php b/core/modules/comment/src/CommentStorageInterface.php
index dc6e419..c1e0e72 100644
--- a/core/modules/comment/src/CommentStorageInterface.php
+++ b/core/modules/comment/src/CommentStorageInterface.php
@@ -44,7 +44,8 @@ public function getMaxThreadPerThread(EntityInterface $comment);
    * @param \Drupal\comment\CommentInterface $comment
    *   The comment to use as a reference point.
    * @param int $comment_mode
-   *   Comment mode (COMMENT_MODE_FLAT or COMMENT_MODE_THREADED).
+   *   Comment mode (CommentManagerInterface::COMMENT_MODE_FLAT or
+   *   CommentManagerInterface::COMMENT_MODE_THREADED).
    * @param int $divisor
    *   Defaults to 1, which returns the display ordinal for a comment. If the
    *   number of comments per page is provided, the returned value will be the

Fixed this on commit - the constants COMMENT_MODE_FLAT and COMMENT_MODE_THREADED no longer exist.

Comment #24

8 July 2014 at 22:36

alexpott committed 47d6bc0 on 8.x

Issue #2262407 by slashrsm, roderik: Deprecate comment_get_display_page/...

Comment #25

andypost

he/him

Russian

CreditAttribution: andypost commented 9 July 2014 at 17:27

14 files were hidden/shown/deleted

File	Size
2262407-comment-display-ordinal-4.patch	8.85 KB

interdiff-2262407-0-4.txt	8.29 KB
2262407_6.patch	8.88 KB

2262407_7.patch	8.47 KB

interdiff.txt	616 bytes
interdiff.txt	18.73 KB
2262407_9.patch	24.17 KB

2262407_11.patch	24.44 KB

interdiff.txt	511 bytes
2262407_13.patch	24.44 KB

interdiff.txt	938 bytes
2262407_17.patch	24.42 KB

2262407_20.patch	24.04 KB

interdiff.txt	2 KB

+++ b/core/modules/comment/comment.module
@@ -1012,35 +1003,16 @@ function comment_num_new($entity_id, $entity_type, $field_name = NULL, $timestam
+  $comment_storage = \Drupal::entityManager()->getStorage('comment');
...
+  $comment = \Drupal::entityManager()->getStorage('comment')->load($cid);
+  return $comment ? $comment_storage->getDisplayOrdinal($comment, $default_mode) : 0;

needs follow-up to use $comment_storage consistently