[regression] custom field twig for second comment fields has null value in comment after 11.3.0 upgrade

Hi, in Drupal core changes are made on on 11.x (our main development branch) first, and are then back ported as needed according to the Core change policies. Thanks.

Thanks for the report. Nic and I both are aware and I will have time over the weekend to look into this if he doesn't get to it sooner. My first guess was it is not comment specific alas rather it's the subtheme and an initial quick investigation seems to corroborate this:

1. no suggestion code in comment
2. rather the suggestions are added by the new SystemThemeHooks::themeSuggestionsField
3. which relies on code in FormatterBase which has been unchanged since 2012.

Something likely broke either with OOP-ifying the suggestion hooks or the theme engine conversion. More later.

#1685492: Convert theme engines into services

Is the theme engine conversion for reference.

A couple other thoughts.

We should git bisect to determine when this broke.

Possible culprits:
Subtheme - unlikely since most themes are a subtheme
Sub subtheme - unlikely since barrio sets base theme to false
Theme engine conversion
Oop themes
A change in comment module
Something with theme suggestions in general

I have a question too. You said a second comment type are you doing the suggestion on the comment bundle?

Please provide some more details on how to reproduce. I tried to reproduce by doing the following.

1. Checkout the branch 10.6.x above
2. drush si
3. Create new starter kit theme
4. Enable new theme and set to default
5. Create a new comment type
6. Go to article
7. Set comment on article to use second type
8. Create article
9. Add comment to article
10. Inspect

I see these suggestions:

 FILE NAME SUGGESTIONS:
   ▪️ field--node--comment--article.html.twig
   ▪️ field--node--comment.html.twig
   ▪️ field--node--article.html.twig
   ✅ field--comment.html.twig
   ✅ field--comment.html.twig
   ▪️ field.html.twig

For completeness I created a new comment field and added a comment there I see these suggestions on that field? Is this what you meant?

 FILE NAME SUGGESTIONS:
   ▪️ field--node--field-comment-two--article.html.twig
   ▪️ field--node--field-comment-two.html.twig
   ▪️ field--node--article.html.twig
   ▪️ field--field-comment-two.html.twig
   ✅ field--comment.html.twig
   ▪️ field.html.twig

Assuming the second is what you mean I then created :
and I see this on 10.6

 FILE NAME SUGGESTIONS:
   ▪️ field--node--field-comment-two--article.html.twig
   ▪️ field--node--field-comment-two.html.twig
   ▪️ field--node--article.html.twig
   ✅ field--field-comment-two.html.twig
   ▪️ field--comment.html.twig
   ▪️ field.html.twig

Updating this afterwards to 11.3 and running updates I do see something weird.
The thing is I have both field--comment.html.twig and field--field-comment-two.html.twig but the second comment field doesn't render at all.

Removing field--field-comment-two.html.twig brings it back. So yes something very weird is going on, let me try a git bisect.

Bisect reveals this as the culprit: e6abe62c098dd6ee04e7165678564164330aac6a
#3536196: Investigate preprocess_field in the comment module

No wonder it tickled the back of my mind...

I confirmed locally that changing

#[Hook('preprocess_field__comment')]

back to

#[Hook('preprocess_field')]

Fixes the issue.

Need to update the IS for some clarity, the field doesn't print if the suggestion exists, the suggestion is right, but the preprocess doesn't populate the variables so the comment field does not print.

One final note is that we changed this because the comment said to change it when https://www.drupal.org/node/939462 was resolved and that was fixed.

This proves that both the comment was wrong and test coverage was not sufficient for this.

I'd recommend a quick fix revert the preprocess, then we can decide if it's worth digging in why there is still and issue with 939462.

Ah. Thanks for sleuthing, great work. So the problem is not at all new, indeed this comment describes it as a D6-D7 regression: field--comment--foo will call preprocess methods of the base hook (field) it will not call the preprocess functions of field--comment.

Fixing this with a loop that peels off -- sections and calls preprocess on each would mean preprocess functions which didn't fire before would fire now which is probably a BC break so it can only be done in D12. Maybe allowing an explicit list of preprocess hooks to fire on hook_theme would help and for D11.* the current and base hook preprocess could be added automatically to achieve BC and then in D12 the loop described above would run instead.

For now revert of this specific method seems to be the right way forward.

If you want to create the MR changing the hook I can create the follow up for the root cause!

If you need help with how to do that don't hesitate to ask!

I've been thinking about the best way to test it in the meantime.

Edited to add: this is a great starting point for some documentation https://www.drupal.org/docs/develop/git/using-gitlab-to-contribute-to-dr...

Let me know if you have any questions.

@nicxvan
I work on the same site as @fl-49 who reported this issue. We can now report that manually reverting the change in this diff that you identified as the culprit indeed fixed the issue reported here. This is the diff file we looked at and manually reverted on our test site. https://git.drupalcode.org/project/drupal/-/merge_requests/12793.diff.
This test was done while running Drupal core 11.3.2. Hope this helps.

I thank you for confirming!

I will offer you the same assistance if you want to open a merge request to fix this, the link with the process is on my previous comment. Feel free to reach out here or in slack if you have any questions!

Happy to help, the actual fix would be just changing the hook attribute from preprocess_field__comment to preprocess_field

We can figure out tests later.

To do this you want to clone drupal from the project page here: https://www.drupal.org/project/drupal/git-instructions

Make sure you have added your ssh keys to your user account.

Then at the top of this issue press the green button that says get push access.

After that click the link that says show commands.

You'll then want to run the first command under

Add & fetch this issue fork’s repository

Then run the command under Check out this branch for the first time

The other command is when switching between issues after you already have it locally. You won't need this today.

Then you can make the change in the code, commit it and push.

Once you have that I can help further, feel free to ping me on the contribute channel in drupal slack others might help too in that case.

Setting to needs work while I work on a test.

I came up with a test setup that I think adds a second comment field to a test entity, we now need to assert that the form is on a display of that entity.

I tried rendering it with viewbuilder and renderRoot was getting this error:

1) Drupal\Tests\comment\Kernel\CommentFieldPreprocessTest::testPreprocessFields
Error: Call to a member function getPattern() on null

I also tried the kernel->render method, it worked once, but I am not sure that will actually test since we need the actual theme to load the template.

Maybe this doesn't belong in a kernel test.

Needs work still to finish or remove the tests.

Refactored the test. Looking back at it, perhaps part of what was missing from the original test was setting the active theme.

Failing test only job: https://git.drupalcode.org/issue/drupal-3566261/-/jobs/7947163

Applied comment suggestion, so build is going again, but previous builds passed. If any tests fail, they'll just need re-running.

I haven't looked at the IS for what updates are outstanding per the tag.

Thank you so much!

I already updated the issue summary, just forgot to remove the tag.

I think this is good to go now, we have a follow up.

I wrote the first pass at the test but it was so significantly refactored I think I can rtbc.

I took a first pass at credit too.

Test only fails as expected and it's essentially a revert of a minor change we just made with better test coverage.

Trying to improve the title which is the commit message

I iterated on the title a bit.

Did you find the cause of 33? I think it's unrelated.

Adding a couple of related issues.

Committed/pushed to 11.x and cherry-picked to 11.3.x, thanks!