Updated: Comment #0

Problem/Motivation

#731724: Convert comment settings into a field to make them work with CMI and non-node entities Will add a new CommentManager service. comment_num_new() belongs on that service.

Proposed resolution

Move to a method and deprecate the original function.

Remaining tasks

Wait for #731724: Convert comment settings into a field to make them work with CMI and non-node entities
Patch

User interface changes

None

API changes

comment_num_new() is deprecated.

Follow-up from #731724: Convert comment settings into a field to make them work with CMI and non-node entities.
#2090139: Tests for: Warning: Illegal string offset 'callable' in rdf_rdfa_attributes() (line 181 of core/modules/rdf/rdf.module).

Files: 
CommentFileSizeAuthor
#104 interdiff.2097123.100.104.txt1.56 KBYesCT
#104 2097123-comment_num_new-104.patch18.78 KBYesCT
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,891 pass(es).
[ View ]
#100 2097123-comment_num_new-100.patch18.75 KBandypost
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,974 pass(es).
[ View ]
#100 interdiff.txt738 bytesandypost
#99 1.patch18.31 KBandypost
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,729 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

Comments

andypost’s picture

Priority:Critical» Major
Status:Postponed» Active
das-peter’s picture

StatusFileSize
new8.1 KB
FAILED: [[SimpleTest]]: [MySQL] 59,237 pass(es), 5 fail(s), and 86 exception(s).
[ View ]

First attempt.
There are still calls to procedural code in the method, is it in the scope of this issue to change this as well?
And I don't know if $user as parameter is the right approach.
So the whole thing doesn't feel sound yet.

andypost’s picture

Status:Active» Needs review

Let's test the patch

  1. +++ b/core/modules/comment/comment.module
    @@ -1250,66 +1250,6 @@ function comment_load($cid, $reset = FALSE) {
    -function comment_num_new($entity_id, $entity_type, $field_name = NULL, $timestamp = 0) {

    +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +242,65 @@ public function getFieldUIPageTitle($field_name) {
    +  public function countNewComments(UserInterface $user, $entity_id, $entity_type, $field_name = NULL, $timestamp = 0) {

    Please @deprecated

  2. +++ b/core/modules/comment/comment.module
    @@ -1250,66 +1250,6 @@ function comment_load($cid, $reset = FALSE) {
    -function comment_num_new($entity_id, $entity_type, $field_name = NULL, $timestamp = 0) {

    +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +242,65 @@ public function getFieldUIPageTitle($field_name) {
    +   * @param \Drupal\user\UserInterface $user
    +   *   The user for which to count the new comments.
    ...
    +  public function countNewComments(UserInterface $user, $entity_id, $entity_type, $field_name = NULL, $timestamp = 0) {

    $user makes no sense as first argument

Status:Needs review» Needs work

The last submitted patch, d8-deprecate-comment_num_new-2097123-2.patch, failed testing.

das-peter’s picture

Status:Needs work» Needs review
StatusFileSize
new7.67 KB
FAILED: [[SimpleTest]]: [MySQL] 59,436 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new5.77 KB

Addressed feedback in #3.

  • Restored legacy function and added @deprecated
  • Removed $user parameter, really doesn't make sense as long as we don't have a way to pass it on.

Btw. I don't get how ForumManager::numberNew() ever worked, it looks like this method called comment_num_new() always with wrong parameters.

Status:Needs review» Needs work

The last submitted patch, d8-deprecate-comment_num_new-2097123-5.patch, failed testing.

andypost’s picture

@das-peter Awesome finding about forum!
Not sure why tokens fail, will check tonight

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
@@ -312,7 +312,7 @@ protected function getTopicOrder($sortby) {
   protected function numberNew($nid, $timestamp) {
-    return comment_num_new($nid, $timestamp);
+    return \Drupal::service('comment.manager')->countNewComments($nid, 'node', NULL, $timestamp);

yep, seems we have no test for that.
Also forum adds own comment field so it makes sense to use the name here

andypost’s picture

The last submitted patch, d8-deprecate-comment_num_new-2097123-5.patch, failed testing.

andypost’s picture

Title:Deprecate comment_num_new() in favour of method on CommentManager» Use \Drupal instead of Drupal to make IDEs and static code analyse tools happy
Component:comment.module» other
Category:task» bug
Priority:Major» Normal
andypost’s picture

Title:Use \Drupal instead of Drupal to make IDEs and static code analyse tools happy» Deprecate comment_num_new() in favour of method on CommentManager
Component:other» comment.module
Category:bug» task
Priority:Normal» Major
andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new9.45 KB
FAILED: [[SimpleTest]]: [MySQL] 59,331 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new3.04 KB

Found 2 occurrences of broken forum field usage, now tests should be fine

larowlan’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +   * @param int $entity_id

    I think this should take an array of entity ids, like we did for history_read_multiple(). There are several instances where we're calling this method in a foreach loop which could be more performant in a single query.

  2. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +    $user = \Drupal::currentUser();

    This should be injected

  3. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +    if ($user->isAuthenticated() && \Drupal::moduleHandler()->moduleExists('history')) {

    So should module handler

  4. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +          $timestamp = history_read($entity_id);

    Do we have a history service yet? If not we should put history_read behind a method so we have ability to unit-test

  5. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +      $query = db_select('comment', 'c');

    We should inject the database connection

  6. +++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.php
    @@ -69,7 +69,7 @@ function testCommentInstallAfterContentModule() {
    +    if ($field = $this->container->get('field.info')->getField('node', 'comment_forum')) {

    +++ b/core/modules/forum/forum.module
    @@ -791,7 +791,7 @@ function template_preprocess_forum_topic_list(&$variables) {
    +        $variables['topics'][$id]->new_url = url('node/' . $topic->id(), array('query' => comment_new_page_count($topic->comment_count, $topic->new_replies, $topic, 'comment_forum'), 'fragment' => 'new'));

    Good catch. So far this is the only issue from the 450kb comment-field patch. I can live with that.

larowlan’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +   * Returns the number of new comments for an user and the specified entity.

    'for the current user' might push it past 80 chars, so 'for a user'

  2. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +241,64 @@ public function getFieldUIPageTitle($field_name) {
    +   * @todo Help, this uses procedural code! We've to get rid of that.

    Remove this line

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-12.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new13.4 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
new6.57 KB

Patch fixes injections, multiple IDs needs a bit more work,

Also found Warning: call_user_func() expects parameter 1 to be a valid callback, function 'd' not found or invalid function name in rdf_rdfa_attributes() (line 183 of core/modules/rdf/rdf.module). but that's not patch related, thrown on forum page

+ $user = \Drupal::currentUser();
This should be injected

Done, but still not sure about injection of common services.

Do we have a history service yet?

No, that's for @todo Remove once http://drupal.org/node/1029708 lands.

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-16.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new14.21 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

let's debug bot

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-17-test.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new13.96 KB
FAILED: [[SimpleTest]]: [MySQL] 59,192 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
new3.42 KB

Current user is request scope limited so we can't pass it to services

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-20.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new15.48 KB
FAILED: [[SimpleTest]]: [MySQL] 58,267 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new2.33 KB

Proper fix for tests:
1) ForumManagerTest test needs to mock comment manager
2) CommentTokenReplaceTest should check new comments for user that never read comments

Status:Needs review» Needs work

The last submitted patch, drupal8.comment-module.2097123-22.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new16.12 KB
PASSED: [[SimpleTest]]: [MySQL] 58,674 pass(es).
[ View ]
new1.82 KB
larowlan’s picture

Disregard the comment regarding fetching in a single query (aka taking array of ids), this can't be done because the $timestamp is different in each case.

+++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
@@ -330,7 +330,7 @@ public function renderNewCommentsNodeLinks(Request $request) {
+      $new = \Drupal::service('comment.manager')->countNewComments($node->id(), 'node');

Can this be injected?

Other than that looks good to go

andypost’s picture

StatusFileSize
new16.66 KB
FAILED: [[SimpleTest]]: [MySQL] 58,317 pass(es), 22 fail(s), and 9 exception(s).
[ View ]
new5.78 KB
andypost’s picture

StatusFileSize
new16.66 KB
PASSED: [[SimpleTest]]: [MySQL] 58,341 pass(es).
[ View ]
new636 bytes

And last hunk

dawehner’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -241,4 +260,65 @@ public function getFieldUIPageTitle($field_name) {
    +  public function getCountNewComments($entity_id, $entity_type, $field_name = NULL, $timestamp = 0, AccountInterface $account = NULL) {
    +    if (!isset($account)) {
    +      $account = \Drupal::currentUser();
    +    }

    Is there a specific reason why we don't inject the currentUser?

  2. +++ b/core/modules/comment/lib/Drupal/comment/Controller/CommentController.php
    @@ -330,7 +330,7 @@ public function renderNewCommentsNodeLinks(Request $request) {
    -      $new = comment_num_new($node->id(), 'node');
    +      $new = $this->commentManager->getCountNewComments($node->id(), 'node');

    It would be maybe helpful to have swap the order of the arguments so we kind of have similar signature as with things like entity_load in D7.

andypost’s picture

StatusFileSize
new15.72 KB
PASSED: [[SimpleTest]]: [MySQL] 58,765 pass(es).
[ View ]
new4.87 KB

Order of arguments makes a lot of sense!

Is there a specific reason why we don't inject the currentUser?

We pass $account as argument so calling code should pass it right, it makes no sense to make manager depends on service that will be limited with request scope #2102027: Add back the request scope to the current user service

dawehner’s picture

We pass $account as argument so calling code should pass it right, it makes no sense to make manager depends on service that will be limited with request scope #2102027: Add back the request scope to the current user service

Well, then we maybe should require the account variable?

pfrenssen’s picture

StatusFileSize
new15.71 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.comment-module.2097123-31.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I don't think requiring the account is a good idea DX-wise, since most of the time this is called it will be for the current account, and it would put additional burden on the developer to always have to retrieve the current user.

I couldn't make sense of the current user / request scope issue, but looking at the bootstrap, the request is initialized right after the DRUPAL_BOOTSTRAP_CODE phase, which means it is the very first thing happening after the system becomes usable. I can't imagine many cases where it would be necessary to retrieve a comment count earlier than this bootstrap phase :) Trying to do so will throw fatal errors anyway.

Did some work on this:

pfrenssen’s picture

StatusFileSize
new4.35 KB

Here's the interdiff.

andypost’s picture

Probably we should implement this method in storage controller or postpone on #2068331: Convert comment SQL queries to the Entity Query API

tim.plunkett’s picture

Can someone explain why this is a major beta blocker? Also, postponing on #2068331: Convert comment SQL queries to the Entity Query API sounds reasonable.

tim.plunkett’s picture

Issue summary:View changes

Updated issue summary.

catch’s picture

Issue summary:View changes
Issue tags:-beta blocker

Yes I don't think this is a beta blocker, not sure it should block release either since there's no API change necessary.

mgifford’s picture

Status:Needs review» Needs work

The last submitted patch, 31: drupal8.comment-module.2097123-31.patch, failed testing.

andypost’s picture

+++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
@@ -199,4 +222,64 @@ public function getFieldUIPageTitle($field_name) {
+      $query = $this->connection->select('comment', 'c');
+      $query->addExpression('COUNT(cid)');

this query should be converted to EntityQuery

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new16.34 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2097123-newComments-39.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll with EntityQuery and @todo reference for #2081585: Introduce HistoryRepository service (no reason to inject module handler in favor of history manager

andypost’s picture

39: 2097123-newComments-39.patch queued for re-testing.

Status:Needs review» Needs work

The last submitted patch, 39: 2097123-newComments-39.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new16.13 KB
PASSED: [[SimpleTest]]: [MySQL] 59,389 pass(es).
[ View ]

re-roll

larowlan’s picture

+++ b/core/modules/forum/tests/Drupal/forum/Tests/ForumManagerTest.php
@@ -9,6 +9,11 @@
+// @todo Remove once the constants are replaced with constants on classes.

Can we get a url added for this todo?

Other than that, RTBC

andypost’s picture

StatusFileSize
new549 bytes
new16.17 KB
PASSED: [[SimpleTest]]: [MySQL] 59,366 pass(es).
[ View ]
larowlan’s picture

Status:Needs review» Reviewed & tested by the community

Ready, thanks!

xjm’s picture

Priority:Major» Normal

Agreed that this is not major.

catch’s picture

Status:Reviewed & tested by the community» Needs review
+++ b/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php
@@ -86,4 +88,24 @@ public function addBodyField($entity_type, $field_name);
+  public function getCountNewComments($entity_type, $entity_id, $field_name = NULL, $timestamp = 0, AccountInterface $account = NULL);

Sorry one more question here. Why not just pass the $entity object instead of type/id?

andypost’s picture

StatusFileSize
new7.79 KB
new17.9 KB
PASSED: [[SimpleTest]]: [MySQL] 63,081 pass(es).
[ View ]

New patch changes the argument to entity, also removes wrapper in forum manager that was a simple wrapper function and not defined in ForumManagerInterface so no API change here.

Also @larowlan's feedback is welcome

+++ b/core/modules/forum/lib/Drupal/forum/ForumManager.php
@@ -314,7 +325,7 @@ protected function getTopicOrder($sortby) {
   protected function numberNew($nid, $timestamp) {
-    return comment_num_new($nid, $timestamp);
+    return $this->commentManager->getCountNewComments('node', $nid, 'comment_forum', $timestamp);

Because this will need to change forum manager.

larowlan’s picture

Status:Needs review» Reviewed & tested by the community

Great cleanup to see the wrapper gone from ForumManager, getting there slowly :)

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -296,6 +307,48 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
    +    if (\Drupal::currentUser()->isAuthenticated() && \Drupal::moduleHandler()->moduleExists('history')) {

    Let's replace \Drupal::currentUser() with $this->currentUser

  2. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -296,6 +307,48 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
    +  public function getCountNewComments(EntityInterface $entity, $field_name = NULL, $timestamp = 0, AccountInterface $account = NULL) {

    What is the purpose of the $account parameter?

Berdir’s picture

andypost’s picture

Seems this should be postponed on #2081585: Introduce HistoryRepository service

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new16.43 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

re-roll... I tried to address $this->currentUser from #50.

Status:Needs review» Needs work

The last submitted patch, 53: 2097123-newComments-53.patch, failed testing.

mgifford’s picture

Status:Needs work» Needs review
StatusFileSize
new16.45 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

hmm...

Status:Needs review» Needs work

The last submitted patch, 55: 2097123-newComments-55.patch, failed testing.

mgifford’s picture

Issue tags:+Needs reroll
willieseabrook’s picture

Issue tags:+TUNIS_2014_MARCH
ahmed25’s picture

Assigned:Unassigned» ahmed25
ahmed25’s picture

Assigned:ahmed25» Unassigned
Issue tags:-Needs reroll

I just checked this it doesn't need a reroll.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new2.78 KB
new17.54 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 67,929 pass(es), 321 fail(s), and 154 exception(s).
[ View ]

Fix #50, $account was supposed to be passed, but there's a current user service for.

Also removed leftover COMMENT_OPEN constant

andypost’s picture

StatusFileSize
new1.79 KB
new17.56 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,113 pass(es).
[ View ]

And a bit more fixes

The last submitted patch, 61: 2097123-newComments-61.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 62: 2097123-newComments-62.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review

62: 2097123-newComments-62.patch queued for re-testing.

andypost’s picture

StatusFileSize
new895 bytes
new18.43 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,105 pass(es).
[ View ]

And one more place

dawehner’s picture

  1. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -316,6 +327,48 @@ public function forbiddenMessage(EntityInterface $entity, $field_name) {
    +    if ($this->currentUser->isAuthenticated() && \Drupal::moduleHandler()->moduleExists('history')) {

    I guess we also want to inject the module handler.

  2. +++ b/core/modules/forum/tests/Drupal/forum/Tests/ForumManagerTest.php
    @@ -74,12 +74,17 @@ public function testGetIndex() {
    +    $comment_manager = $this->getMockBuilder('\Drupal\comment\CommentManager')
    +      ->disableOriginalConstructor()
    +      ->getMock();
    +

    Can't we just mock the interface?

andypost’s picture

StatusFileSize
new4.16 KB
new19.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

1) fixed, but module handler will not needed after #2081585: Introduce HistoryRepository service
2) fixed

Also I start to think- do new method needs access check for current user...

Status:Needs review» Needs work

The last submitted patch, 68: 2097123-newComments-68.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
StatusFileSize
new0 bytes
new19.06 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed.
[ View ]

missed module handler

Status:Needs review» Needs work

The last submitted patch, 70: 2097123-newComments-70.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review

fixed interdiff

andypost’s picture

StatusFileSize
new712 bytes
new19.06 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 68,121 pass(es).
[ View ]

proper patch & interdiff

YesCT’s picture

Issue tags:+@deprecated

I have a feeling we have a system for dealing with @deprecated things.
Is it, commit the patch like this, and then do a follow-up issue to actually remove it?

roderik’s picture

StatusFileSize
new18.81 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,473 pass(es), 123 fail(s), and 68 exception(s).
[ View ]
new8.65 KB
new661 bytes

Yeah, the system is at #2158871: [Policy, no patch] Clearly denote when @deprecated code is slated to be removed which includes a followup-meta. Reworded @deprecated to fit this.

Also: needed reroll for #2182055: Comment module causes Circular reference error on a REST request and #2079797: Provide a trait for $this->t() and $this->formatPlural().
Unfortunately I don't know what to do with the 'currentUser' class variable which has been removed. Do we pass the current account into getCountNewComments() now?

Attached part of the before-after reroll diff FYI. (Not included there: comment.module. in there, the call to getCountNewComments() was changed.)

roderik’s picture

StatusFileSize
new12.58 KB

Correct version of what I wanted to upload FYI. (I guess it's not customary to do before-after-reroll diffs, they're confusing.)

Status:Needs review» Needs work

The last submitted patch, 75: 2097123-newComments-75.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new18.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 70,596 pass(es).
[ View ]
new2.94 KB

Hah, brain freeze. Forgot to check existing calls to getCountNewComments() before I changed its signature.

Rephrase:
I do not know how to reroll this properly; protected $currentUser wasn't removed for nothing. I placed the reference to \Drupal::currentUser() back in the rerolled patch, to make tests pass, but I'm pretty sure this is not wanted :/

(Maybe #2188287: Split CommentManager in two will make this easier? I don't know.)

slashrsm’s picture

StatusFileSize
new18.04 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,703 pass(es), 0 fail(s), and 1 exception(s).
[ View ]

Reroll after PSR-4.

slashrsm’s picture

StatusFileSize
new18.3 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 71,762 pass(es), 0 fail(s), and 1 exception(s).
[ View ]
new4.06 KB

Fixed few minor things.

The last submitted patch, 79: 2097123_79.patch, failed testing.

Status:Needs review» Needs work

The last submitted patch, 80: 2097123_80.patch, failed testing.

slashrsm’s picture

Status:Needs work» Needs review
StatusFileSize
new18.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2097123_83.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new789 bytes
marcingy’s picture

Status:Needs review» Reviewed & tested by the community

More a comment than anything do we have an issue to fix up comment_new_page_count I am assuming yes!!

slashrsm’s picture

slashrsm’s picture

Status:Reviewed & tested by the community» Needs work

Actually.... this is not OK. DB code should go into CommentStorage.

roderik’s picture

Assigned:Unassigned» roderik

You're right.

roderik’s picture

Status:Needs work» Needs review

Actually, no. This uses an EntityQuery, which is storage agnostic, so it's fine in CommentManager.

I'm not setting back RTBC, only because I don't understand what changed between comment #20 and now. #20 says you can't inject current_user into services.

(If this has indeed changed, just set back RTBC.)

slashrsm’s picture

Status:Needs review» Reviewed & tested by the community

Hm... you're right. current_user is properly injected as of #80.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 83: 2097123_83.patch, failed testing.

slashrsm’s picture

Status:Needs work» Needs review
StatusFileSize
new18.17 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Unable to apply patch 2097123_91.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Reroll.

slashrsm’s picture

Status:Needs review» Reviewed & tested by the community

This was a straight reroll.

Status:Reviewed & tested by the community» Needs work

The last submitted patch, 91: 2097123_91.patch, failed testing.

roderik’s picture

Status:Needs work» Needs review
StatusFileSize
new18.08 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,930 pass(es).
[ View ]

Reroll. (Effectively: one replacement from a field_id expression to field_name.)

slashrsm’s picture

Status:Needs review» Reviewed & tested by the community
webchick’s picture

Status:Reviewed & tested by the community» Fixed

Committed and pushed to 8.x. Thanks!

  • webchick committed 6532718 on 8.x
    Issue #2097123 by slashrsm, roderik, mgifford, pfrenssen, andypost, das-...

  • alexpott committed f0d4b5a on 8.x
    Revert "Issue #2097123 by slashrsm, roderik, mgifford, pfrenssen,...
andypost’s picture

Status:Fixed» Needs review
StatusFileSize
new18.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,729 pass(es), 1 fail(s), and 1 exception(s).
[ View ]

test reverted patch

andypost’s picture

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new738 bytes
new18.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,974 pass(es).
[ View ]

Fix broken test

The last submitted patch, 99: 1.patch, failed testing.

marcingy’s picture

Looks good

slashrsm’s picture

+1 for RTBC.

YesCT’s picture

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new18.78 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 72,891 pass(es).
[ View ]
new1.56 KB

read through this and noticed some nits. just fixed them. should still be rtbc.

also made #2296839: Remove deprecated comment_num_new()

#100 added comment to the list of module dependencies. It had to do this because #2287223: Use KernelTestBase for config schema tests where possible changed BlockConfigSchemaTest to be a kernel test and kernel tests do not auto enable dependencies of modules.

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Awesome! back to rtbc

YesCT’s picture

Issue tags:-accessibility

I dont think this changes anything related to accessibility. that tag probably was inherited from the parent issue.

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
+++ b/core/modules/comment/src/CommentViewBuilder.php
@@ -327,7 +327,8 @@ protected function alterBuild(array &$build, EntityInterface $comment, EntityVie
+    $new = \Drupal::service('comment.manager')
+      ->getCountNewComments(entity_load($context['entity_type'], $context['entity_id']));

This should be injected no?

alexpott’s picture

Status:Needs work» Reviewed & tested by the community

Nope - it's a static function - thanks @slashrsm

alexpott’s picture

Status:Reviewed & tested by the community» Fixed

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

  • alexpott committed 6e70062 on 8.x
    Issue #2097123 by andypost, roderik, slashrsm, das-peter, mgifford,...

Status:Fixed» Closed (fixed)

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