Problem/Motivation

\Drupal\comment\CommentViewBuilder::buildComponents() within loop checking $this->moduleHandler->moduleExists('history') && $this->currentUser->isAuthenticated() for every comment and attach indication of unread comments.
But history module works only for node entities which is a bug when comments attached to other entities.

Also it creates placeholder for each comment to attach similar settings for JS which is needed only once (so could be optimized)

Proposed resolution

- Add condition to attach history libraries to node entities only
- Move common condition check out of loop
-

Remaining tasks

- create a patch
- review
- commit

User interface changes

no

API changes

no

Data model changes

no

CommentFileSizeAuthor
#7 interdiff_2_7.txt2.82 KBzviryatko
#7 2955803-7-test-only.patch2.98 KBzviryatko
#7 2955803-7.patch4.17 KBzviryatko
#5 2955803-5.patch4.2 KBzviryatko
#5 interdiff_2_5.txt2.84 KBzviryatko
#2 2955803-2.patch1.19 KBandypost

Comments

andypost created an issue. See original summary.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Active » Needs review
Related issues: +#2081585: [META] Modernize the History module, +#766670: The {history} table in Drupal doesn't scale at all
StatusFileSize
new 2955803-2.patch1.19 KB

Kind of that fix should work

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Looks good.

Should we add a test?

andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: +Needs tests

Looks yep, this is a bug

Another question is performance - each comment creates it but placeholders are not cheap

zviryatko’s picture

zviryatko commented
StatusFileSize
new interdiff_2_5.txt2.84 KB
new 2955803-5.patch4.2 KB
andypost’s picture

andypost
he/him
Primary language Russian
commented
Issue tags: -Needs tests

Thanx for tests, please create separate patch with suffix -test-only.patch to be sure coverage works

+++ b/core/modules/comment/tests/src/Functional/CommentEntityTest.php
@@ -0,0 +1,84 @@
\ No newline at end of file

minor, needs new line

zviryatko’s picture

zviryatko commented
StatusFileSize
new 2955803-7.patch4.17 KB
new 2955803-7-test-only.patch2.98 KB
new interdiff_2_7.txt2.82 KB

Status: Needs review » Needs work

The last submitted patch, 7: 2955803-7-test-only.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: Unassigned » larowlan
Status: Needs work » Reviewed & tested by the community

IMO it's ready

  • catch committed fb4c903 on 8.6.x 
    Issue #2955803 by zviryatko, andypost: Do not attach history for non-...

  • catch committed b77a757 on 8.5.x 
    Issue #2955803 by zviryatko, andypost: Do not attach history for non-...
catch’s picture

catch
he/him
Primary language English
commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Fixed the following phpcs issues on commit:

FILE: .../core/modules/comment/tests/src/Functional/CommentEntityTest.php
----------------------------------------------------------------------
FOUND 3 ERRORS AND 1 WARNING AFFECTING 4 LINES
----------------------------------------------------------------------
  1 | ERROR   | [x] The PHP open tag must be followed by exactly one
    |         |     blank line
  2 | ERROR   | [x] Namespaced classes, interfaces and traits should
    |         |     not begin with a file doc comment
 14 | WARNING | [x] Unused use statement
 84 | ERROR   | [x] The closing brace for the class must have an
    |         |     empty line before it
----------------------------------------------------------------------
PHPCBF CAN FIX THE 4 MARKED SNIFF VIOLATIONS AUTOMATICALLY
----------------------------------------------------------------------

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

andypost’s picture

andypost
he/him
Primary language Russian
commented
Assigned: larowlan » Unassigned

Status: Fixed » Closed (fixed)

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