Problem/Motivation
Comment "body" field could be removed via field ui interface, so each comment trying to retrieve non-existent property (entity field api just silently returns NULL)
But when you delete a comment body field then you get notice in watchdog:
<em class="placeholder">Notice</em>: Trying to get property of non-object in <em class="placeholder">Drupal\comment\CommentForm->buildEntity()</em> (line <em class="placeholder">280</em> of <em class="placeholder">/home/andypost/www/core8/core/modules/comment/src/CommentForm.php</em>).
Proposed resolution
Skip additional processing when no comment body field.
(test only patch in #6)
Remaining tasks
review/commit
User interface changes
no
API changes
no
Beta phase evaluation
Issue category | Bug because core assumes that comment always have "comment_body" field so throws warning |
---|---|
Issue priority | Normal because just displays annoying warning |
Disruption | Non-disruptive |
Comment | File | Size | Author |
---|---|---|---|
#19 | 2422353-comment_body-19.patch | 5.9 KB | andypost |
#6 | 2422353-comment_body-6-fail.patch | 1.68 KB | andypost |
Comments
Comment #1
andypostMore clean-up:
1) use keyed array to not load each comment multiple times
2) comment author is rendered within build
3) use link template for edit form, content translation somehow not register template
PS: maybe change title to - revamp comment admin form
Comment #2
mgiffordSo what are the best steps for testing this. It applies nicely.
Comment #3
andypostThere's no way to test that because
\Drupal\Core\Entity\ContentEntityBase::__get
cares to return null for unknown propertiesBut when you delete a comment body field then you get notice in watchdog
<em class="placeholder">Notice</em>: Trying to get property of non-object in <em class="placeholder">Drupal\comment\CommentForm->buildEntity()</em> (line <em class="placeholder">280</em> of <em class="placeholder">/home/andypost/www/core8/core/modules/comment/src/CommentForm.php</em>).
So I checked all core for other places, that included in patch
Comment #4
mgiffordOk.. I reviewed the code. Looks fine. I applied it to SimplyTest.me, no problem. Added a article and a comment. All good.
Comment #5
alexpottSurely we can write a test which create a comment type without a body field?
Comment #6
andypostHere's a test
Comment #7
andypostLink related issue that found when writing a test
Comment #9
andypostback to rtbc, test to post comment when no body field fails
Comment #10
webchickHm. Why is the solution to this problem to check for an optional comment_body field versus just making comment_body required and unable to be removed from the UI? Like what does comment module actually do anymore without a comment body?
Comment #11
andypost@webchick Personally I'm pretty sure that there's like 5% of usage for new comments to live without body. For example Event content type, with comments used to attach photo reports from the event.
Also we have a lot of troubles trying to prevent things (delete config entity) that supposed to be available. Here's what I'm following:
#1831928: Support a 'locked' property on ConfigEntities
#2147549: Prevent taxonomy_forums field be removed in admin interface
#2274433: Do not allow to alter Locked field via UI
#2289551: Clarify what 'locked' means for a config entity and whether it's okay for code to rely on a locked config entity existing
So I'm sure that better allow fault-tolerant solution then trying to limit core module functionality
Comment #12
andypostremoved unnecessary changes to me more inline with current HEAD
@larowlan maybe separate admin overview fix (author is rendered) and what do you think overall on issue
Comment #13
larowlanOther than the out of scope changes, this looks good to me
This seems out of scope, can we do that in a separate issue please?
Again, out of scope
Comment #14
andypostAdded beta evaluation and removed unneeded changes #13 to separate issues:
#2505835: Optimize CommentAdminOverview
#2505841: Make CommentAdminOverview use link templates
Comment #15
larowlanThanks @andypost
Comment #18
jibranComment #19
andypostRe-roll after #2505835: Optimize CommentAdminOverview
Comment #20
alexpottI agree that it is super hard to prevent deleting of configuration. Code should not rely on configurable fields.
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 4eff3d1 and pushed to 8.0.x. Thanks!