Problem/Motivation

Follow-up to #1498662-11: Refactor comment entity properties to multilingual
currently CommentStorage::buildQuery() adds inner join that have @todo to remove

Proposed resolution

fix todo

Remaining tasks

tbd

User interface changes

no

API changes

no

CommentFileSizeAuthor
#5 comment-join-user-2292815.5.patch1.62 KBlarowlan
#5 interdiff.txt554 byteslarowlan
#3 2292815-3.patch614 bytesjoshi.rohit100
#1 2292815-1.patch1.08 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
2292815-1.patch1.08 KB

stub

Status: Needs review » Needs work

The last submitted patch, 1: 2292815-1.patch, failed testing.

joshi.rohit100’s picture

joshi.rohit100
he/him
Primary language Hindi
Location Delhi, India
CreditAttribution: joshi.rohit100 commented
Status: Needs work » Needs review
FileSize
2292815-3.patch614 bytes

Updated.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs review » Needs work

@joshi.rohit100 As the issue said the patch should remove this join - no reason to join users because comment does not use the fetched data.
See comment_prepare_author()

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Needs review
FileSize
interdiff.txt554 bytes
comment-join-user-2292815.5.patch1.62 KB

Like so? Not sure of the perf implications here though.
i.m.o the join is better.

andypost’s picture

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

Actually it depends on usage, and a kind of commenters ;)

Anyway there's no performance effect because:
1) comment_prepare_author() in template_preprocess_comment() anyway loads/creates a user object to properly theme a username
2) parent comment is loaded for each rendered comment #1857946: Comment parent template variables are built twice

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

Yeah, a mongodb backend for example couldn't add such a join, and one entity storage shouldn't assume anything about another, they might not even be in the same place.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Are we already multiple loading all referenced users for comment listings?

  • catch committed 4ab1724 on 8.x 
    Issue #2292815 by larowlan, joshi.rohit100, andypost: Remove join...
andypost’s picture

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

Status: Fixed » Closed (fixed)

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