Comment form on same page as comments forces node render caching to be per user

Unblocked as comments are now a field.

More accurate title.

This is now blocked on #2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache.

Once that is in, this patch solves this issue.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php
@@ -150,6 +150,7 @@ public function instanceSettingsForm(array $form, array &$form_state) {
+      '#description' => t('Beware! This makes the page inherently slower for logged in users.'),

What's with all the drama here?

Well, 99% of people won't realize the harm that comes with having the comment form on the same page. I figured it might be worthwhile to mention that explicitly.

However, if reviewers don't think that's a good idea, I don't mind removing that :)

Updated issue summary.

Reroll to track changes in other patch at #2118703-28: Introduce #post_render_cache callback to allow for personalization without breaking the render cache. The code has become much simpler :)

#2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache got committed, which means this issue is now unblocked, yay! :)

Reroll to ensure it applies cleanly, and to s/comment_insert_form()/comment_replace_form_placeholder/, which better communicates what the #post_render_cache callback function does.

Don't forget about the "Beware!" stuff. A user facing text like that has no place for being in core :) .. or you might as well change it to "Here be dragons!" or something like that.

8: comment_form_breaks_render_cache-2099133-8.patch queued for re-testing.

8: comment_form_breaks_render_cache-2099133-8.patch queued for re-testing.

This reroll is identical to the patch in #8, it only removes the "Beware" message amateescu did not like. Could not think of better wording, so it's better to omit it.

(No interdiff because I just removed that hunk from the patch.)

Code looks great, and the last patch came back green so most recent one should do so as well.

Thanks!

+1 to patch, suppose no reason to have tests for this kind of render cache

#18: there is *extensive* test coverage in #2118703: Introduce #post_render_cache callback to allow for personalization without breaking the render cache for #post_render_cache. More importantly: existing integration tests don't need to be modified, so we already have test coverage.

Yeah, this looks good to me as well.

Could we make comment_replace_form_placeholder() a static method on CommentDefaultFormatter instead ?

#23: not yet, because #post_render_cache does not yet support object methods, for the reasons outlined at #2118703-42: Introduce #post_render_cache callback to allow for personalization without breaking the render cache. Rest assured that if it becomes possible, I will happily get rid of comment_replace_form_placeholder().

Re #23/#24:

As call_user_func_array() is used to call the callbacks, I would have thought that static class methods are in fact support by #post_render_cache. Because $callback is used as an array key, what is not supported is array($object, 'method') but I don't see a reason statics like 'Drupal\foo\ClassName::staticMethod' shouldn't be supported. I might be wrong, though.

#25: you're absolutely right. This is much better.

Thanks, yched & tstoeckler!

Awesome, thanks! Unless the bot objects this is RTBC.

+++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldFormatter/CommentDefaultFormatter.php
@@ -151,7 +151,7 @@ public function viewElements(FieldItemListInterface $items) {
+              '#callback' => 'Drupal\comment\Plugin\Field\FieldFormatter\CommentDefaultFormatter::renderForm',

Missing the leading '\'.

Addresses #29 and improves a comment.

We're very inconsistent with this in core currently, so I'm not marking back to "needs work", but on a general note our current coding standard is:

When specifying a class name in a string, use its full name including namespace, without leading \.

(Not advocating that that standard is a good thing, but it's our curent standard.)

Ups, didn't know about that, sorry. But since we're inconsistent anyway, this can be a novice task to fix all over core.

Hah, so that's exactly what I was thinking: none of our use Some\Crazy\Namespace\Class statements also omit a leading backslash.

Sorry: I wasn't sure what was preferred. Will do so in the future!

Retroactively tagging beta blocker as a blocker for #2151459: Enable node render caching.

Retroactively marking critical: a blocker to a critical issue should itself be critical, and this blocked #2151459: Enable node render caching, which is critical.

This one already fixed, any reason to mark it?