Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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
Issue category | Bug because no way to use comments for other entity types. |
---|
Comment | File | Size | Author |
---|---|---|---|
#17 | comment-type-required-2380071-17-complete.patch | 3.76 KB | geertvd |
#17 | comment-type-required-2380071-17-test.patch | 2.35 KB | geertvd |
Comments
Comment #1
andypostProbably better to add validation to manage fields screen by preventing to add unconfigurable field
Comment #2
andypostSuppose needs other issue - when any field is deleted from comment type the comment body is lost
Comment #3
jibranI am also getting same kind of exception in #569434-204: Remove taxonomy term description field; provide description field for forum taxonomy
Comment #4
larowlanYeah 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.
Comment #5
larowlanMy comment there "126 I respectfully disagree. Having to create the comment type first is a far worse UX."
Comment #6
larowlanBut this would make sense
Comment #8
brahmjeet789 CreditAttribution: brahmjeet789 commentedI applied patch 6 but it's not working .
Comment #9
larowlanI 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.
Comment #10
andypostAs i said in summary we need to provide useful error message by pointing user to comment types page
Comment #11
pcambraI 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.
Comment #12
larowlanWorks for me
Comment #14
andypostRe-roll, the route name changed to "entity.comment_type.collection"
Comment #15
larowlancheers
Comment #16
alexpottStill needs tests.
Comment #17
geertvd CreditAttribution: geertvd commentedComment #18
geertvd CreditAttribution: geertvd commentedComment #20
andypostGreat
Comment #21
alexpottThis 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!