Now we have fieldable comments, it makes sense to have comment body and subject as fields. Follow-up from #504666: Make comments fieldable.

CommentFileSizeAuthor
#181 drupal.comment-body-required.181.patch662 bytessun
#172 comment_update_quickfix.patch977 bytesyched
#167 comment-body_fields-538164-163.patch29.4 KBscor
#164 comment-body_fields-538164-163.patch29.4 KBscor
#163 comment-body_fields-538164-162.patch29.43 KBscor
#158 comment-body_fields-538164-158.patch32.14 KBeffulgentsia
#157 comment-body_fields-538164-157.patch32.14 KBeffulgentsia
#156 comment-body_fields-538164-156.patch30.84 KBscor
#147 comment-body_fields-538164-147.patch32.61 KBeffulgentsia
#146 comment-body_fields-538164-146.patch29.71 KBscor
#145 comment-body_fields-538164-145.patch28.75 KBscor
#143 comment-body_fields-538164-143.patch28.89 KBscor
#140 comment-body_fields-538164-140.patch29.13 KBscor
#135 benchmark-538164-135.txt2.64 KBscor
#134 ab-comment-body-field-538164.txt2.72 KBeffulgentsia
#133 comment-body_fields-538164-133.patch30.82 KBeffulgentsia
#130 comment-body_fields-538164-130.patch31.07 KBeffulgentsia
#123 benchmark-538164-123.txt2.71 KBscor
#119 comment-body_fields-538164-119.patch28.48 KBscor
#116 comment-body_fields-538164-116.patch27.92 KBscor
#115 comment-body_fields-538164-115.patch28.03 KBscor
#110 comment-body_fields-538164-110.patch28.21 KBscor
#106 comment-body_fields-538164-106.patch28.3 KBscor
#104 comment-body_fields-538164-104.patch27.56 KBscor
#103 comment-body_fields-538164-103.patch27.98 KBscor
#101 comment-body_fields-538164-101.patch28.14 KBscor
#99 comment-body_fields-538164-99.patch29.64 KBlinclark
#97 comment-body_fields-538164-97.patch33.09 KBlinclark
#88 comment-body_fields-538164-88.patch30.54 KBscor
#86 comment-body_fields-538164-86.patch29.03 KBscor
#84 comment-body_fields-538164-84.patch28.49 KBscor
#81 comment-body_fields-538164-81.patch29.71 KBlinclark
#78 comment-body_fields-538164-78.patch30.86 KBlinclark
#77 comment-body_fields-538164-77.patch3.86 KBlinclark
#74 comment-body_fields-538164-74.patch29.14 KBscor
#71 comment-body_fields-538164-70.patch27.72 KBlinclark
#69 comment-body_fields-538164-69.patch25.19 KBlinclark
#68 comment-body_fields-538164-68.patch22.31 KBlinclark
#67 comment-body_fields-538164-67.patch22.08 KBlinclark
#66 comment-body_fields-538164-66.patch22 KBlinclark
#65 comment-body_fields-538164-65.patch21.78 KBlinclark
#61 comment_fields_11.patch19.43 KBscor
#52 comment_fields_10.patch41.63 KBscor
#47 comment_fields.patch42.74 KBcatch
#45 comment.patch42.68 KBcatch
#43 comment_70.patch40.64 KByched
#42 comment_69.patch40.3 KByched
#41 comment.patch40.9 KBcatch
#40 comment.patch40.9 KBcatch
#38 comment.patch41 KBcatch
#33 comment_fields.patch39.78 KBcatch
#31 comment_fields.patch40.37 KBcatch
#31 nolabels.png16.22 KBcatch
#30 comment_fields.patch39.55 KBcatch
#28 comment_fields.patch39.55 KBcatch
#20 comment_fields.patch39.15 KBcatch
#17 comment_fields.patch38.73 KBcatch
#16 comment_fields.patch37.13 KBcatch
#7 comment_fields.patch37.21 KBcatch
#5 comment_block.png11.35 KBcatch
#4 comment_fields.patch34.59 KBcatch
#3 comment_fields.patch19.12 KBcatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Issue tags: +Fields in Core

Adding tag.

catch’s picture

Bump. 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.

catch’s picture

Title: Comment body as field » Comment body and subject as fields
Status: Active » Needs review
FileSize
19.12 KB

Here'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.

catch’s picture

FileSize
34.59 KB

All 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.

catch’s picture

FileSize
11.35 KB

Since 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
37.21 KB

Down 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..

Status: Needs review » Needs work

The last submitted patch failed testing.

yched’s picture

Crazy 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.

+  // @TODO do we need to check if any non-comment bundle has these fields
+  // assigned before deleting? If so, what happens then?
+
+  // Delete comment_body field.
+  field_delete_field('comment_body');

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.

catch’s picture

The 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.

yched’s picture

re 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.

yched’s picture

moshe weitzman’s picture

The 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.

yched’s picture

"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 ?

catch’s picture

Hmm. 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.

catch’s picture

Status: Needs work » Needs review
FileSize
37.13 KB

All 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().

catch’s picture

FileSize
38.73 KB

Upgrade 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

Concurrency Level:      1
Time taken for tests:   235.325 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      131357000 bytes
HTML transferred:       130892000 bytes
Requests per second:    4.25 [#/sec] (mean)
Time per request:       235.325 [ms] (mean)
Time per request:       235.325 [ms] (mean, across all concurrent requests)
Transfer rate:          545.11 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   220  235  11.7    232     302
Waiting:      212  226  11.4    223     292
Total:        220  235  11.7    232     302

Patch:
Executed 51 queries in 35.04 milliseconds. Page execution time was 297.99 ms.

Document Path:          /node/1
Document Length:        156276 bytes

Concurrency Level:      1
Time taken for tests:   278.143 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      156741000 bytes
HTML transferred:       156276000 bytes
Requests per second:    3.60 [#/sec] (mean)
Time per request:       278.143 [ms] (mean)
Time per request:       278.143 [ms] (mean, across all concurrent requests)
Transfer rate:          550.32 [Kbytes/sec] received

30% less time in the database. 15% slower overall.

yched’s picture

Probably that same old 'field rendering is costly' issue ?

catch’s picture

Very likely, yes.

catch’s picture

FileSize
39.15 KB

Tidied up some code comments and small change to comment.tpl.php to put the permalink somewhere more sensible.

mlncn’s picture

Hate 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.

-    'subject' => array('data' => t('Subject'), 'field' => 'subject'),
-    t('Are you sure you want to delete the comment %title?', array('%title' => $comment->subject)),
+    t('Are you sure you want to delete the comment by %name', array('%name' => $comment->name)),

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

catch’s picture

It'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.

pwolanin’s picture

I'm a little behind still on grasping field and entity APi stuff, but support getting conversions in if possible [i.e. subsbcribe]

Anonymous’s picture

Catch:

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.

catch’s picture

Hmm, 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.

pwolanin’s picture

Status: Needs review » Needs work

Reviewed 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

@@ -2059,8 +2029,8 @@ function comment_form_submit($form, &$fo
     $redirect = array('comment/' . $comment->cid, array(), 'comment-' . $comment->cid);
   }
   else {
-    watchdog('content', 'Comment: unauthorized comment submitted or comment submitted to a closed post %subject.', array('%subject' => $comment->subject), WATCHDOG_WARNING);
-    drupal_set_message(t('Comment: unauthorized comment submitted or comment submitted to a closed post %subject.', array('%subject' => $comment->subject)), 'error');
+    watchdog('content', 'Comment: unauthorized comment submitted or comment submitted to a closed post %title.', array('%title' => $node->title), WATCHDOG_WARNING);
+    drupal_set_message(t('Comment: unauthorized comment submitted or comment submitted to a closed post %title.', array('%title' => $comment->title)), 'error');

minor typo leading to a bug here "comment->$comment->cid":

@@ -2085,7 +2055,7 @@ function template_preprocess_comment(&$v
   $variables['new']       = !empty($comment->new) ? t('new') : '';
   $variables['picture']   = theme_get_setting('toggle_comment_user_picture') ? theme('user_picture', $comment) : '';
   $variables['signature'] = $comment->signature;
-  $variables['title']     = l($comment->subject, 'comment/' . $comment->cid, array('fragment' => "comment-$comment->cid"));
+  $variables['permalink'] = l('#', 'comment/' . $comment->cid, array('fragment' => "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?

Subject: 
hahaha
Body: 

some text here

If I try to run the update code with no nodes having comments there is an SQL error:

Message: PDOException: SQLSTATE[42000]: Syntax error or access violation: 1064 You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'AS etid, 0 AS deleted, 'zxx' AS language, 0 AS delta
FROM
comment c
INNER JOIN ' at line 1: INSERT INTO {field_data_comment_body_2} (entity_id, bundle, comment_body_value, comment_body_format, etid, deleted, language, delta) SELECT c.cid AS entity_id, n.type AS bundle, c.comment AS comment_body_value, c.format AS comment_body_format, AS etid, 0 AS deleted, 'zxx' AS language, 0 AS delta
FROM
{comment} c
INNER JOIN {node} n ON c.nid = n.nid; Array
(
)
in comment_update_7007() (line 250 of modules/comment/comment.install).

The error is from an empty IN(), so maybe it needs to be checked.

catch’s picture

UI 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.

catch’s picture

Status: Needs work » Needs review
FileSize
39.55 KB

Untested 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
39.55 KB

Fix stupid syntax error.

catch’s picture

FileSize
16.22 KB
40.37 KB

Now hiding subject and body labels by default.
nolabels.png

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
39.78 KB

Sorry, minus debug this time.

SeanBannister’s picture

sub

pwolanin’s picture

Lots 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

yched’s picture

update 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.

catch’s picture

- 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.

catch’s picture

FileSize
41 KB

Converted 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.

moshe weitzman’s picture

FYI, 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.

catch’s picture

FileSize
40.9 KB

Fixed 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.

catch’s picture

FileSize
40.9 KB

And yet another typo.

yched’s picture

FileSize
40.3 KB

Just removed the hardcoded 'zxx' strings in favor of FIELD_LANGUAGE_NONE in tests.

yched’s picture

FileSize
40.64 KB

- 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
42.68 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Status: Needs work » Needs review
FileSize
42.74 KB

Straight re-roll of yched's patch from #43 again to see where we are with test failures.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Version: 7.x-dev » 8.x-dev

Moving to D8.

mlncn’s picture

Version: 8.x-dev » 7.x-dev

RDF wants this, marking as not quite dead yet.

scor’s picture

Issue tags: -Fields in Core

rdf.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.

scor’s picture

Issue tags: +Fields in Core
FileSize
41.63 KB

d.o returned an error. trying again with the patch.

alippai’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

webchick’s picture

I 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?

catch’s picture

I 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.

mlncn’s picture

Issue tags: +Exception code freeze

Speaking 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).

scor’s picture

Title: Comment body and subject as fields » Comment body as fields

At 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

<div property="content:encoded" class="content">
  <p>content of the comment body goes here...</p>
  <p>and we can have as many tags as we want here</p>
  <p>the body does not have a parent tag of its own which wraps the whole body content. what a pity, if only this was a field.</p>
  <div class="field field-name-firstname field-type-text field-label-above clearfix">
    <div class="field-label">Field label:&nbsp;</div>
      <div class="field-items">
        <div class="field-item even">the content of the field goes here</div>
      </div>
    </div>
  </div>
</div>

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.

webchick’s picture

I 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.

yched’s picture

Title: Comment body as fields » Comment body as field

fix title ;-)

scor’s picture

Status: Needs work » Needs review
Issue tags: +RDF
FileSize
19.43 KB

Patch 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.

Status: Needs review » Needs work

The last submitted patch failed testing.

effulgentsia’s picture

subscribing

Anonymous’s picture

I 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.

Anonymous’s picture

Added to the last patch:

  1. In comment_admin_overview (comment.admin.inc), a join with the comment body table. Otherwise, attempts to access $comment->comment were throwing exceptions.
  2. In the test, used 'comment_body[zxx][0][value]' for the input field name instead of 'comment' because that seems to be the default way that the field attach api handles input field names. See comment 64 above.
  3. Commented out the line at 1444 in comment.module that must have been added in one of the patches. It was causing 2 errors. Will have to find who added it and whether it is still necessary in the 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.

Anonymous’s picture

Removed 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.

Anonymous’s picture

Corrected 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.

Anonymous’s picture

Corrected 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.

Anonymous’s picture

Corrected 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.

WorldFallz’s picture

Status: Needs work » Needs review

and the bot says....

Anonymous’s picture

Variables corrected in trigger and tracker tests as per comment 64.

Rerolled to work with latest version of head.

Status: Needs review » Needs work

The last submitted patch failed testing.

catch’s picture

Can'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().

scor’s picture

Fixed 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).

WorldFallz’s picture

Status: Needs work » Needs review

kicking the bot again...

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

Thanks for pointing out the error, catch.

I have used comment_load_multiple.

EDIT: I need to reroll this real quick

Anonymous’s picture

Ok, this time for real.

The edit from the last patch is at line 82 of comment.admin.inc.

NaheemSays’s picture

Status: Needs work » Needs review

I assume this needs testing?

Status: Needs review » Needs work

The last submitted patch failed testing.

Anonymous’s picture

Since 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.

WorldFallz’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch failed testing.

scor’s picture

Status: Needs work » Needs review
FileSize
28.49 KB

comment_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).

Status: Needs review » Needs work

The last submitted patch failed testing.

scor’s picture

Status: Needs work » Needs review
FileSize
29.03 KB

fixes the search ranking test and removes some trailing spaces.

Status: Needs review » Needs work

The last submitted patch failed testing.

scor’s picture

Status: Needs work » Needs review
FileSize
30.54 KB

fixes the remaining failing tests in field_ui.test and forum.test.

scor’s picture

+++ modules/comment/comment.module
@@ -1911,8 +1908,7 @@ function comment_preview($comment) {
+    $comment->format = $comment->comment_body['zxx'][0]['format'];

ignore 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.

scor’s picture

shame 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.

catch’s picture

As 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:

Concurrency Level:      1
Time taken for tests:   428.009 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      140984000 bytes
HTML transferred:       140458000 bytes
Requests per second:    2.34 [#/sec] (mean)
Time per request:       428.009 [ms] (mean)
Time per request:       428.009 [ms] (mean, across all concurrent requests)
Transfer rate:          321.68 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   394  428  26.9    422     613
Waiting:      384  417  26.1    411     603
Total:        394  428  26.9    422     613

Patch:

Document Path:          /node/2
Document Length:        150997 bytes

Concurrency Level:      1
Time taken for tests:   476.468 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      151523000 bytes
HTML transferred:       150997000 bytes
Requests per second:    2.10 [#/sec] (mean)
Time per request:       476.468 [ms] (mean)
Time per request:       476.468 [ms] (mean, across all concurrent requests)
Transfer rate:          310.56 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.0      0       0
Processing:   450  476  21.5    473     792
Waiting:      439  464  20.9    461     780
Total:        450  476  21.5    473     792

catch’s picture

Cross 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.

scor’s picture

I 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:

Concurrency Level:      1
Time taken for tests:   362.018 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      152513000 bytes
HTML transferred:       152000000 bytes
Requests per second:    2.76 [#/sec] (mean)
Time per request:       362.018 [ms] (mean)
Time per request:       362.018 [ms] (mean, across all concurrent requests)
Transfer rate:          411.41 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.1      0       2
Processing:   342  362  18.3    356     505
Waiting:      328  347  17.1    342     486
Total:        342  362  18.3    356     505

HEAD with patch:

Concurrency Level:      1
Time taken for tests:   375.987 seconds
Complete requests:      1000
Failed requests:        0
Write errors:           0
Total transferred:      162763000 bytes
HTML transferred:       162250000 bytes
Requests per second:    2.66 [#/sec] (mean)
Time per request:       375.987 [ms] (mean)
Time per request:       375.987 [ms] (mean, across all concurrent requests)
Transfer rate:          422.75 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:        0    0   0.2      0       5
Processing:   360  376   9.3    375     483
Waiting:      346  361   8.5    359     431
Total:        360  376   9.3    375     483

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.

yched’s picture

It 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 ?

yched’s picture

Status: Needs review » Needs work

Other remarks:

+++ modules/comment/comment.install
@@ -207,6 +223,92 @@ function comment_update_7010() {
+    $body_instance['bundle'] = 'comment_node_' . $info->type;

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.

+++ modules/comment/comment.admin.inc
@@ -72,26 +72,37 @@ function comment_admin_overview($form, &$form_state, $arg) {
+  // We need to load the comment object to get the comment_body field which is
+  // stored in its own table. To do this, get an array of cids.
+  foreach ($result as $comment) {
+    $comments[] = $comment;
+    $cid[] = $comment->cid;
+  }
+  $comment_objects = comment_load_multiple($cid);

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() ?

+++ modules/comment/comment.install
@@ -207,6 +223,92 @@ function comment_update_7010() {
+function comment_update_7013(&$context) {
+  $ret = array();

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'].

+++ modules/comment/comment.pages.inc
@@ -108,7 +108,7 @@ function comment_reply($node, $pid = NULL) {
-  $comment->comment_format = $comment->format;
+//  $comment->comment_format = $comment->format;

Should I stay or should I go ? ;-)

I'm on crack. Are you, too?

Anonymous’s picture

I don't get why we bother with both $comments (fetched from a direct query) and $comment_objects (loaded by comment_load_multiple()).

I 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 are using the node title in the display, which I don't think is returned as part of the comment object (correct me if I'm wrong).
  • We need the status field so that we only return comments that have not been approved.
  • We need subject, name, node_title, and changed for the TableSort.

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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
33.09 KB

In this patch:

  • Changed the hardcoded 'zxx' to the constant FIELD_LANGUAGE_NONE.
  • Removed the join with the users table in comment.admin.inc

Still needs to be done:

  • Change in permissions in #90
  • Parameter in hook_update in #95
  • No return for update function in #95

Going to kick the bot to make sure this passes as expected.

yched’s picture

Status: Needs review » Needs work

re #96:

* We are using the node title in the display, which I don't think is returned as part of the comment object (correct me if I'm wrong).
* We need the status field so that we only return comments that have not been approved.
* We need subject, name, node_title, and changed for the TableSort.

Right, I forgot about the tablesort.
Then the query should only include the columns needed for the tablesort.

->fields('c', array('subject', 'nid', 'cid', 'changed', 'status', 'name', 'homepage'))

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.

Anonymous’s picture

Status: Needs work » Needs review
FileSize
29.64 KB

Patch includes:

  • From #94—Removal of changes from comment->subject to comment->name/comment->cid. I think I caught all of them
  • From #95—Change in the hook_update parameter
  • From #95—Updated instances of $ret in hook_update_N

Still needs to be done:

  • From #90—Not sure which test needs the permissions change, will ask scor
scor’s picture

@linclark: you can ignore this permission glitch, it was just necessary to run the benchmarks.

+++ modules/comment/comment.module	22 Nov 2009 11:49:26 -0000
@@ -1353,8 +1360,6 @@ function comment_save($comment) {
@@ -1425,6 +1430,7 @@ function comment_delete_multiple($cids) 
@@ -1425,6 +1430,7 @@ function comment_delete_multiple($cids) 
       comment_delete_multiple($child_cids);
       _comment_update_node_statistics($comment->nid);
     }
+    //parent::attachLoad($comments);
   }
 } 

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).

+++ modules/tracker/tracker.test	22 Nov 2009 11:49:28 -0000
@@ -74,7 +74,7 @@ class TrackerTest extends DrupalWebTestC
-      'comment' => $this->randomName(20),
+      'comment_body[zxx][0][value]' => $this->randomName(20),

zxx needs to be FIELD_LANGUAGE_NONE

This review is powered by Dreditor.

scor’s picture

fix the remaining bits of [zxx].

yched’s picture

Status: Needs review » Needs work

Scor 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:

+++ modules/comment/comment.install
@@ -10,6 +10,9 @@
+  // Delete comment_body field.
+  field_delete_field('comment_body');
+

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.

+++ modules/comment/comment.install
@@ -43,6 +46,19 @@ function comment_enable() {
+  // Create comment body field.
+  // @todo this should be done in comment_install, but causes exceptions
+  // in simpletest due to dependencies between entity_get_info() and
+  // taxonomy_install().

Could we just give it another shot - things *might* have gotten better meanwhile ?

+++ modules/comment/comment.install
@@ -43,6 +46,19 @@ function comment_enable() {
+  // Create instances on all existing node types.
+  node_types_rebuild();

I'm not sure I get this.

+++ modules/comment/comment.install
@@ -207,6 +223,87 @@ function comment_update_7010() {
+    // Hide field labels by default.

Nitpick: 'label' (singular) might be more accurate ?

+++ modules/comment/comment.install
@@ -207,6 +223,87 @@ function comment_update_7010() {
+  foreach (node_type_get_types() as $info) {
+    $body_instance['bundle'] = 'comment_node_' . $info->type;
+    field_create_instance($body_instance);
+  }

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]

+++ modules/comment/comment.install
@@ -207,6 +223,87 @@ function comment_update_7010() {
+  $sandbox['#finished'] = 0;

not needed

+++ modules/comment/comment.install
@@ -207,6 +223,87 @@ function comment_update_7010() {
+    if (!$comments) {
+      // The update will finish when $sandbox['types'] is an empty array.
+      // Since we have nothing to do if there's no comments on the site, except
+      // drop the columns, set that here for convenience.
+      $sandbox['types'] = array();
+    }
+    else {
+      $sandbox['etid'] = _field_sql_storage_etid('comment');
+      $sandbox['types'] = node_type_get_types();
+    }

could be shortened to

$sandbox['types'] = array();
if ($comments) {
  $sandbox['etid'] = _field_sql_storage_etid('comment');
  $sandbox['types'] = node_type_get_types();
}
+++ modules/comment/comment.module
@@ -1425,6 +1430,7 @@ function comment_delete_multiple($cids) {
+    //parent::attachLoad($comments);

Should I stay or should I go ? ;-)

I'm on crack. Are you, too?

scor’s picture

Status: Needs work » Needs review
FileSize
27.98 KB

rerolled the patch since it didn't apply any longer. kicking the bot to make sure all tests are passing.

from #98

Then the query should only include the columns needed for the tablesort.
->fields('c', array('subject', 'nid', 'cid', 'changed', 'status', 'name', 'homepage'))

should be
->fields('c', array('cid', 'subject', 'name', 'changed'))

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.

scor’s picture

All of yched points have been addressed.

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.

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...

+++ modules/comment/comment.install
@@ -43,6 +46,19 @@ function comment_enable() {
+  // Create comment body field.
+  // @todo this should be done in comment_install, but causes exceptions
+  // in simpletest due to dependencies between entity_get_info() and
+  // taxonomy_install().

Could we just give it another shot - things *might* have gotten better meanwhile ?

no luck. It works during a normal install but not during testing.

FieldException: Attempt to create a field of unknown type <em>text_long</em>. in field_create_field() (line 279 of /Applications/MAMP/htdocs/d7git/modules/field/field.crud.inc).

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.

+++ modules/comment/comment.install
@@ -43,6 +46,19 @@ function comment_enable() {
+  // Create instances on all existing node types.
+  node_types_rebuild();

I'm not sure I get this.

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).

yched’s picture

Status: Needs review » Needs work
+++ modules/comment/comment.install
@@ -43,6 +46,20 @@ function comment_enable() {
+  // Create comment body field.
+  // @todo this should be done in comment_install, but causes exceptions
+  // in simpletest due to dependencies between entity_get_info() and
+  // taxonomy_install().

I'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() ;-)

+++ modules/comment/comment.admin.inc
@@ -67,31 +67,38 @@ function comment_admin_overview($form, &$form_state, $arg) {
+  foreach ($result as $comment) {

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.

+++ modules/comment/comment.install
@@ -43,6 +46,20 @@ function comment_enable() {
+  // Create comment body field instances on all existing node types via
+  // comment_node_type_insert().
+  node_types_rebuild();

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.

scor’s picture

Status: Needs work » Needs review
FileSize
28.3 KB

Thanks yched, we're almost there ;) I've fixed all your comments. There is now a helper function to cover your last concern.

yched’s picture

Status: Needs review » Needs work
+++ modules/comment/comment.module
@@ -239,7 +239,7 @@ function comment_count_unpublished() {
 function comment_node_type_insert($info) {
-  field_attach_create_bundle('comment', 'comment_node_' . $info->type);
+  _comment_body_field_instance_create($info);
 }

Hm, I don't think we want to remove the call to f_a_create_bundle() ?

I'm on crack. Are you, too?

scor’s picture

Status: Needs work » Needs review

@yched, it's not removed, it's moved with the rest in _comment_body_field_instance_create():

+++ modules/comment/comment.module
@@ -271,6 +271,30 @@ function comment_node_type_delete($info) {
+function _comment_body_field_instance_create($info) {
+  field_attach_create_bundle('comment', 'comment_node_' . $info->type);
yched’s picture

Status: Needs review » Needs work

Oops, 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() ;-)

scor’s picture

Status: Needs work » Needs review
FileSize
28.21 KB

oh, right, I've moved it back in comment_node_type_insert().

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks ! RTBC, then :-).

scor’s picture

Issue tags: +beta blocker

bumping 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...)

webchick’s picture

Dries, 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. :)

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs work

I'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:

+++ modules/comment/comment.install
@@ -214,6 +237,80 @@ function comment_update_7011() {
+  // On the last pass of the update, $sandbox['types'] will be empty.
+  if (empty($sandbox['types'])) {
+    db_drop_field('comment', 'subject');
+  }

Is this correct? If so, should there also be a removal of 'subject' from hook_schema()?

This review is powered by Dreditor.

scor’s picture

@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.

scor’s picture

Status: Needs work » Needs review
FileSize
27.92 KB

now with the new LANGUAGE_NONE constant.

scor’s picture

created #652246: Optimize theme('field') and use it for comment body as follow up issue for when this is in.

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

Human 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

scor’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
28.48 KB

comment_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

mlncn’s picture

Status: Needs review » Reviewed & tested by the community

Oh, wonderful! I do feel better now.

effulgentsia’s picture

On 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 :).

chx’s picture

Obviously no. Six or nineteen , too much.

scor’s picture

FileSize
2.71 KB

@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:

for i in {1..100}
do
echo "Round $i"
ab -c1 -n1 http://538164-head.drupalrdf.openspring.net/node/9 >> bench_results_head.txt
ab -c1 -n1 http://538164-patch.drupalrdf.openspring.net/node/9 >> bench_results_patch.txt
done

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 :(

scor’s picture

And 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?

yched’s picture

re #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.

effulgentsia’s picture

catch's results in #91 are closer to 11% than 19%

You'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.

effulgentsia’s picture

I'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.

effulgentsia’s picture

Almost 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 of template_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.

moshe weitzman’s picture

I'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.

effulgentsia’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
31.07 KB

Here's a patch that's identical to #119 except for this extra hunk:

@@ -848,6 +868,19 @@ function comment_build_content($comment,
 
   // Allow modules to make their own additions to the comment.
   module_invoke_all('comment_view', $comment, $build_mode);
+
+  // @todo Comment body used to not be a field, and when it was converted to a
+  //   a field, it resulted in too much rendering overhead (primarily from
+  //   field.tpl.php, but also extra nested drupal_render() calls). We should
+  //   optimize field rendering generically, or at least for comment body
+  //   specifically, but until then, set the render array to the single item in
+  //   order to bypass field rendering overhead. This results in 'comment_body'
+  //   being similar in structure to what it would be if it were not a field.
+  //   Remove this code when a better optimization is in place.
+  //   @see http://drupal.org/node/538164.
+  if (isset($comment->content['comment_body']['items'][0])) {
+    $comment->content['comment_body'] = $comment->content['comment_body']['items'][0];
+  }
 }

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?

effulgentsia’s picture

Alternatively, we could replace the new hunk in #130 with this instead:

@@ function comment_build_content($comment,
 
   // Allow modules to make their own additions to the comment.
   module_invoke_all('comment_view', $comment, $build_mode);

+ // @todo Some appropriate comment.
+ if (isset($comment->content['comment_body']['#theme']) && ($comment->content['comment_body']['#theme'] == 'field')) {
+    unset($comment->content['comment_body']['#theme']);
+  }
 }

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?

catch’s picture

I 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.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
30.82 KB

Ok, this patch has #131 flushed out. Here's the relevant hunk:

@@ -829,15 +854,20 @@ function comment_build_content($comment,
   // Build fields content.
   field_attach_prepare_view('comment', array($comment->cid => $comment), $build_mode);
   $comment->content += field_attach_view('comment', $comment, $build_mode);
 
+  // @todo Having comment body as a field is awesome, but theme('field') is too
+  //   slow, considering there can be 50+ comments on a page accessed by many
+  //   concurrent users. If theme('field') is sufficiently optimized prior to
+  //   Drupal 7 release, then this hack can be removed. Otherwise, a module can
+  //   undo it within a hook_comment_view() or hook_comment_build_alter()
+  //   implementation. @see http://drupal.org/node/538164.
+  if (isset($comment->content['comment_body']['#theme']) && ($comment->content['comment_body']['#theme'] == 'field')) {
+    unset($comment->content['comment_body']['#theme']);
+  }
+
   if (empty($comment->in_preview)) {
     $comment->content['links']['comment'] = array(
       '#theme' => 'links',

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.

effulgentsia’s picture

Benchmarks from my system attached for viewing an article node with 50 comments.

HEAD: 207.7ms
Patch 133: 208.9ms
Impact of patch: 0.5%

scor’s picture

FileSize
2.64 KB

OMG! Thank you effulgentsia! #133 shows a 1% performance improvement compared to HEAD on my machine! :)
HEAD: 309.4 ms
patch: 306.2ms

moshe weitzman’s picture

Very nice work. Patch gets +1 from me.

yched’s picture

Very 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

effulgentsia’s picture

I 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.

scor’s picture

@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.

scor’s picture

rerolled 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 :)

effulgentsia’s picture

Dries’s picture

Status: Reviewed & tested by the community » Needs work

Patch no longer applies -- could we do a reroll please? Thanks! :)

scor’s picture

Status: Needs work » Needs review
FileSize
28.89 KB

here it is!

Dries’s picture

+++ modules/comment/comment.install
@@ -43,6 +46,26 @@ function comment_enable() {
+  // Ensures all node types have an instance of the comment body field. An
+  // instance is created if it does not exist.
+  foreach (node_type_get_types() as $type => $info) {
+    if (!field_info_instance('comment', 'comment_body', 'comment_node_' . $info->type)) {
+      _comment_body_field_instance_create($info);
+    }
+  }

I don't understand this part yet. Why would we attach an instance of comment_body to each node type?

+++ modules/comment/comment.module
@@ -845,15 +870,20 @@ function comment_build_content($comment, $node, $build_mode = 'full') {
+  // @todo Having comment body as a field is awesome, but theme('field') is too
+  //   slow, considering there can be 50+ comments on a page accessed by many
+  //   concurrent users. If theme('field') is sufficiently optimized prior to
+  //   Drupal 7 release, then this hack can be removed. Otherwise, a module can
+  //   undo it within a hook_comment_view() or hook_comment_build_alter()
+  //   implementation. @see http://drupal.org/node/538164.
+  if (isset($comment->content['comment_body']['#theme']) && ($comment->content['comment_body']['#theme'] == 'field')) {
+    unset($comment->content['comment_body']['#theme']);
+  }

Nice to do, but we don't explain what the hack does, or why it makes things faster.

+++ modules/comment/comment.test
@@ -28,8 +28,9 @@ class CommentHelperCase extends DrupalWebTestCase {
+    $edit['comment_body['.$langcode.'][0][value]'] = $comment;

Spacing is incorrect. There are many other instances of this.

This review is powered by Dreditor.

scor’s picture

I don't understand this part yet. Why would we attach an instance of comment_body to each node type?

During 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.

Nice to do, but we don't explain what the hack does, or why it makes things faster.

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?

Spacing is incorrect. There are many other instances of this.

fixed.

EDIT: the right patch is in the next comment #146.

scor’s picture

uploading the right patch this time.

effulgentsia’s picture

I 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.

scor’s picture

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.

The 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:

Prior to Drupal 7, the comment body was a simple text variable, but with Drupal 7, it has been upgraded to a field. However, using theme('field') to render the comment body would result in a noticeable performance degradation, considering there can be 50+ comments on a page accessed by
many concurrent users. By unsetting #theme, we avoid the overhead of theme('field') and instead settle for simply rendering the formatted field value that exists as a child element of the 'comment_body' render array. Modules that require the comment body to be rendered as a full field can
restore #theme to 'field' within a hook_comment_view() or hook_comment_build_alter() implementation.
@todo Bypassing theme('field') is not ideal, because:
  - It results in the field label not being displayed even if the setting is for it to be displayed.
  - It results in hook_preprocess_field() functions not running, and attributes added in those functions (for example, for RDF) not being
    output.
  - It results in the HTML markup that's within field.tpl.php to not be output, requiring theme developers to use different CSS rules for the
    comment body than for all other fields.
The goal is for theme('field') to be sufficiently optimized prior to Drupal 7 release, that this code can be removed, so that the comment body is rendered just like all other fields. Otherwise, something else will be needed to address the above problems. @see http://drupal.org/node/659788.
Dries’s picture

It seems like the name comment_body is inconsistent with other field names in comment, but also inconsistent with the node body name.

effulgentsia’s picture

We need testbot back before this patch can land, but we can keep discussing outstanding issues, to help this move quickly once it is back.

  • @Dries: What name do you suggest instead of 'comment_body'? It can't be 'body', because it uses a different field type (text_long) than the 'body' field already in use for nodes (text_with_summary). I suppose it could be 'comment', since that's the name of the database column that it's replacing, but since comments are fieldable, other fields can be added to comments, so does it make sense to privilege this one with the shorter name?
  • I'm not fully convinced by scor's answer in #145 as to why 'comment_body' needs to be attached to the node type. Seems like there ought to be a better way to deal with old comments for node types with comments currently disabled. But I haven't kept up with this issue's history or delved deep enough into the patch to suggest an alternative. Can someone else confirm whether this is ok, or whether it needs more work?
  • Is there anything else of concern?
scor’s picture

Why would we attach an instance of comment_body to each node type?

This 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.

Status: Needs review » Needs work
Issue tags: -RDF, -Fields in Core, -Exception code freeze, -beta blocker

The last submitted patch, , failed testing.

Status: Needs work » Needs review
Issue tags: +RDF, +Fields in Core, +Exception code freeze, +beta blocker

Re-test of from comment #2396766 was requested by @user.

catch’s picture

I 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.

Status: Needs review » Needs work

The last submitted patch, , failed testing.

scor’s picture

Status: Needs work » Needs review
FileSize
30.84 KB

Patch updated according to the changes #658364: Does build/view/formatter terminology make sense? introduced.

effulgentsia’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
32.14 KB
+++ modules/comment/comment.install
@@ -43,6 +46,28 @@ function comment_enable() {
+  // Ensures all node types have an instance of the comment body field
+  // regardless of whether they have comments enabled or not. Node types with
+  // comments disabled might have comments nonetheless and we need this field
+  // to store these comments. An instance is created if it does not exist.
+  foreach (node_type_get_types() as $type => $info) {
+    if (!field_info_instance('comment', 'comment_body', 'comment_node_' . $info->type)) {
+      _comment_body_field_instance_create($info);
+    }
+  }

#157 changes the code comment here and adds an @todo.

+++ modules/comment/comment.module
@@ -240,6 +240,8 @@ function comment_count_unpublished() {
 function comment_node_type_insert($info) {
   field_attach_create_bundle('comment', 'comment_node_' . $info->type);
+
+  _comment_body_field_instance_create($info);
 }

#157 also adds an @todo here to match the @todo added to comment_enable() above.

+++ modules/comment/comment.module
@@ -844,15 +869,37 @@ function comment_build_content($comment, $node, $build_mode = 'full') {
+  // Prior to Drupal 7, the comment body was a simple text variable, but with
+  // Drupal 7, it has been upgraded to a field. However, using theme('field') to
+  // render the comment body would result in a noticeable performance
+  // degradation, considering there can be 50+ comments on a page accessed by
+  // many concurrent users. By unsetting #theme, we avoid the overhead of
+  // theme('field') and instead settle for simply rendering the formatted field
+  // value that exists as a child element of the 'comment_body' render array.
+  // Modules that require the comment body to be rendered as a full field can
+  // restore #theme to 'field' within a hook_comment_view() or
+  // hook_comment_view_alter() implementation.
+  // @todo Bypassing theme('field') is not ideal, because:
+  //   - It results in the field label not being displayed even if the setting
+  //     is for it to be displayed.
+  //   - It results in hook_preprocess_field() functions not running, and
+  //     attributes added in those functions (for example, for RDF) not being
+  //     output.
+  //   - It results in the HTML markup that's within field.tpl.php to not be
+  //     output, requiring theme developers to use different CSS rules for the
+  //     comment body than for all other fields.
+  //   The goal is for theme('field') to be sufficiently optimized prior to
+  //   Drupal 7 release, that this code can be removed, so that the comment body
+  //   is rendered just like all other fields. Otherwise, something else will be
+  //   needed to address the above problems. @see http://drupal.org/node/659788.
+  if (isset($comment->content['comment_body']['#theme']) && ($comment->content['comment_body']['#theme'] === 'field')) {
+    unset($comment->content['comment_body']['#theme']);
+  }
+

#157 cleans up this code comment a little.

+++ modules/forum/forum.test
@@ -78,7 +78,8 @@ class ForumTestCase extends DrupalWebTestCase {
-    $this->drupalPost("node/$node->nid", array('comment' => $this->randomName()), t('Save'));
+    $edit_comment['comment_body[' . LANGUAGE_NONE . '][0][value]'] = $this->randomName();
+    $this->drupalPost("node/$node->nid", $edit_comment, t('Save'));

#157 changes $edit_comment to $edit and initializes it before adding to it.

+++ modules/field_ui/field_ui.test
@@ -177,7 +177,9 @@ class FieldUITestCase extends DrupalWebTestCase {
-    $this->assertNoText(t('Body'), t('Body field was deleted.'));
+    // Since the comment still has a body field, we need to target the Body
+    // label in the field list.
+    $this->assertNoRaw('<span class="label-field">' . t('Body') . '</span>', t('Body field was deleted.'));

#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)?

effulgentsia’s picture

This 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.

effulgentsia’s picture

Issue tags: -beta blocker +alpha blocker

From webchick's D7 alpha1 announcement:

Once the alpha is released, we will start to treat Drupal 7 as an actual stable release, which means nothing but non-API-breaking bug fixes.

It would be such a shame to not see this make it for D7. What's needed for this to land before 1/15?

axyjo’s picture

+++ modules/comment/comment.test	27 Dec 2009 19:58:46 -0000
@@ -72,6 +74,7 @@ class CommentHelperCase extends DrupalWe
     preg_match('/#comment-([0-9]+)/', $this->getURL(), $match);
 
+
     // Get comment.

Minor, 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?

scor’s picture

rerolled the patch with axyjo's comment (removed a couple of other newline hunks too).

scor’s picture

rerolling 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?

effulgentsia’s picture

I 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.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, comment-body_fields-538164-163.patch, failed testing.

scor’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
29.4 KB

cannot 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs documentation

Margh! :( 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.

Anonymous’s picture

When 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).

scor’s picture

Thank 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

  $sandbox['#finished'] = 1 - count($sandbox['types']) / $sandbox['total'];

Could you create an issue on this and give instructions to reproduce?

Anonymous’s picture

I 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.

yched’s picture

Status: Needs work » Needs review
FileSize
977 bytes

Fix for the (harmless) "Warning: Division by zero in comment_update_7013()" error reported in #169. Happened when there was no comments to process.

webchick’s picture

Status: Needs review » Needs work

On that note, when you go to admin/content/comment without any comments, you also get the following notice:

Notice: Undefined variable: cids in comment_admin_overview() (line 85 of /Users/webchick/Sites/core/modules/comment/comment.admin.inc).

Could we fix both of 'em up here?

aspilicious’s picture

yched’s picture

Status: Needs work » Needs review

If the other bug has it's own thread, followup fix for the update in #172 is back to CNR.

fgm’s picture

yched’s picture

webchick’s picture

Status: Needs review » Needs work

Ok, cool. I committed the fix in #173, since the rest are documented elsewhere.

Moving back to "needs work" for documentation.

yched’s picture

FYI - patch to enable a UI for fields on comments: #537750: Field UI for comments

sun’s picture

Priority: Normal » Critical

Problem: 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.

sun’s picture

Status: Needs work » Needs review
FileSize
662 bytes

Is it that simple?

yched’s picture

Status: Needs review » Reviewed & tested by the community

should be that simple.

effulgentsia’s picture

Priority: Critical » Normal
Status: Reviewed & tested by the community » Needs work

I 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.

effulgentsia’s picture

Priority: Normal » Critical
Status: Needs work » Reviewed & tested by the community

Oops. sorry about the priority and status changes. Not intentional.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed #181 to HEAD.

Status: Fixed » Closed (fixed)

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

BenK’s picture

Keeping track of this