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.
Now we have fieldable comments, it makes sense to have comment body and subject as fields. Follow-up from #504666: Make comments fieldable.
Comment | File | Size | Author |
---|---|---|---|
#181 | drupal.comment-body-required.181.patch | 662 bytes | sun |
#172 | comment_update_quickfix.patch | 977 bytes | yched |
#167 | comment-body_fields-538164-163.patch | 29.4 KB | scor |
#164 | comment-body_fields-538164-163.patch | 29.4 KB | scor |
#163 | comment-body_fields-538164-162.patch | 29.43 KB | scor |
Comments
Comment #1
catchAdding tag.
Comment #2
catchBump. Not sure I'm going to have time to work on this before Wednesday, but an additional sweetener is this would remove a database query per check_markup() (cache_get(), so only with database caching)) - which is up to 300 on pages with long comment threads.
Comment #3
catchHere's an initial patch:
Took on both comment body and comment subject - you might want just body for a blog or forum, or just title + video for something else, so makes sense to have them both fields and both optional.
Removed some of the hard-coded references to comment->subject - we either use 'in reply to $node->title' or $comment->cid in those situations.
Still TODO:
fix tests. I got weird taxonomy errors locally so had trouble even seeing what the breakage was.
Data migration for the upgrade path should be possible to just do an INSERT INTO SELECT FROM - but should fix tests first.
CNR for the test bot ony, fair bit still to do here.
Comment #4
catchAll comment, tracker and forum tests pass with this, will let the test bot handle the others. Shouldn't be too far off, needs benchmarks and upgrade path now.
Comment #5
catchSince we no longer have a title, which is optional, but not really optional since it's hard coded into queries, so was filled in anyway with whatever garbage was at the top of a comment in a _submit handler, I had to change the comment block a bit. Obligatory screenshot. IMO it's better like this.
Comment #7
catchDown to three fails in search module now. This is due to #523894: Comment indexing does not include the fully rendered comment which may have to be merged in here..
Comment #9
yched CreditAttribution: yched commentedCrazy idea:
Current behavior is: comment_submit() fills {comment}.subject with the beginning of {comment}.body if there's no explicit subject.
Maybe we could change that slightly:
- comment_submit() calls field_attach_view() on the whole content, renders it, strips HTML, and stores the result in {comment}.precomputed or whatever.
- where we previously displayed $comment->subject (comment administration, UI messages...), we can use
substr($comment->precomputed, 0, N)
- comment_node_update_index() can simply return $comment->precomputed, and we solve performance concerns of running field_attach_view() on an arbitrary large # of comments at search_index time.
Well, I was just recently thinking that having 'comment_body' available in Field UI to be shared on non-comment entities makes little sense, and will mainly clutter the 'add existing field' selector and confuse people.
I think this calls for an (optional)
$field['entity_types'] = array('comment')
property, forbidding the creation of an instance on non-comment bundles.Comment #10
catchThe pre-computed property doesn't sound too bad to me - we still wouldn't require a subject field but it'd give something from the comment itself to look at on comment administration and elsewhere. One issue with using it for search is it might mess up search rankings for semantic HTML tags a little - but really, these are comments, and that's very broken how it is now, so I think it's fine if comment text gets a flat ranking regardless of markup.
Comment #11
yched CreditAttribution: yched commentedre search index: Yeah, I had the same reasoning.
Although, if we want to let the HTML in comments affect node search rankings, we could store the pre-computed output as HTML, and do the strip tags in
substr(strip_tags($comment->precomputed), 0, N)
when doing the 'print beginning of comment in admin pages and messages'.Not sure that's worth it, though.
Comment #12
yched CreditAttribution: yched commentedCreated #568084: Limit the scope of some fields to some entity types.
Comment #13
moshe weitzman CreditAttribution: moshe weitzman commentedThe precomputed value is basically a cache and thats just a pain to keep fresh considering how dynamic fields can be. IMO not a good idea. I am not too worried about the indexing performance. Those pages are only viewed by a computer.
Comment #14
yched CreditAttribution: yched commented"The precomputed value is basically a cache and thats just a pain to keep fresh considering how dynamic fields can be"
Well, we do know that field values only ever change when there's a call to field_attach_presave(). Comment module could implement hook_field_attach_presave() to recompute the value whenever a comment gets saved or updated ?
Comment #15
catchHmm. I'm inclined to agree with moshe. It's a problem already and a search issue rather than comment specifically. In which case we just need to fix the fails.
Comment #16
catchAll tests should pass now. Because we render the full comment thread in search indexing, some tests which assumed just subject and body were broke, also last patch wasn't returning anything in comment_node_update_index().
Comment #17
catchUpgrade path is working - getting notices in batch.inc but seems unrelated to this patch.
Here's how it looks for performance. Generated one node with 300 comments, displaying 50 per page.
HEAD:
Executed 100 queries in 50.89 milliseconds. Page execution time was 243.59 ms.
ab -c1 -n1000 http://d7.7/node/1
Patch:
Executed 51 queries in 35.04 milliseconds. Page execution time was 297.99 ms.
30% less time in the database. 15% slower overall.
Comment #18
yched CreditAttribution: yched commentedProbably that same old 'field rendering is costly' issue ?
Comment #19
catchVery likely, yes.
Comment #20
catchTidied up some code comments and small change to comment.tpl.php to put the permalink somewhere more sensible.
Comment #21
mlncn CreditAttribution: mlncn commentedHate to be a nitpicker here, especially since I am looking at this patch for an example for converting taxonomy term names and descriptions to fields, but is the compromise of removing comment subjects from the administrative interface necessary?
e.g.
Is this potentially temporary simplicity for the patch, or is code size and performance a serious consideration in pulling this data from fields?
ben, agaric
Comment #22
catchIt's quite possible to imagine a comment with either a subject or a body, so makes sense to have those optional IMO - I might want to allow only video comments, or only one line subjects + ratings, or only bodies.
With term names, I think we'd want to make that a locked field since the term is it's own distinct entity with it's own management interface - can't have a term without a name.
We could potentially do that for comment->subject too, but personally I find the current behaviour of requiring it and filling it in if it's not there pretty horrible in terms of how comment subjects actually look when presented - and I've not seen many other systems with a similar implementation - most just have a single text field, or text field + rating etc. so preventing that (apart from altering the field definition if that allows you to unlock fields) seems like an unnecessary restriction to me.
Comment #23
pwolanin CreditAttribution: pwolanin commentedI'm a little behind still on grasping field and entity APi stuff, but support getting conversions in if possible [i.e. subsbcribe]
Comment #24
Anonymous (not verified) CreditAttribution: Anonymous commentedCatch:
On my end, I'm seeing gremlins in the changes to themes/garland/comment.tpl.php. These need to be submitted as encoded entities?
–
When that happens, I can mark RTBC.
Comment #25
catchHmm, I've seen that before in other patches, and been told it's OK - I guess it's how patch itself interprets those characters. in core, they aren't htmlencoded, so not sure how to deal with that encoding issue.
Edit: figured it out. Firefox defaults to showing patch files in ISO - if you manually change to UTF-8 it'll show up fine as the correct character.
Comment #26
pwolanin CreditAttribution: pwolanin commentedReviewed on the plane - some actual problems and some questions
I should probably read the issue more, but I was confused by the fact that there is no UI to manage comment fields - this is a code-only feature?
$permalink is not documented in the .tpl file doxygen - is that a new variable?
If I uninstall comment module and install it again, there is a problem that the comment form on any pre-exitiong node type seems to be broken - no fields show up. This may well be a field API bug though.
A bug here - in the second line it should be $node->title, not $comment->title I think
minor typo leading to a bug here "comment->$comment->cid":
Also the construction of the fragment is inconsistent - concat vs "" interpolated, though that's possibly exising. Should really be a helper function likely.
The default display settings seem a bit ugly - the field labels show - can we hide them?
If I try to run the update code with no nodes having comments there is an SQL error:
The error is from an empty IN(), so maybe it needs to be checked.
Comment #27
catchUI for comment fields is in #537750: Field UI for comments. Without the UI, you'll just get subject and body by default. I'm hoping placing the UI later will be a 'code slush' patch.
Rest of the review looks good, will re-roll a bit later. I'll look into hiding field labels here, although personally I'd be tempted to make that the default setting for labels.
Comment #28
catchUntested re-roll which incorporates most of pwolanin's review (comment_enable() was missing a node_types_rebuild(), not a field API bug thankfully).
The building of the comment permalink is indeed inconsistent, I think this was probably inherited from the building of the node/$nid/#comment-$cid links in there before. Since it's not introduced here, I opened a new issue for a helper function #570266: Add helper function to generate comment permalinks which since it doesn't change any APIs ought to be possible to get in during code slush cleanup. Having rolled a couple of patches with those links, it's a real pain to type each time.
Unresolved:
label on textfields - is it worth trying to default that to off in general? If not I agree we can try to do that in the instance settings here.
UI for adding fields - this becomes a bit more important with this patch, but we should deal with it in the existing issue.
Comment #30
catchFix stupid syntax error.
Comment #31
catchNow hiding subject and body labels by default.
Comment #33
catchSorry, minus debug this time.
Comment #34
SeanBannister CreditAttribution: SeanBannister commentedsub
Comment #35
pwolanin CreditAttribution: pwolanin commentedLots of improvements, but I still see that there are no comment for fields on existing node types when I uninstall then re-install comment module
Comment #36
yched CreditAttribution: yched commentedupdate path:
- directly casting {node}.type to 'bundle' misses the 'comment_node_type_' prefix currently used by comment bundles
- the instances created in the upgrade path should also have their display settings adjusted (maybe a helper function would be useful here ?)
- the update function doesn't create instances of the 'subject' field if subject was disabled, but the data migration queries will migrate the corresponding values no matter what, thus inserting stale field data for objects that don't have an instance of the field.
- are we confident that a single-step, 'INSERT FROM (query)' migration will not timeout on comments-heavy sites ? FWIW, webchick agreed that turning heavy update funcs into multistep could be accepted as followup (critical) bugfixes.
Comment #37
catch- directly casting {node}.type to 'bundle' misses the 'comment_node_type_' prefix currently used by comment bundles
OK. We can probably use CONCAT there, or just do a query per node-type.
- the instances created in the upgrade path should also have their display settings adjusted (maybe a helper function would be useful here ?)
That makes sense.
- the update function doesn't create instances of the 'subject' field if subject was disabled, but the data migration queries will migrate the corresponding values no matter what, thus inserting stale field data for objects that don't have an instance of the field.
Arghh. That's a good reason to do this per-content type then.
- are we confident that a single-step, 'INSERT FROM (query)' migration will not timeout on comments-heavy sites ? FWIW, webchick agreed that making heavy upgrade funcs multistep could be accepted as followup (critical) bugfixes.
I was split on this one - MySQL ought to be well optimized for INSERT ... SELECT, so doing it in one pass would both be faster for sites without all that many comments, and big sites should be running update.php with drush really - where it'd be also faster than a multi-pass. However it's potentially a bit more risky in that someone can try to upgrade a huge site via the browser with a short PHP time limit, and the multi-pass would save them there.
I can't get update.php to work in HEAD at the moment, which means while the upgrade path worked a couple of days ago, there's no way to test it now - either for other people to test it, or for me to properly re-roll to fix yched's points. Not really sure what to do about that.
Comment #38
catchConverted to a multi-pass update (per content type though, so still the potential for some long queries, but I really think that's an edge case which should be handle by command line updates rather than slowing things down for everyone else). And also set labels to disabled in the update path.
Comment #39
moshe weitzman CreditAttribution: moshe weitzman commentedFYI, time spent in mysql does not count against php timeout. So we should never use batch API when a single (long) SQL query will do.
Comment #40
catchFixed some batch API stuff via yched in irc. Given Moshe's reminder about SQL execution time, we're definitely good on a pass per content type here.
Comment #41
catchAnd yet another typo.
Comment #42
yched CreditAttribution: yched commentedJust removed the hardcoded 'zxx' strings in favor of FIELD_LANGUAGE_NONE in tests.
Comment #43
yched CreditAttribution: yched commented- Fixed a typo in comments (Hide field lables by default)
- Fixed some flawed 'multipass' logic in comment_update_7008
- Added: drop comment.format column at the end of the update.
Comment #45
catchRe-rolled.
Comment #47
catchStraight re-roll of yched's patch from #43 again to see where we are with test failures.
Comment #49
catchMoving to D8.
Comment #50
mlncn CreditAttribution: mlncn commentedRDF wants this, marking as not quite dead yet.
Comment #51
scor CreditAttribution: scor commentedrdf.module could save some logic if we were using fields in comment. I've rerolled the patch and tried it successfully with the RDF module (no comment specific logic needed in rdf.module). comment.test remains to be fixed.
Comment #52
scor CreditAttribution: scor commentedd.o returned an error. trying again with the patch.
Comment #53
alippai CreditAttribution: alippai commentedComment #55
webchickI guess I'm fine going forward with this in D7 if Dries is, but IMO it should be possible to RDF-enable fields even if they don't go through field API. For example, Profile module's fields are not going to be converted to Field API this release, it's looking like. There will also be contributed modules that don't embrace the field API convention but will still want to leverage RDF. So from this perspective, Comment module could serve as an example of how to get RDF working for non-Field API fields?
Comment #56
catchI think we can make this patch a fair bit simpler by doing body but not subject - we didn't really come up with a solution to having both body and subject optional (although it makes sense for one of them to be optional on most sites) in terms of administration etc. Comment body as field would just be a straight conversion though.
Comment #57
mlncn CreditAttribution: mlncn commentedSpeaking for the RDF(a) effort, we have strong interest in converting comment body to a field in the interests of avoiding special casing comments with workaround code (which scor will be explaining in greater detail) even though, rest assured, the RDF API can and does apply to arbitrary non-FieldAPI field-like things (and here we come to understand that Drupal has more concepts than there are words in English).
Comment #58
scor CreditAttribution: scor commentedAt present the rdf module does not deal very well with the comment body because it is not a field and therefore inconsistent with the other entities (node for example has its body as a field). The user bundle with profile.module is an exception, but profile.module is better than comment because it has the concept of fields (they are just not Field API fields). Profile has tpl files in which we can potentially add RDFa attributes. But the body of comment has no such tpl wrapper that we can use. Let's look at an example. I added a field to a comment bundle to illustrate the difference between a real field and the current comment body. This is the code it produces
Comment body lacks a proper HTML wrapper to which we can attach RDFa markup. There might be way to work around this by adding markup to #prefix and #suffix, but would feel like a hack since the Field API would give that for free. Right now the RDFa markup (property="content:encoded") is in the top HTML tag because there is no other wrapping tag to use. The consequence of this is that the RDFa in the fields of comment are being ignored by nature of the RDFa processing rules: any attribute property means that the content of the tag is considered a literal (plain text or XML/HTML), hence the reason why we're trying to put the RDFa markup in a parent tag which is as close as possible of the content we want to annotate.We can specially modify comment body for RDFa, do ugly workarounds, or simply make it a Field proper.
Catch suggested to scale down the patch to just body as field and that would be enough already for the rdf patch to deal automatically with the comment body. Doing comment body is UI cleanup - since it's inconsistent with node at the moment.
EDIT: It would be great to have Dries' thumbs up for this before anyone invest time working on the patch.
Comment #59
webchickI spoke with Dries this evening. The gist is that common-sense clean-ups like this are okay, but it's critical that we do not further degrade performance. So please accompany patches with benchmarks.
Comment #60
yched CreditAttribution: yched commentedfix title ;-)
Comment #61
scor CreditAttribution: scor commentedPatch updated to only convert comment body to a Field. I haven't fixed the tests yet, but Drupal installs and you can create comments without blowing the site up! I've left some commented bits of patches here and there, but will clean that up after a good night sleep. For now, just curious to see how the bot will digest that.
Comment #63
effulgentsia CreditAttribution: effulgentsia commentedsubscribing
Comment #64
Anonymous (not verified) CreditAttribution: Anonymous commentedI may have stumbled upon one of the issues with the patch.
To see the form that Drupal was receiving, I output the return value of the call to drupalGet on line 34 of comment.test. In the form, I noticed that the textarea name was set to 'comment_body[zxx][0][value]' instead of 'comment'.
When I changed the array key from 'comment' to 'comment_body[zxx][0][value]' in the test on line 27, I went from 47 fails to 2 fails.
Comment #65
Anonymous (not verified) CreditAttribution: Anonymous commentedAdded to the last patch:
parent::attachLoad($comments);
With these changes, Anonymous Comments, Comment Approval, and Comment RSS tests now pass. There are still 77 fails in the other 4 comment tests.
Comment #66
Anonymous (not verified) CreditAttribution: Anonymous commentedRemoved the changes to theme_comment_block() that were in the patch because they do not reflect what is currently in core and are giving errors.
Comment #67
Anonymous (not verified) CreditAttribution: Anonymous commentedCorrected the variables used when truncating the comment to create the subject in comment_submit().
This now passes the Comment Block tests, which leaves 3 tests still failing.
Comment #68
Anonymous (not verified) CreditAttribution: Anonymous commentedCorrected variables in comment_preview and in the interface test based on comment 64.
This passes the Interface and Paging tests, so just the Preview test to go in the Comments test group.
Comment #69
Anonymous (not verified) CreditAttribution: Anonymous commentedCorrected a bunch of variables left in the test file based on comment 64.
This now works for all Comment tests, as well as RDFa and Trigger.
Still fails for Tracker.
Comment #70
WorldFallz CreditAttribution: WorldFallz commentedand the bot says....
Comment #71
Anonymous (not verified) CreditAttribution: Anonymous commentedVariables corrected in trigger and tracker tests as per comment 64.
Rerolled to work with latest version of head.
Comment #73
catchCan't do this:
+ $query->join('field_data_comment_body', 'b', 'c.cid = b.entity_id');
Because field storage is pluggable. Instead any code checking for $comment->comment needs to look for the field location $comment->field_body[FIELD_LANGUAGE_NONE][0] etc.. If there's a direct query on the {comment} table including comment body, that needs to be converted to comment_load_multiple().
Comment #74
scor CreditAttribution: scor commentedFixed comment_node_update_index() to include $mode and $comments_per_page. Fixed search.test to account for the new comment body. The previous test was searching for comment title although the search keyword was the body of the comment. I'm not sure why but with comment body as field, the subject of the comment does not appear entirely but only the end. In general, if the title is too long, it won't necessarily appear completely in the search results. So instead of expecting the comment subject for a search of the body, I have split the test into two searches, the first for the subject (checking the subject appears) and the second searching for the body and checking it appears (plus other body full HTML tests).
Comment #75
WorldFallz CreditAttribution: WorldFallz commentedkicking the bot again...
Comment #77
Anonymous (not verified) CreditAttribution: Anonymous commentedThanks for pointing out the error, catch.
I have used comment_load_multiple.
EDIT: I need to reroll this real quick
Comment #78
Anonymous (not verified) CreditAttribution: Anonymous commentedOk, this time for real.
The edit from the last patch is at line 82 of comment.admin.inc.
Comment #79
NaheemSays CreditAttribution: NaheemSays commentedI assume this needs testing?
Comment #81
Anonymous (not verified) CreditAttribution: Anonymous commentedSince the last patch was uploaded, a new comment schema update was committed to comment.install, so needed to update the numbers on the schema.
This rerolled patch should apply cleanly, but the Search Engine Ranking test that failed after #74 continues to fail, so this still needs work.
Comment #82
WorldFallz CreditAttribution: WorldFallz commentedComment #84
scor CreditAttribution: scor commentedcomment_update_7012() was defined twice in comment.install. rerolled.
I did some basic performance benchmark like catch did at #17. (Generated one node with 300 comments, displaying 50 per page.) but could not find any noticeable impact with this patch (4.82 #/sec for both). I'd like someone else to confirm that please :). devel generate does work for D7, except you might have to retry a few times until it creates the nodes/comments for real. (It worked for me with 10 nodes and 300 comments).
Comment #86
scor CreditAttribution: scor commentedfixes the search ranking test and removes some trailing spaces.
Comment #88
scor CreditAttribution: scor commentedfixes the remaining failing tests in field_ui.test and forum.test.
Comment #89
scor CreditAttribution: scor commentedignore these
comment_body['zxx']
which need to be replaced with the right variable. Nevertheless this patch is ready for reviews and benchmarks. Note that I ran some benchmark tests but could not see any performance impact with this patch, but I'd like someone else to confirm this.Comment #90
scor CreditAttribution: scor commentedshame on me for not setting the right permission for the anonymous users to access the comments, that's lame (thanks catch for pointing this out). some quick benchmarking with 300 comments, 50 displayed and ab -c 1 -n 100 gives me 2.34 request / sec without the patch and 2.67 request / sec with the patch applied. I don't want to draw any conclusion without someone else with more benchmark experience confirming this.
Comment #91
catchAs before PHP performance is degraded, number of queries reduced due to skipping the separate filter cache for text fields. So that doesn't help non db-caching sites, although they'd also be getting the entire comment body from memcache instead of the db, so maybe it'd help a bit.
Personally I think the results are OK enough that potentially scalability improvements from using field API down the line cancel it out, and we should be focusing on #438570: Field API conversion leads to lots of expensive function calls and other bottlenecks rather than keeping stuff like this out because Field API is slow - that's a bad situation to be in with Drupal 7's most touted feature.
Devel output: (anonymous user with access to see devel stuff - ignore everything except the query counts since the timings vary significantly between requests).
HEAD:
Page execution time was 492.52 ms. Executed 109 queries in 47.05 milliseconds.
Page execution time was 546.5 ms. Executed 58 queries in 61.62 milliseconds.
ab -c1 -n1000 node/2
HEAD:
Patch:
Comment #92
catchCross posted with scor, looks like his come out slightly faster with the patch - that may be well be down to the specs of our respective machines and how MySQL is set up on each, either way it shows the performance impact isn't overly bad here.
Comment #93
scor CreditAttribution: scor commentedI reran some benchmarks again, following this procedure:
1. install HEAD. enable devel generate and create one node with 300 comments. allow anonymous to access comments. benchmark /node/1.
2. patch HEAD with #88. run update.php (there should be 2 updates). benchmark /node/1.
this procedure should be fair since it ensures we're benchmarking with exactly the same set of comments.
HEAD without patch:
HEAD with patch:
that's an overall 5 % performance loss.
Here are devel output (by anonymous user):
- cold caches
HEAD without patch: Executed 238 queries in 109.61 milliseconds. Page execution time was 585.52 ms.
HEAD with patch: Executed 139 queries in 139.77 milliseconds. Page execution time was 652.35 ms.
- warm caches
HEAD without patch: Executed 106 queries in 35.06 milliseconds. Page execution time was 368.53 ms.
HEAD with patch: Executed 55 queries in 22.02 milliseconds. Page execution time was 383.7 ms.
Comment #94
yched CreditAttribution: yched commentedIt seems the patch still has parts where $comment->subject is replaced by $comment->cid or $comment->user in displayed messages, administrative lists, etc.
That was needed in a previous state of the patch where 'Subject' was moved to a 'field' too, but given that the scope has been reduced to only comment body now, are those changes still needed ?
Comment #95
yched CreditAttribution: yched commentedOther remarks:
Shameless plug: #613630: Remove prefix on comment bundles names removes those 'comment_node_' prefixes, they are not needed anymore. Needs someone to RTBC.
They should stay in this patch until that other one is committed, of course.
I don't get why we bother with both $comments (fetched from a direct query) and $comment_objects (loaded by comment_load_multiple()). If we need to load the full comments anyway, why not have the query select only the relevant cids, and pass them to comment_load_multiple() ?
The official name of the param in hook_update_N() is $sandbox. $context is for generic batch API callbacks, update functions have an extra layer.
(node_update_7006 is wrong and should be fixed separately).
+ No more $ret arrays in update functions now.
Don't return anything, and set '#finished' in $sandbox['#finished'].
Should I stay or should I go ? ;-)
I'm on crack. Are you, too?
Comment #96
Anonymous (not verified) CreditAttribution: Anonymous commentedI was reviewing this today, I had thought the same thing and couldn't remember why we hadn't just pulled cids. But I think we do need to both query from the database and load the full comment.
We need things besides cid in the query because:
We can remove the join with the users table if we pass in the comment_object instead of comment to theme_username, though.
I will be posting a patch with that change shortly.
Comment #97
Anonymous (not verified) CreditAttribution: Anonymous commentedIn this patch:
Still needs to be done:
Going to kick the bot to make sure this passes as expected.
Comment #98
yched CreditAttribution: yched commentedre #96:
Right, I forgot about the tablesort.
Then the query should only include the columns needed for the tablesort.
should be
->fields('c', array('cid', 'subject', 'name', 'changed'))
The actual comments data should be loaded by comment_load_multiple() from the cids. Less data flying around, no more $comments / $comment_objects confusion.
The arrays collecting cids to load should be names $cids instead of $cid, which is confusing.
Also, I still wonder about #94.
Comment #99
Anonymous (not verified) CreditAttribution: Anonymous commentedPatch includes:
Still needs to be done:
Comment #100
scor CreditAttribution: scor commented@linclark: you can ignore this permission glitch, it was just necessary to run the benchmarks.
I think we haven't determine whether we need this or not. Can someone familiar with the Field API enlighten us? (this was commented out in #65).
zxx needs to be FIELD_LANGUAGE_NONE
This review is powered by Dreditor.
Comment #101
scor CreditAttribution: scor commentedfix the remaining bits of [zxx].
Comment #102
yched CreditAttribution: yched commentedScor asked me to give this another round of review.
Aside from my comment in #98 (i.e second point in #95), here are my remarks:
This is only valid if we allow fields to be restricted to some entity types.
Short of that, you cannot be sure that the 'comment_body' field has not been reused on other entities than comment.
I do think we need that API addition anyway, because having the 'comment body' field show up in the 'add existing field' UI in nodes will be pretty confusing.
I don't mind keeping the current code for now, but then this feature becomes a critical followup.
Could we just give it another shot - things *might* have gotten better meanwhile ?
I'm not sure I get this.
Nitpick: 'label' (singular) might be more accurate ?
Hm - on all node types ? or only the ones with comment enabled ?
[edit: all node types, because some might have comments but with comments disabled afterwards, and we don't want to lose that data. I think this deserves a comment][edit: forget about this, I'm on crack - code is fine]
not needed
could be shortened to
Should I stay or should I go ? ;-)
I'm on crack. Are you, too?
Comment #103
scor CreditAttribution: scor commentedrerolled the patch since it didn't apply any longer. kicking the bot to make sure all tests are passing.
from #98
removed status which was still in the latest patch. The other comments from #98 should have been addressed by #99, i.e. all comment data is loaded from comment_load_multiple(), except the cids of course.
still need to work on the other comments from yched.
Comment #104
scor CreditAttribution: scor commentedAll of yched points have been addressed.
Let's follow up in a different issue since this feature should be available and is not comment specific. This patch has been waiting here for a long while...
no luck. It works during a normal install but not during testing.
field_info_field_types() returns an empty array during the tests. This sounds like a bug in testing which should be fixed in its own issue.
I improved the comment. It also works without having an explicit node_types_rebuild() in hook_enable() but that's because node_types_rebuild() is called after enabling the module. Not sure we should remove it though since it's there for consistency (create field, then create instances).
Comment #105
yched CreditAttribution: yched commentedI'm OK for investigating this in a separate issue, but the comment doesn't seem accurate - dependencies between entity_get_info() / taxonomy_install() ?
- The error you mention (Attempt to create a field of unknown type text_long) seems related to _field_info_collate_types() cached info, not entity info.
- there's no taxonomy_install() ;-)
Thanks for the refactoring, much clearer now. Last nitpick: I'd suggest naming the var $row or $record instead of $comment, the query doesn't return actual comment objects.
Sorry to be blunt but I still don't get this.
If comment.module gets enabled months after the initial install (or gets disabled and re-enabled months after), then a node_types_rebuild() won't trigger a hook_node_type_insert() because there are no new content types on the site.
If the code that needs to run on comment "module being enabled" and on "node type being created", then I'd suggest calling the same helper function in both hooks, but not artificially firing one hook from the other.
This review is powered by Dreditor.
Comment #106
scor CreditAttribution: scor commentedThanks yched, we're almost there ;) I've fixed all your comments. There is now a helper function to cover your last concern.
Comment #107
yched CreditAttribution: yched commentedHm, I don't think we want to remove the call to f_a_create_bundle() ?
I'm on crack. Are you, too?
Comment #108
scor CreditAttribution: scor commented@yched, it's not removed, it's moved with the rest in _comment_body_field_instance_create():
Comment #109
yched CreditAttribution: yched commentedOops, right. Well, I don't think that's correct. Creating the bundle for comment-on-some-node-type does not belong to a function whose advertised role is to create a field instance. It does exactly belong to hook_node_type_insert() ;-)
Comment #110
scor CreditAttribution: scor commentedoh, right, I've moved it back in comment_node_type_insert().
Comment #111
yched CreditAttribution: yched commentedThanks ! RTBC, then :-).
Comment #112
scor CreditAttribution: scor commentedbumping this issue, it's been RTBC for a few days now. Since it changes comment body into field, we should get this in before any beta comes out. (Note that this is also the condition for having proper RDFa support for comments and streamlines another old built in field cruft...)
Comment #113
webchickDries, I'll leave this one to you. This is a pretty major API change, but if you are okay with it, given your knowledge of RDF stuff, I won't squabble. :)
Comment #114
effulgentsia CreditAttribution: effulgentsia commentedI'm joining late; I think in principle this is awesome, but I haven't reviewed the code in detail. I wanted to generate some new benchmarks, but ran into errors.
1. Patch doesn't apply and needs a reroll.
2. I tried to reroll, but then ran into errors. Perhaps caused by:
Is this correct? If so, should there also be a removal of 'subject' from hook_schema()?
This review is powered by Dreditor.
Comment #115
scor CreditAttribution: scor commented@effulgentsia right, this piece had nothing to do there. Rerolled patch without it since we do not touch the comment subject. Applied the patch on a working HEAD with node and comment content and ran update.php with no problem. rerolled patch so you can go ahead with your benchmark.
Comment #116
scor CreditAttribution: scor commentednow with the new LANGUAGE_NONE constant.
Comment #117
scor CreditAttribution: scor commentedcreated #652246: Optimize theme('field') and use it for comment body as follow up issue for when this is in.
Comment #118
mlncn CreditAttribution: mlncn commentedHuman tested on articles, forum, and custom content; and with tracker. Functions as before, and makes RDFa goodness easier (and also makes comment body play well with custom comment fields).
The comment subject appears below the comment body (and also the body is labeled "body" instead of "comment'), but if no one has complained about this in 117 comments let's just pretend i didn't bring it up. That can be done in a UI fix after this gets in. (Note: If it cannot be corrected by way of what is passed through field_attach_form, subject and author can be manually weighted -1 and -2 respectively and the form looks as it looks for comments pre-patch.)
benjamin, agaric
Comment #119
scor CreditAttribution: scor commentedcomment_update_7013() was not dropping the column comment and format after moving the data to field, I've added these to the patch. Also fixed the form element ordering pointed by Benjamin. See comment form as logged in user and anonymous user.
I also ran some benchmarks which showed 6% performance impact on my localhost:
HEAD 3.28 #/s - 308ms
patched 3.10 #/s - 322ms
Comment #120
mlncn CreditAttribution: mlncn commentedOh, wonderful! I do feel better now.
Comment #121
effulgentsia CreditAttribution: effulgentsia commentedOn my computer, I see a 19% performance degradation for an anonymous user with permission to view comments viewing an article node with 50 comments (200ms vs 238ms). That matches what catch saw in #91. However, as catch points out in that comment, the numbers are very sensitive to machine specs, since there's both rendering factors and db query factors, so I'm not surprised that scor is seeing only a 6% change. My procedure was to install HEAD, use devel_generate to make a node with 50 comments, benchmark, apply patch 119 and run update.php, and benchmark again.
This is a frustrating situation. On the one hand, I think this is an architecturally awesome thing to do, and my preference would be to see it land, because on any site I work on, I can use the drupal_render() cache to optimize comment display, and if on a particular site I couldn't, I would scale up hardware, and would see that as a good trade-off for getting the benefits of this patch. And I agree with scor that if it's to be in D7 at all, it should land before the first beta. And some very smart people are working on improving the performance of Field API: #438570: Field API conversion leads to lots of expensive function calls. On the other hand, we're a community, and even a 6% performance impact (and certainly a 19% one) will upset a significant portion of the community. Note Moshe's comment when a 6% degradation was introduced for contextual links: #607244-8: Decrease performance impact of contextual links. And that was for something that we knew could be turned off by people who care more about performance than features (as of yesterday, it's now a module that can be disabled). This patch, however, is not optional. If it lands, it cannot be disabled.
Tough decision, and I'm glad I'm not the one that needs to make it. Dries: good luck :).
Comment #122
chx CreditAttribution: chx commentedObviously no. Six or nineteen , too much.
Comment #123
scor CreditAttribution: scor commented@effulgentsia: interesting results. What system do you use? For what it's worth, I ran another series of benchmark on a basic site5 hosting server. I installed HEAD on a subdomain and generated 10 nodes with 150 comments max. I cloned the db onto a second subdomain, applied the patch and ran update.php. In order to account for the fluctuating server load, I used a script to alternate the ab command from one site to the other:
I parsed the results and the poorest average result I got was still less than 8%. The sites are public so feel free to check them out.
EDIT: @effulgentsia catch's results in #91 are closer to 11% than 19% - though I admit either is still high :(
Comment #124
scor CreditAttribution: scor commentedAnd if comment body is not meant to become a field, why having fieldable comment to begin with if core doesn't even take advantage of it?
Comment #125
yched CreditAttribution: yched commentedre #123: 8% for a page displaying
150 fields[edit: er, more probably 50 fields, since default comments per page is 50 - my remark still applies though] sounds within the realm of the optimization brought by #652834: Field formatters as render arrays (see moshe's benches in #438570-60: Field API conversion leads to lots of expensive function calls).Would be great if you could bench HEAD vs "this patch + #652834: Field formatters as render arrays", see what we come up with.
Comment #126
effulgentsia CreditAttribution: effulgentsia commentedYou're right. Also, I realized that after running update.php, if I also clear drupal caches (admin/config/development/performance), then the performance on my machine (MAMP) improves to only 10% degradation (I keep forgetting that update.php doesn't empty drupal caches any more). So there's less machine sensitivity than I thought, and our benchmarks are compatible with each other: awesome!
I'm about to test out the impact of #652834: Field formatters as render arrays and will post the results.
Comment #127
effulgentsia CreditAttribution: effulgentsia commentedI'm actually seeing degradation with #652834: Field formatters as render arrays:
HEAD: 205ms
Patch 119: 225ms
Patch 119 + Patch 652834: 227ms
Let's continue work on that issue, and post back here when it successfully optimizes comment body to within acceptable levels.
Comment #128
effulgentsia CreditAttribution: effulgentsia commentedAlmost all of the performance difference can be traced down to theme('field'). It's a template file, and those are slow. If you add
unset($variables['content']['comment_body']['#theme']);
at the end oftemplate_preprocess_comment()
, then it removes the calls to theme('field') and the performance impact of the patch drops to 2%, which can probably be optimized even lower. I'm not saying we necessarily want to do that, though I think doing that would be preferable to not having the patch land at all, but there's probably something we can do to make comment_body special and get themed as a field in a more optimized way (or maybe even optimize theme('field') generically). I'll post more thoughts here when I'm able to work on it again.Comment #129
moshe weitzman CreditAttribution: moshe weitzman commentedI've looked a bit at template flow and didn't find a way to speed it up. Hope you guys can.
Meanwhile, maybe we turn 'field' theme hook into a function instead of a template? Anyone who wanted template way could do so with hook_theme_regestry_alter, or just call out to a template from a theme function. I agree that this is far preferable to shying away from making stuff like comment body into a field.
Comment #130
effulgentsia CreditAttribution: effulgentsia commentedHere's a patch that's identical to #119 except for this extra hunk:
This results in this patch being faster than HEAD. On my computer, the anonymous user viewing an article node with 50 comments (note that HEAD number is slightly different than in #127, because it was a new install with a new run of devel_generate, so different data in each comment):
HEAD: 207.9ms
This patch: 206.0ms
@scor: please post benchmarks from your system.
This patch results in comment body being a field with respect to everything storage related, and this results in fewer database queries (no need to grab cached check_markup() from cache_filter table, because it's part of the cache_field data). But it mimics HEAD in terms of not being a field with respect to the render array returned by comment_build_content(): we avoid the call to theme('field') and two calls to drupal_render() per comment that is otherwise introduced by conversion to a field. This also results in no change to HTML output relative to HEAD from this patch.
This lets us have the issue land (no downside relative to HEAD), and then we can decide whether to work on field rendering optimization, some other optimization for the comment body field, or leave as-is. The RDF use-cases may inform the direction needed with respect to that.
Thoughts?
Comment #131
effulgentsia CreditAttribution: effulgentsia commentedAlternatively, we could replace the new hunk in #130 with this instead:
which has the advantage of retaining the field render structure, doesn't mess things up if someone makes the body field into a multi-valued field, and allows someone to implement hook_comment_build_alter() to add #theme='field' back in if they're ok with the performance hit.
However, it results in a net performance impact in the patch of about 1% relative to HEAD. @scor: can you check that number on your system too?
Again, neither #130 nor this need to be permanent solutions, but is there anything we can agree on that would allow this issue to land quickly, be removed as a beta-blocker, and allow further refinements to proceed at whatever pace they need to?
Comment #132
catchI like #131 for this - if we want this in at all (and I think we do, it's only an API change in relation to $comment object structure, but it's a massive win for consistency with nodes) then needs to be really soon.
Field performance is a separate, critical issue. Any inconsistencies this approaches introduces compared to other fields are nothing compared to it not being a field, and we may want to look at removing the template by default anyway (a field_templates.module or base theme can easily put them back). However doing this allows us to remove interdependencies between what are very different issues.
Note that we recorded in the region of a 30-40% hit with node body, only some of which has been removed by now (and mainly those database queries which are cache hits anyway), and committed anyway in the hope of optimizing it later, it's only as we convert more things, that we find more optimizations to be made.
Comment #133
effulgentsia CreditAttribution: effulgentsia commentedOk, this patch has #131 flushed out. Here's the relevant hunk:
Setting this to RTBC, because we need this in front of Dries, and he can kick it back if he doesn't like it. I'll follow up with benchmarks from my system. @scor, @catch: please post your benchmarks as well.
Comment #134
effulgentsia CreditAttribution: effulgentsia commentedBenchmarks from my system attached for viewing an article node with 50 comments.
HEAD: 207.7ms
Patch 133: 208.9ms
Impact of patch: 0.5%
Comment #135
scor CreditAttribution: scor commentedOMG! Thank you effulgentsia! #133 shows a 1% performance improvement compared to HEAD on my machine! :)
HEAD: 309.4 ms
patch: 306.2ms
Comment #136
moshe weitzman CreditAttribution: moshe weitzman commentedVery nice work. Patch gets +1 from me.
Comment #137
yched CreditAttribution: yched commentedVery interesting. In #438570: Field API conversion leads to lots of expensive function calls we tried several tracks to speedup field rendering (reduce the number of template suggestions, use a theme function rather than a template) and none seemed to bring any speedup.
I agree that the fix is good enough to let 'comment body as field' patch get in.
Two drawbacks:
- field UI display options for label ('hidden', 'above', 'inline') are overlooked - label is not displayed no matter what the setting
- the field theme also takes care of displaying RDF properties, so the patch wipes these of for the 'comment body'. Streamlining RDF handling was the primary reason for 'comment body as field' :-p
Comment #138
effulgentsia CreditAttribution: effulgentsia commentedI agree that #133 is not the end of this issue, and that RDF use-cases and work on field rendering optimization will impact what comes next.
But I'm glad that we seem to be in agreement that #133 is good enough to land and have the "beta blocker" tag removed once it does.
Comment #139
scor CreditAttribution: scor commented@yched, yes I noticed the field markup was not around the comment body anymore, but we'll follow up on that in #652246: Optimize theme('field') and use it for comment body while keeping performance in mind - at least we're making progress here by having comment body as field, which is the whole purpose of this issue. The real beta blocker was to move from custom comment body to proper field and we have it now in #133 RTBC.
Comment #140
scor CreditAttribution: scor commentedrerolled patch from #133.
To summarize the situation, we now have a RTBC patch with no performance impact on HEAD. The patch is an API cleanup but it's also a major API change... that means it should probably be committed asap :)
Comment #141
effulgentsia CreditAttribution: effulgentsia commentedI opened a separate issue for optimizing theme('field'): #659788: [meta issue] theme('field') is too slow, which is inhibiting its more widespread use
Comment #142
Dries CreditAttribution: Dries commentedPatch no longer applies -- could we do a reroll please? Thanks! :)
Comment #143
scor CreditAttribution: scor commentedhere it is!
Comment #144
Dries CreditAttribution: Dries commentedI don't understand this part yet. Why would we attach an instance of comment_body to each node type?
Nice to do, but we don't explain what the hack does, or why it makes things faster.
Spacing is incorrect. There are many other instances of this.
This review is powered by Dreditor.
Comment #145
scor CreditAttribution: scor commentedDuring the upgrade, there might be node types with comments disabled but which had comments enabled in the past. We don't want to lose these comments and we need a place to store them, so as a precaution we attach an instance of comment_body to each node type. I added a comment in comment.install to make this more clear.
we're basically bypassing the theme('field') step to gain performance (it was proven theme('field') is the main bottleneck in this issue). Note that we will still need a wrapper around the body field, and that's why I created #664602: Wrap comment body in a tag. There are also work going on other issues to improve this, see #659788: [meta issue] theme('field') is too slow, which is inhibiting its more widespread use. @effulgentsia, could you reroll the patch and elaborate on this in your comment?
fixed.
EDIT: the right patch is in the next comment #146.
Comment #146
scor CreditAttribution: scor commenteduploading the right patch this time.
Comment #147
effulgentsia CreditAttribution: effulgentsia commentedI have not reviewed #146. I'm hoping someone more familiar with this issue can comment on the discussion and changes since #142 and get this issue back to RTBC status relatively soon. This patch is just a re-roll of #146, but with a change to the comment above the code that unsets $comment->content['comment_body']['#theme'].
The new @todo links to #659788: [meta issue] theme('field') is too slow, which is inhibiting its more widespread use instead of to this issue. There's a patch on that issue that offers a very modest performance improvement to theme('field'), but I'll be using that issue to also link to other issues that optimize other things that result in theme('field') becoming faster. For example, that issue already links to #660856: Optimize template_preprocess(), and I'm hoping to add another optimization issue and patch later today. I do believe we'll get to an acceptable level of performance, and will be able to use theme('field') for theming the content body before D7 release. Having this issue land will actually be very helpful, as it will provide the necessary use-case for benchmarking those optimization issues.
Comment #148
scor CreditAttribution: scor commentedThe patch hasn't fundamentally changed since it was set from RTBC to "needs work" by Dries in #142. I simply rerolled it to keep up with HEAD, and it's basically the same as #140/#133 which was RTBC'ed 2 weeks ago.
Thanks effulgentsia for the thorough explanation in comment.module, which I can't help pasting here as it highlights the current situation of this issue:
Comment #149
Dries CreditAttribution: Dries commentedIt seems like the name comment_body is inconsistent with other field names in comment, but also inconsistent with the node body name.
Comment #150
effulgentsia CreditAttribution: effulgentsia commentedWe need testbot back before this patch can land, but we can keep discussing outstanding issues, to help this move quickly once it is back.
Comment #151
scor CreditAttribution: scor commentedThis was implemented by catch in #3 and has been kept since then. Hopefully catch can chime in and tell us if there strong reasons why he implemented it like that at the first place.
Comment #154
catchI originally added comment body to each comment bundle because I was converting both ->subject and ->comment to fields, and they were both optional - in HEAD we allow subject to be optional, there are plenty of use cases (like video comments) where you might want body to be optional. If neither instance was created by default, we'd ship with empty comment forms by default which would seem a bit wierd - considering nodes have at least a title field.
However, now we're not doing subject here, there's less of a reason to create it automatically.
However I think it makes sense to do it in the upgrade path, since sites upgrading from D6 shouldn't have to go in manually and create fields. The issue with per-content-type settings is that comments enabled or disabled is only a default, you can go in and change this, in either direction, for any individual node.
Comment #156
scor CreditAttribution: scor commentedPatch updated according to the changes #658364: Does build/view/formatter terminology make sense? introduced.
Comment #157
effulgentsia CreditAttribution: effulgentsia commented#157 changes the code comment here and adds an @todo.
#157 also adds an @todo here to match the @todo added to comment_enable() above.
#157 cleans up this code comment a little.
#157 changes $edit_comment to $edit and initializes it before adding to it.
#157 removes this hunk, because I don't think it's needed (we'll see what bot says). The comment body field is added to comment bundles, not to node type bundles, so it shouldn't interfere with node type field ui screens.
I'm setting this to RTBC, because it's just code comment cleanup and a tiny code cleanup compared to #156, which passed bot and was a cleanup of an already RTBC patch.
@Dries: regarding points you raised in your prior reviews, are you satisfied with the updated explanation and @todo for the code you were questioning in comment_enable()? Are you okay with 'comment_body' as the field name, or would you prefer 'comment' or some other name (see #150)?
Comment #158
effulgentsia CreditAttribution: effulgentsia commentedThis is a re-roll of #157 due to HEAD changes. Plus I changed the instance label of the comment_body field to 'Comment' instead of 'Body'. I did this because having it as 'Body' causes a test failure on field_ui.test line 180, which #156 and prior had fixed by changing the assertion, but I removed that hunk in #157 (see last code block reviewed in #157 comment), because asserting on a full span seems too prone to false negatives (what if field_ui module changes the class used in that span, then the assertion won't trigger even if deletion of the body field fails). It does seem unfortunate that the assertion as-is is so prone to false positives (having any other field with a label of 'Body' causes the assertion to trigger even if the intended body field got deleted successfully), but that should be addressed in a separate issue, at which time the 'Comment' label can be changed back to 'Body' if that's what we want (though I'm not convinced that 'Body' is the label we want to use).
This patch passes Comment, Field API, and Field UI tests. I don't know if it passes all tests, because bot is down, and my computer is too slow at running the full test suite.
Since the change of instance label is so trivial and can be changed again after the patch lands without really affecting anyone, I'm leaving this as RTBC.
Comment #160
effulgentsia CreditAttribution: effulgentsia commentedFrom webchick's D7 alpha1 announcement:
It would be such a shame to not see this make it for D7. What's needed for this to land before 1/15?
Comment #161
axyjo CreditAttribution: axyjo commentedMinor, but is that newline necessary?
Besides that, applies on my copy and tests pass on my machine as well. Great job!
I'm on crack. Are you, too?
Comment #163
scor CreditAttribution: scor commentedrerolled the patch with axyjo's comment (removed a couple of other newline hunks too).
Comment #164
scor CreditAttribution: scor commentedrerolling to keep up with HEAD. What's blocking this from being committed before the alpha release? Proper RDFa support for comments is dependent on this. Dries, webchick?
Comment #165
effulgentsia CreditAttribution: effulgentsia commentedI second the request to have this land, or for an update as to what's stopping it from landing and/or if/when we can expect it to land. Whether or not this lands will have impact on other work (RDF, theme('field') optimization, and other things), so given that alpha1 is approaching fast, an update on what the plan is for this issue would be extremely helpful. Thanks.
Comment #167
scor CreditAttribution: scor commentedcannot reproduce this failure on neither MAMP (MySQL 5.1) nor my local PIFR 2 configuration vbox, which both give me:
DBLog functionality 414 passes, 0 fails, and 0 exceptions
reuploading patch.
Comment #168
webchickMargh! :( I lost my comment! :( I said something like this...
I wanted to talk to Dries before going ahead with this, since this was his baby, and got a chance to do that today. We both agreed that this should be committed ASAP, though it'll be a hugely disruptive change. Ugh. :\
The code looks well-documented, esp. that wall of text about the theme('field') stuff. ;) I didn't thoroughy review every hunk, since testbot would be screaming bloody murder if something was terribly amiss, but the changes look consistent with what we saw on node body/title conversion. I also played around with it for a bit and was unable to break it.
Committed to HEAD. Let the avalanche of follow-ups begin. ;)
Marking needs work for documentation.
Comment #169
Anonymous (not verified) CreditAttribution: Anonymous commentedWhen running the database update this morning, I received this message.
Warning: Division by zero in comment_update_7013() (line 318 of /Users/linclark/Sites/drupal/modules/comment/comment.install).
Comment #170
scor CreditAttribution: scor commentedThank you Webchick for finally getting this in!
@linclark: tried to upgrade from HEAD of 2 days ago but could not reproduce. The code at line 318 is
Could you create an issue on this and give instructions to reproduce?
Comment #171
Anonymous (not verified) CreditAttribution: Anonymous commentedI think I was working from yesterday's HEAD.
It didn't affect the functionality of the site, the comment tests are still passing, so it shouldn't be a problem if other people aren't getting the same warning.
I'll post if I can reproduce.
Comment #172
yched CreditAttribution: yched commentedFix for the (harmless) "Warning: Division by zero in comment_update_7013()" error reported in #169. Happened when there was no comments to process.
Comment #173
webchickOn that note, when you go to admin/content/comment without any comments, you also get the following notice:
Could we fix both of 'em up here?
Comment #174
aspilicious CreditAttribution: aspilicious commented@webchick ==> #679960: Add test for "Notice: undefined variable cids" when there are no comments available
Comment #175
yched CreditAttribution: yched commentedIf the other bug has it's own thread, followup fix for the update in #172 is back to CNR.
Comment #176
fgmAnother issue might be related: #679936: Remnants of comment information when module is disabled
Comment #177
yched CreditAttribution: yched commentedRelated followup: #680910: Allow a given field to be restricted to some entity types.
Comment #178
webchickOk, cool. I committed the fix in #173, since the rest are documented elsewhere.
Moving back to "needs work" for documentation.
Comment #179
yched CreditAttribution: yched commentedFYI - patch to enable a UI for fields on comments: #537750: Field UI for comments
Comment #180
sunProblem: The comment body is no longer required. The only talk about required fields was back in #22 in this issue and only related to the comment subject.
I don't think it makes sense to have a non-required comment body field. As of now, both the subject and comment body are optional by default (and there is no UI to alter this yet), so you can post plenty of comments containing nothing.
Comment #181
sunIs it that simple?
Comment #182
yched CreditAttribution: yched commentedshould be that simple.
Comment #183
effulgentsia CreditAttribution: effulgentsia commentedI love seeing all the work that's making all this better!
Just cross-linking here to another issue for removing the temporary hack that's causing comment body to bypass normal theme('field') rendering: #652246: Optimize theme('field') and use it for comment body. If you're interested in weighing in on the benefit vs. performance cost, please comment on that issue.
Comment #184
effulgentsia CreditAttribution: effulgentsia commentedOops. sorry about the priority and status changes. Not intentional.
Comment #185
webchickCommitted #181 to HEAD.
Comment #187
BenK CreditAttribution: BenK commentedKeeping track of this