Problem/Motivation

Core provides only one comment type that bound to nodes.
So when user trying to add a comment field to any other entity type (e.g. user, term) the list of comment types for field storage is empty. User can save the field storage settings without any warnings or exceptions.
But on entity page (user/1) you get

LogicException: A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break. in Drupal\Core\Render\Renderer->render() (line 344 of /home/andypost/www/core8/core/lib/Drupal/Core/Render/Renderer.php).

Drupal\Core\Render\Renderer->render(Array, 1)
Drupal\Core\Render\BareHtmlPageRenderer->renderBarePage(Array, 'Error', 'maintenance_page')
Drupal\Core\EventSubscriber\DefaultExceptionSubscriber->onHtml(Object)
Drupal\Core\EventSubscriber\DefaultExceptionSubscriber->onException(Object, 'kernel.exception', Object)
Drupal\Component\EventDispatcher\ContainerAwareEventDispatcher->dispatch('kernel.exception', Object)
Symfony\Component\HttpKernel\HttpKernel->handleException(Object, Object, 1)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\PageCache->handle(Object, 1, 1)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1)
Stack\StackedHttpKernel->handle(Object, 1, 1)
Drupal\Core\DrupalKernel->handle(Object)

Proposed resolution

Add validation to field settings with a message "Add new comment type at *URL*"
Add check to comment formatter to prevent this exception

Remaining tasks

User interface changes

API changes

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because no way to use comments for other entity types.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

andypost’s picture

Probably better to add validation to manage fields screen by preventing to add unconfigurable field

andypost’s picture

Suppose needs other issue - when any field is deleted from comment type the comment body is lost

jibran’s picture

larowlan’s picture

Yeah I'm seeing this exception in HEAD, the issue seems to bubble from how views adds contextual links.

We have #2374087: Create a persistent comment body field storage for the body.

Ironically I had support for creating the comment-type on the fly in the settings form in the original comment-type patch - but was asked to remove it for UX reasons - and @berdir warned that we'd hit this issue - see #2228763: Create a comment-type config entity and use that as comment bundles, require selection in field settings form - comments 126-144.

larowlan’s picture

My comment there "126 I respectfully disagree. Having to create the comment type first is a far worse UX."

larowlan’s picture

Status: Active » Needs review
FileSize
720 bytes

But this would make sense

brahmjeet789’s picture

I applied patch 6 but it's not working .

larowlan’s picture

I don't get what the issue is here other than poor ux and the missing required attribute.
You have to create the comment type first.
We voted for inline creation but lost. We voted for creation in a modal but lost. All we can do here is add the required attribute and be done.

andypost’s picture

As i said in summary we need to provide useful error message by pointing user to comment types page

pcambra’s picture

I got really confused encountering this today.
I think the required is the simplest solution to avoid problems. Added a link to the comment types page.

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

Works for me

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 11: comment-type-required-2380071-11.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.41 KB

Re-roll, the route name changed to "entity.comment_type.collection"

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

cheers

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

Still needs tests.

geertvd’s picture

geertvd’s picture

Status: Needs work » Needs review

The last submitted patch, 17: comment-type-required-2380071-17-test.patch, failed testing.

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Great

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed dac834e and pushed to 8.0.x. Thanks!

  • alexpott committed dac834e on 8.0.x
    Issue #2380071 by geertvd, pcambra, andypost, larowlan: No way to add...

Status: Fixed » Closed (fixed)

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