Updated: Comment #8

Problem/Motivation

Function comment_count_unpublished() is used only as title of local task UnapprovedComments (added in #2032309: Use local tasks derivatives to provide local tasks for views) and does not support a swapable comment storage, because could not be overriden

Proposed resolution

Implement CommentStorageInterface::getUnapprovedCount() that uses EntityQuery to count unapproved comments

API changes

remove comment_count_unpublished()
add CommentStorageInterface::getUnapprovedCount()

Original report by @andypost

Follow-up from #1978904: Convert comment_admin() to a Controller

Also usage of the function ('title callback' => 'comment_count_unpublished',) should be replaced with proper method in storageController

CommentFileSizeAuthor
#29 issue-2111357-29.patch4.18 KBmarcingy
#27 issue-2111357-27.patch4.13 KBmarcingy
#25 issue-2111357-25.patch4.1 KBmarcingy
#21 issue-2111357-21.patch4.08 KBmarcingy
#13 2111357-comment-approved-count-13.patch6.03 KBandypost
#13 interdiff.txt1.93 KBandypost
#11 2111357-comment-approved-count-11.patch6.09 KBandypost
#11 interdiff.txt1.99 KBandypost
#8 2111357-comment-approved-count-8.patch6.01 KBandypost
#7 2111357-comment-approved-count-7.patch3.14 KBandypost
#5 2111357-comment-approved-count-5.patch3.07 KBandypost
#1 drupal8.comment-module.2111357-1.patch3.5 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Needs review
FileSize
drupal8.comment-module.2111357-1.patch3.5 KB

Following #2068325: [META] Convert entity SQL queries to the Entity Query API we should use Entity Query API
The second level tabs seems should wait for #2102125: Big Local Task Conversion

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented 
+++ b/core/modules/comment/comment.module
@@ -241,9 +241,7 @@ function comment_menu_alter(&$items) {
 function comment_count_unpublished() {

Let's add @deprecate and add @todo in hook_menu for local task conversion.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
CreditAttribution: tstoeckler commented 
  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -199,4 +210,14 @@ public function getFieldUIPageTitle($field_name) {
+      ->condition('status', COMMENT_NOT_PUBLISHED, '=')

    So I guess this function should be called *Unpublished not *Unapproved?! 

  2. +++ b/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php
@@ -84,4 +84,12 @@ public function addBodyField($entity_type, $field_name);
+  public function getNumberOfUnapproved();

    Naming suggestion: getUnapprovedCount() or even countUnapproved()

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

Not sure that it makes sense to put that into manager class because:
- the conversion #2068331: Convert comment SQL queries to the Entity Query API moves this to storage controller
- maybe better to follow #2068331-32: Convert comment SQL queries to the Entity Query API and implement special data classes

Also related
#2054993: Remove (untested, unused, broken) comment_get_recent()
#1961548: [Policy, no patch] Decide how the module-specific services are made available as static methods (similar to \Drupal::$service())

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes
Issue tags: +API clean-up
FileSize
2111357-comment-approved-count-5.patch3.07 KB

Anther approach, the only place where this function used - local task (tab) in admin/content/comment
So I think better to get rid of it and move query in corresponding plugin class

PS: need approval from commiters

Status: Needs review » Needs work

The last submitted patch, 5: 2111357-comment-approved-count-5.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
2111357-comment-approved-count-7.patch3.14 KB

right patch

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes
FileSize
2111357-comment-approved-count-8.patch6.01 KB

Discussed with @msonnabaum and he suggest to implement the method in manager because this method uses storage details

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented 
+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -9,6 +9,7 @@
+use Drupal\Core\Entity\Query\QueryFactory;

@@ -31,16 +32,26 @@ class CommentManager implements CommentManagerInterface {
+   * @var \Drupal\Core\Entity\Query\QueryFactory
...
+   * @param \Drupal\Core\Entity\Query\QueryFactory $query_factory
...
+  public function __construct(FieldInfo $field_info, EntityManagerInterface $entity_manager, QueryFactory $query_factory) {

We should be typehinting the QueryFactoryInterface.
Other than that RTBC

areke’s picture

areke CreditAttribution: areke commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch should be re-rolled because it doesn't apply against the current HEAD. Also, @larowlan is right so the proposed change noted in #9 should be made.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
interdiff.txt1.99 KB
2111357-comment-approved-count-11.patch6.09 KB

I found that QueryFactoryInterface is not used in core and QueryFactory is different from interface?!

Anyway here is re-roll and interface related code

Status: Needs review » Needs work

The last submitted patch, 11: 2111357-comment-approved-count-11.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
interdiff.txt1.93 KB
2111357-comment-approved-count-13.patch6.03 KB

I don;t know why Drupal\Core\Entity\Query\QueryFactorydoes not implements QueryFactoryInterface so asked in #2019651-15: Add a QueryFactoryInterface for QueryFactory classes

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

13: 2111357-comment-approved-count-13.patch queued for re-testing.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed
Issue tags: +@deprecated

Hm. I guess this is OK since CommentManagerInterface is currently a grab-bag of methods that each do absolutely nothing to actually manage comments. :P~ FWIW though, I had envisioned this interface basically containing comment CRUD but instead it's a random assortment of methods with absolutely no rhyme or reason to why they exist grouped together that I can see.

I was going to commit this anyway, since it doesn't make what's already a bad situation tangibly worse, but apparently someone already did.

Status: Fixed » Closed (fixed)

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

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Closed (fixed) » Needs review

So this seems like it never actually got committed as the function still exists - I assume this will fail a retest

marcingy’s picture

marcingy CreditAttribution: marcingy commented

13: 2111357-comment-approved-count-13.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 13: 2111357-comment-approved-count-13.patch, failed testing.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Needs work » Needs review
FileSize
issue-2111357-21.patch4.08 KB

No interdiff as we now have a comment storage service, so couldn't get anything clean. Code is otherwise the same just in a new home.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Title: Get rid of comment_count_unpublished() in favor of CommentManager method » Get rid of comment_count_unpublished() in favor of CommentStorage method
marcingy’s picture

marcingy CreditAttribution: marcingy commented
Issue summary: View changes

Status: Needs review » Needs work

The last submitted patch, 21: issue-2111357-21.patch, failed testing.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Needs work » Needs review
FileSize
issue-2111357-25.patch4.1 KB

Take 2

Status: Needs review » Needs work

The last submitted patch, 25: issue-2111357-25.patch, failed testing.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Needs work » Needs review
FileSize
issue-2111357-27.patch4.13 KB

Ok bit better still getting local fails but my machine is also hating me at the moment.

Status: Needs review » Needs work

The last submitted patch, 27: issue-2111357-27.patch, failed testing.

marcingy’s picture

marcingy CreditAttribution: marcingy commented
Status: Needs work » Needs review
FileSize
issue-2111357-29.patch4.18 KB
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Reviewed & tested by the community

Needs this in #1498662: Refactor comment entity properties to multilingual

OTOH maybe reuse entity query here

slashrsm’s picture

slashrsm CreditAttribution: slashrsm commented
Parent issue: » #2068331: Convert comment SQL queries to the Entity Query API
webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed 81604ee on 8.x 
    Issue #2111357 by marcingy, andypost: Get rid of...

Status: Fixed » Closed (fixed)

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

roderik’s picture

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented

Hm, followup needs review: move this to CommentManager for consistency.
(see also https://www.drupal.org/node/2156089#comment-8273841 / https://www.drupal.org/node/2068331#comment-8917365)

edit: let's mention the issue # then :) #2318539: Move CommentStorage::getUnapprovedCount() to CommentManager