Updated: Comment #6

Problem/Motivation

Comment field 'field_id' is a surrogate field that holds ({entity_type}__{field_name}) 'node__comment' data so it can't be user as ER field as is.
We have to store the value in that form because comment module uses that name as bundle for comment entity but field instance can't have a dot in it's name, it was decided in #731724: Convert comment settings into a field to make them work with CMI and non-node entities

Proposed resolution

introduce field item and value class to properly return value.

Remaining tasks

Find solution and fix.

User interface changes

no

API changes

tbd

Original report by @amateescu

I guess this wasn't done in the first place because entity reference couldn't reference config entity types, but now it does so we can convert this field.

CommentFileSizeAuthor
#15 2149859-comment-field-id-15.patch3.6 KBandypost
#13 2149859-comment-field-id-13.patch4.47 KBandypost
#13 interdiff.txt1.93 KBandypost
#9 2149859-comment-field_id-8.patch2.54 KBandypost
#9 interdiff.txt1.55 KBandypost
#8 2149859-comment-field_id-7.patch2.34 KBandypost
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented

This will make a lot of code cleaner

amateescu’s picture

amateescu CreditAttribution: amateescu commented
Status: Active » Postponed

Forgot to say that I'd prefer this to wait for #2112239: Convert base field and property definitions.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Postponed » Active
klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
CreditAttribution: klausi commented
Status: Active » Closed (duplicate)

Duplicate of #2181593: Convert entity bundle base fields to entity reference.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Closed (duplicate) » Active

Active again because #2181593-26: Convert entity bundle base fields to entity reference
Also closed #2197723: Comment::getFieldId() should return actual field ID as duplicate

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Issue summary: View changes

updated summary about 'field_id' is used as bundle for comment entity

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Active » Needs review
FileSize
2149859-comment-field_id-7.patch2.34 KB

let's try fix in constructor

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
FileSize
interdiff.txt1.55 KB
2149859-comment-field_id-8.patch2.54 KB

fix bug

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Related issues: +#1995944: Remove entity_id and entity_type from the comment table and replace with relationship tables, +#2197723: Comment::getFieldId() should return actual field ID

added related issues

The last submitted patch, 8: 2149859-comment-field_id-7.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 9: 2149859-comment-field_id-8.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
2149859-comment-field-id-13.patch4.47 KB

Another round

Status: Needs review » Needs work

The last submitted patch, 13: 2149859-comment-field-id-13.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Needs review
FileSize
2149859-comment-field-id-15.patch3.6 KB

Suppose the issue should be renamed to "rename confusing field_id to bundle" - there's no way to make it ER.

Also patch just cleans up a bit code, core needs to provide a right approach to create comments - by providing entity type and field name.
Main purpose of the patch:
1) remove ER definition - a part of #2002158: Convert form validation of comments to entity validation
2) change the logic to get field name.

Probably one day we could make 'entity_type' a required field...

Details:
At least EntiyDisplayBase calls _field_create_entity_from_ids() to create comment entity for internal use, so we still can't make "entity_type" field required.
I've checked that core does not trying to exlode "field_id" value to get entity type ID - so for now comment module is safe.

Status: Needs review » Needs work

The last submitted patch, 15: 2149859-comment-field-id-15.patch, failed testing.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented

The problem here that I was trying to convert to field_id that actually a reference to field instance, that generated when comment field is bound to entity.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Needs work » Closed (duplicate)
Related issues: +#2233157: Make the comment entity_id be a reference field

#2233157: Make the comment entity_id be a reference field is commited so we can close this one

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

This issue is about the field_id, the other one was about the entity_id, not the same thing...

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost commented
Status: Closed (duplicate) » Needs work

sure

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs work » Closed (fixed)

field_id is gone now, we have comment_type instead (which is an ER field)