#2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI deals only with nodes. This issue does the same for the comment entity types. Fix preprocess and templates to bypass special processing of base fields if the new setting is enabled:

  • 'created'
  • 'uid'
  • 'pid'
  • 'subject'

Solution

Copy everything done in #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI.

CommentFileSizeAuthor
#18 interdiff-14-18.txt4.08 KBilya.no
#18 3090187-18.patch39.85 KBilya.no
#14 interdiff-12-14.txt896 bytesilya.no
#14 3090187-14.patch35.08 KBilya.no
#12 3090187-12.patch35.14 KBabx
#10 3090187-10.patch35.14 KBandypost
#10 interdiff.txt14 KBandypost
#8 3090187-8.patch22.08 KBandypost
#2 comment-base.3090187-2.patch22.58 KBadamps

Comments

AdamPS created an issue. See original summary.

adamps’s picture

adamps commented
Title: Mechanism to disable preprocessing of base fields in comment and custom block entity types so they can be configured via the field UI » Mechanism to disable preprocessing of base fields in comment entity type so they can be configured via the field UI
Issue summary: View changes
Status: Active » Needs review
Issue tags: +Needs tests
StatusFileSize
new comment-base.3090187-2.patch22.58 KB

Custom block entity type seems fine - there is no preprocess function or template that interferes with the display of any base field.

First patch is missing tests but it gives us a starting point for discussion.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Component: entity system » comment.module
Related issues: +#1857946: Comment parent template variables are built twice, +#2227503: Apply formatters and widgets to Comment base fields

Looks nice start, also this may affect related issues

andypost’s picture

andypost
he/him
Primary language Russian
commented
Version: 8.9.x-dev » 9.1.x-dev

Version: 9.1.x-dev » 9.2.x-dev

Drupal 9.1.0-alpha1 will be released the week of October 19, 2020, which means new developments and disruptive changes should now be targeted for the 9.2.x-dev branch. For more information see the Drupal 9 minor version schedule and the Allowed changes during the Drupal 9 release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs review » Needs work
Issue tags: +Needs reroll 
core9$ curl https://www.drupal.org/files/issues/2019-11-05/comment-base.3090187-2.patch |patch -p1
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100 23120  100 23120    0     0  45964      0 --:--:-- --:--:-- --:--:-- 45964
patching file core/modules/comment/comment.module
Hunk #1 succeeded at 546 (offset -87 lines).
Hunk #2 FAILED at 659.
Hunk #3 succeeded at 640 (offset -88 lines).
Hunk #4 succeeded at 652 (offset -88 lines).
1 out of 4 hunks FAILED -- saving rejects to file core/modules/comment/comment.module.rej
patching file core/modules/comment/templates/comment.html.twig
patching file core/themes/bartik/templates/comment.html.twig
patching file core/themes/classy/templates/content/comment.html.twig
patching file core/themes/stable/templates/content/comment.html.twig
Hunk #2 succeeded at 12 with fuzz 1.
andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new 3090187-8.patch22.08 KB

Re-roll

Status: Needs review » Needs work

The last submitted patch, 8: 3090187-8.patch, failed testing. View results

andypost’s picture

andypost
he/him
Primary language Russian
commented
Status: Needs work » Needs review
StatusFileSize
new interdiff.txt14 KB
new 3090187-10.patch35.14 KB

Fix tests

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

abx’s picture

abx commented
StatusFileSize
new 3090187-12.patch35.14 KB

I couldn't apply patch due to this line

'comment.html.twig' => '68718b6de9a0d21f5180a9fbcc40987f',

it should be

'comment.html.twig' => 'f5f6371850371b2db33dacceca455b1c',

Status: Needs review » Needs work

The last submitted patch, 12: 3090187-12.patch, failed testing. View results

ilya.no’s picture

ilya.no commented
Status: Needs work » Needs review
StatusFileSize
new 3090187-14.patch35.08 KB
new interdiff-12-14.txt896 bytes

I've updated patch with tests fix.

adamps’s picture

adamps commented

Thanks @ilya.no. The patch still needs tests, so I wonder it if needs to be "Needs work"? I hope to find some time soon but also quite busy, so if anyone else can that would be great.

andypost’s picture

andypost
he/him
Primary language Russian
commented

Thank you, meantime I see following related efforts
- #1875974: Abstract 'component type' specific code out of EntityDisplay
- #2869124: Base field overrides cannot be translated which renders some entity form labels untranslatable
- #2844302: Move Field Layout data model and API directly into \Drupal\Core\Entity\EntityDisplayBase

andypost’s picture

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

NW for tests, I bet something could be taken from node tests

ilya.no’s picture

ilya.no commented
Status: Needs work » Needs review
StatusFileSize
new 3090187-18.patch39.85 KB
new interdiff-14-18.txt4.08 KB

Attaching patch with test. Not sure, whether it's too simple.

andypost’s picture

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

Thank you, looks great

adamps’s picture

adamps commented
Status: Needs review » Reviewed & tested by the community

Great thanks, looks good to me too.

It seems that my original patch has been reviewed by @ilya.no + @andypost, and now @ilya.no wrote tests reviewed by @andypost and me.
It feels to me that this is ready for the core committers to look at now. It's very much a copy of #2923701: Mechanism to disable preprocessing of node base fields so they can be configured via the field UI so hopefully nothing controversial, thanks.

Strictly speaking we probably ought to have an "only tests" patch that fails.

adamps’s picture

adamps commented

You can test out this patch using the latest 2.0.x dev release of Manage Display module.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 18: 3090187-18.patch, failed testing. View results

adamps’s picture

adamps commented
Status: Needs work » Reviewed & tested by the community

Random fail

  • catch committed 5fb8e24 on 10.0.x 
    Issue #3090187 by ilya.no, andypost, AdamPS, abx: Mechanism to disable...

  • catch committed f7ecd6b on 9.4.x 
    Issue #3090187 by ilya.no, andypost, AdamPS, abx: Mechanism to disable...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Great to see this one ready. Will be amazing to be able to remove these hunks from preprocess when the rest of the steps are done.

Committed/pushed to 10.0.x and cherry-picked to 9.4.x, thanks!

adamps’s picture

adamps commented

Great many thanks @catch.

Yes, this completes phase A from #2353867: [META] Expose Title and other base fields in Manage Display of "removing the obstacles". We can now move onto the journey to deprecate the old hunks hopefully in D10 for removal in D11.

Status: Fixed » Closed (fixed)

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