Make comments fieldable

This will need #470242: Namespacing for bundle names too, because bundles for 'fieldable comments' will probably be node types too.

Sure. We'll need a better fix at some point, but this would allow the work to begin.

Tagging "Fields in Core"

Yay, catch !

A couple remarks:
- missing comment_fieldable_info() ? (could possibly explain some of the exceptions)
- I'm not sure this will work:

+  field_attach_submit('comment', (object) $form_state['values'], $form, $form_state);

because field_attach_submit() alters its second argument ($object) by adding the field values extracted from $form_state. If this $object is a one-off object cast from $form_state['values'], I don't think the field additions will persist.
Having some code to build a real 'stub' comment $object from form values, then passing it to field_attach_submit() seems like a cleaner approach.

Just like we did for 'taxo term' edit forms, we'll have to make sure the 'submit comment' forms handle multistep (for the 'Add more values' button on multi-values fields).

Also : until #502538: Multiple load comments gets in, field_attach_load() could be used with a single-object array. Obviously not what we want in the end, but it would let this patch be actually tested.

Typo : foreach ($node_type_get_names() :-p

Hm. The tricky question behind this, of course, is : UI-wise, where would we put the 'manage fields' screen for comments on node type 'article' ?

First naïve idea would be to move the 'Comment settings' fieldgroup at the bottom of the node type edit form into it's own primary tab.
admin/build/node-type/article would then have primary tabs : Edit | Comments | Manage Fields | Display Fields
And 'Comments' would have three secondary tabs : Settings | Manage Fields | Display Fields

The current Field UI pages kind of assume they live as primary tabs and thus have 'secondary tab' level available for direct access to field configure pages, but I'm not sure we'll want to keep those anyway.
This would mean paths like admin/build/node-type/page/comments/fields/body/remove (8 parts)

Well, I'm not sure this needs to be solved here. The patch could leave hook_fieldable_info()'s $return['comment']['bundles'] with no 'admin' section (CCK HEAD and the field UI patch shouldn't choke on this), and figure out a home for the UI in a followup. Meanwhile, fieldable comments would have no attachable UI, which works as a temporary step (fieldable terms started out this way too).

Attached patch fixes a couple details:

- With the current bundle names, $comment->node_type = $node->type; needs to be $comment->node_type = 'comment_node_' . $node->type;

- Added missing field_attach_load() call in the comment_edit() page callback.

- Fixes some test exceptions: $info->new_type should be $info->type in comment_node_type()

Quickly tested using an API-added field on article comments (run the following code in a devel "execute PHP" block)

$field = array(
  'field_name' => 'field_multiple',
  'cardinality' => FIELD_CARDINALITY_UNLIMITED,
  'type' => 'text',
);
field_create_field($field);
$instance = array(
  'field_name' => 'field_multiple',
  'bundle' => 'comment_node_article',
  'label' => 'Multiple',
);
field_create_instance($instance);

Fields can be added to comment objects, field values can be entered and edited back.
Field values are not displayed, though, because the patch doesn't make any field_attach_view() call yet - I guess this will require some refactoring of comment display into render arrays :-(

Some additional refactoring on the 'form' part will be needed to make the 'add more' button work in non-JS mode.

Added the form refactoring for non-JS 'add more':
- added function comment_form_submit_build_comment() : Build a comment by processing form values and prepare for a form rebuild
- made the comment form multistep
- added $form['#builder_function'] = 'comment_form_submit_build_comment'; property needed by field_add_more_submit()

I did not check the 'Preview' part.

Actually, it seems my changes for multistep comment form broke comment preview (which I guess is not tested). Didn't really investigated yet, but comment preview are handled very differently than node previews, might be good to unify the two a little.

I'm not where I can re-test right now, but it's possible that it was the "press 'add new value' button, then preview" sequence that was broken - needs to be tested with JS on and JS off.

As suggested in #21, this was because comments previews were still D5 style: added on #after_build when $_POST['op'] == 'Preview'.
Yuck + incompatible with #ahah, which sets $form['#cache'] = TRUE, which then skips form building and fetches from the cache...

Updated patch moves comment preview to use the same workflow than node preview:
- $form['preview'] = array('#type' => 'submit', '#submit' => array('comment_form_build_preview'));
- comment_form_build_preview() computes a preview and puts it in $form_state['comment_preview']
- comment_form() outputs $form_state['comment_preview'] if it finds any.

Previewing seems to work fine now.

Left as a TODO: I'm not sure whether we want a $comment->in_preview property, like nodes have.

Also fixed a bug in comment_load() : always returned FALSE - I don't really understand why, but current() doesn't seem to apply here. Updated to use the same code than node_load().
(technically, this would be more a followup to #502538: Multiple load comments)

CNR for the bot, but this still needs work for the refactoring of comment display into render arrays. I'm happy if someone else takes care of that ;-)

subscribing, interested to build on this wrt to RDF support in core.

Where will you add fields to comments?

@Bohjan: that's a challenge indeed - see #15. I don't think we need to solve this in this patch though.

lol, the practice of lets do the code first and UI second in all this fields stuff is getting funky.

Inspired by the progress here, I put on my hazmat suit and hacked on comment rendering until it complied. See #523950: Update comment rendering. My patch is pretty big. Better to keep it separate.

Yay, thanks moshe. Do you guys think the 'refactor comment preview' part (included in the patch already) should be moved out to a separate patch too ?

I moved the 'form and preview' rework over to #524652: Overhaul comment form and preview

New patch without those parts (should be fairly similar to #16)

#524652: Overhaul comment form and preview was commited, so set this back to active

Well, yay for the quick commit of the 'form + preview' patch, but we still need #523950: Update comment rendering :-)

#523950: Update comment rendering is in, rock on !
Will try to reroll shortly if no-one beats me to it.

Rerolled. Seems to work pretty well. As we saw with taxo terms, 'make X fieldable' is a ~10K patch - that is, once X is up to current drupal patterns for load, form and view :-/.

Not really in the scope of this patch, but anyone knows why comment_reply() does // Load the comment whose cid = $pid using a direct query instead of using comment_load() ? Looks pretty ugly, and this comment escapes hook_comment_load(). Well, current patch adds a field_attach_load() in there for now.

Also, as noted in #15, this patch doesn't expose the informations to attach Field UI pages (be it CCK HEAD or the the field_ui patch in #516138: Move CCK HEAD in core).
This is better debated in a followup task.

To test 'fieldable comments' for now, run the following code in a devel "execute PHP" block:

$field = array(
  'field_name' => 'field_multiple',
  'cardinality' => FIELD_CARDINALITY_UNLIMITED,
  'type' => 'text',
);
field_create_field($field);
$instance = array(
  'field_name' => 'field_multiple',
  'bundle' => 'comment_node_article',
  'label' => 'Multiple',
);
field_create_instance($instance);

And play with comments on an article node.

Of course, a cool followup task would be 'comment body and subject as Fields' :-p. I probably won't tackle it myself, though.

So, comment_node_delete() currently does 2 quick queries to delete all affiliated comments. This patch does a delete per comment plus a watchdog(). Thats a pretty hefty change. Would it be too much to introduce comment_delete_multiple() with this patch? We would model it on node_delete_multiple(). This could be called from comment_node_delete() and from _comment_delete_thread().

The casting to array and from array is a bit unfortunate. Perhaps not fixable here.

Otherwise, code looks pretty good to me.

cool.

watchdog('content', 'Comment: deleted %subject.', array('%subject' => $comment->subject));
+      field_attach_delete('comment', $comment);

could you move the watchdog() outside of the foreach so it isn;'t called for each comment? Thats what we did for nodes IIRC. No field_attach_delete_multiple()? Oh well.

The exceptions were caused by that silly direct query in comment_preview(), missing 'node_type' (bundle name for comments).
Easily fixed, but here's a patch that kicks that custom query out and relies on regular comment load.

About field_attach_delete_multiple() : See #467926: make field_attach_delete() multiple.
But it might also be affected by the outcome of #367753: Bulk deletion, because we might want to move individual object deletion to 'bulk delete' too. Fieldable comments, notably, introduces cases where deletion of a single node or comment cascades down to arbitrary large # of comments being deleted.

Agreed. RTBC.

Before I commit this, I'd like to see us do some benchmarks on nodes that have quite a few comments. Maybe one test where the comments have no additional fields (i.e. old behavior so we can see if there is a regression) and one test with comments that have one or two extra fields.

Rerolled (fuzz) and updated CHANGELOG

Crossposted catch's data over to #438570: Field API conversion leads to lots of expensive function calls - and commented over there.

Doh, there was a nasty bug in comment_load_multiple(): the node type was cast as is as the 'bundle' key $comment->node_type, instead of 'comment_node_[node_type]'. Thus a comment inherited the fields of the node type (body, on a fresh HEAD), causing the additional rendering calls I was intrigued about in #438570-44: Field API conversion leads to lots of expensive function calls.
[edit: note that this kind of things shouldn't happen when #470242: Namespacing for bundle names is fixed)

So while rendering fields is still expensive, the benchmarks above do not reflect the real numbers for '300 comments with no field' (they are probably more accurate for 'HEAD, 300 comments with no field' vs. 'patch, 300 comments with one field'...)

Personally, I think the performance loss is quite worrisome.

Where extra SQL queries introduced or does the overhead comes solely from PHP code? It is weird how the self-cost of PDOStatement->execute changes while the number calls remains the same.

It looks like the culprit is actually drupal_render() but it is not clear yet how this matters as we shouldn't have introduced more drupal_render() calls -- we didn't add any fields to the comments so it is unclear why there is more rendering to do.

@Dries: see #58. The patch that was benchmarked had a bug: the loaded comments had no fields but 'took' the fields of their parent node type (the fields, not the values in them). Thus the benchmark ran with each comment having one (empty) 'body' field each, which were being rendered. (empty fields still trigger a render with template call, because some formatters or even field.tpl overrides might want to display something in case of 'no value').

Patch in #58 fixes the bug, and should show better results for the 'no field' benches.

@yched, awesome. Maybe @catch can benchmark things once more? Seems like a good way to detect important bugs. ;-)

That is a much prettier picture, and it looks like some follow-up patches could make it even faster.

Committed to CVS HEAD. Thanks all -- awesome patch.

Followup : #537746: fields on comments are not indexed

And also: #537750: Field UI for comments

Cleanup after #470242: Namespacing for bundle names:
#613630: Remove prefix on comment bundles names

Keeping track of this thread...