Suggested commit message

Issue #731724 by larowlan, andypost, dixon_, tsvenson | nbz: Convert comment settings into a field to make them work with CMI and non-node entities.

The most recent patch is here #2079093: Patch testing issue for Comment-Field, earlier reviews are here #1907960: Helper issue for "Comment field"

Problem/Motivation

  • With the new Entity API we shouldn't keep arbitrary data structures on entities like $node->comment.
  • Comment module is hard-wired to work only with node entity types.
  • Customizations of the comment for positioning and other elements are solved with one-off solutions, which we don't like

Proposed resolution

Convert $node->comment to a field that only will store the comment "status" for that entity.

If you want to enable comments on a certain bundle of an entity type you basically have to add such a "comment" field to that bundle. For example, the article node type will have a comment field called comment by default.

The comment settings, such as enabling comments by default, threading on/off etc, will be stored in the field instance settings for that comment field.
The comment form and all it's comments will be rendered through the field's formatter.

The comment entity (and hence the {comment} table) will be more generic and will hold the properties entity_id, entity_type and field_name.
The field_name property is an indicator through which field (on the "parent" entity) this comment was added. This property also acts as the bundle key for the comment entity.

This also means that we can have multiple comment fields on an entity. For example, you could have different comment fields (forms) depending on if you're "for" or "against" an article, with different sets of fields depending on that.

Remaining tasks

  1. Update issue summary
  2. Convert CommentUserTest to use entity_test_render instead of user entity
  3. Update This spreadsheet with comments from comment 348, comment 349 and comment 351, comment 359, comment 360, comment 362 of the review/testing issue.
  4. Finish the unresolved items from the same spreadsheet
  5. Final pass over the code to ensure that any @todos are encapsulated in follow up issues
  6. Fix ajax callback for new indicator in a multiple comment-field world.
  7. Another UX review (have send message via contact form to @tsvenson 26-9)
  8. Manual upgrade path test with a real D7 site with comments (eg from devel generate or real site) - done - contact @larowlan on irc for link to screencast - (note that image styles aren't upgraded, but that is a separate issue)
  9. Update API changes list
  10. Re-review test coverage
  11. Revisit history module integration
  12. Revisit access control on comment-admin screen
  13. Revisit hook_comment_access() may no longer be required in light of all entities having entity access controllers now

Out of scope tasks

  • New formatters - these add new functionality and can either be in contrib or follow ups
  • WSCCI conversions - eg comment_reply - there are existing issues covering these
  • Converting $comment->entity_id to an entity reference field, @berdir has indicated this is not required in this issue

User interface changes

  • Comment field ui now has it's own distinct place, on the same level as content type settings.
  • Comment settings are no longer managed on the content type settings page, instead these are field instance settings and are managed by editing the comment field on an entity

Field settings
field-settings.pngAdmin settings
admin-settings.png

Upgrade path

The upgrade path is written so that all node types that have comments enabled by default gets a new comment field created for it (even if no nodes of that type have any comments). Comment settings are migrated over the that field's instance settings.

Node types that doesn't have comments enabled by default but where at least one node of that type have comments also gets a new separate comment field created for that node type. Comment settings are migrated over the that field's instance settings.

Follow-ups/Related issues

Related
#1777956: Provide a way to define default values for entity fields
#2078387: Add an EntityOwnerInterface

Some major API changes

Contstants changed

COMMENT_NODE_OPEN => COMMENT_OPEN
COMMENT_NODE_CLOSED => COMMENT_CLOSED
COMMENT_NODE_HIDDEN => COMMENT_HIDDEN

To determine if an entity has a comment field

Use comment manager service.

<?php
// Check if there are any comment fields on nodes, returns array of field info
// consistent with field_info_fields() filtered to only comment fields.
$field_names =\Drupal::service('comment.manager')->getFields('node');
// Do something with a particular field.
if (isset($field_names['my_comment_field'])) {
 
$comment_count = $node->my_comment_field->comment_count;
}
?>

Creating a comment field using code

The simplest way is to use the manager.

<?php
// Add a field using default name of comment and default value of hidden.
\Drupal::service('comment.manager')->addDefaultField('node', 'page');
// Add a second comment field called admin_comments, defaulting to open.
\Drupal::service('comment.manager')->addDefaultField('node', 'page', 'admin_comments', COMMENT_ENTITY_OPEN);
?>

Comment configuration

All variables of form comment_XXX_%type are removed, use instance settings instead.

D7

<?php
$mode
= variable_get('comment_default_mode_' . $node->type, COMMENT_MODE_THREADED);
?>
D8

<?php
use \Drupal\field\Field.
$instance = Field::fieldInfo()->getInstance('node', $node->type, 'comment');
$mode = $instance->getFieldSetting('default_mode');
?>

Forum

Forum creates its own comment field called comment_forum.

D7

<?php
 
if ($node->comment == COMMENT_NODE_OPEN) {
   
// Do something.
 
}
?>
D8

<?php
 
if ($node->comment_forum->status == COMMENT_OPEN) {
   
// Do something.
 
}
?>

Comment statistics

Because we can have multiple comment fields on an entity/bundle - the comment_count etc (anything that was in node_comment_statistics table) have been moved from top level node object properties to properties of each field. Note that node_comment_statistics table is now comment_entity_statistics for consistency.

D7

<?php
$comment_count
= $node->comment_count;
?>
D8

<?php
$field_name
= 'comment';
$comment_count = $node->$field_name->comment_count;
?>

Sandbox

See comment-fieldapi5 branch of drupal.org/sandbox/larowlan/1790736

Original report by [nbz]

While Drupal 7 Core will not support comments for none node entities, I think since most other things are in place, there should be some groundwork done to make sure the eventual addition of this feature (in contrib for Drupal 7 or core for Drupal 8) should be done.

As a not-really-a-coder, I can see two ways this can be done:

1. new comment table per entity type. This has the benefit of not needing any core groundwork done (yet?). It has the downside that comment permalinking would need to be modified when this feature is eventually moved to core. (another benefit would be potentially smaller tables)

2. Add a new column to the comment table to store the entity type and then instead of the nid and eid. (by default the type would be node, the eid would be the same as the nid).

This has the advantage that permalinks will have a smooth transition.

Is there a third or better option?

If not, atleast a preferred direction may be needed to be chosen before the release of D7 in order to give contrib the right hints.

Files: 
CommentFileSizeAuthor
#526 731724_526.patch617 byteschx
PASSED: [[SimpleTest]]: [MySQL] 59,054 pass(es).
[ View ]
#510 drupal8.comment-module.731724-510.patch450.54 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 59,272 pass(es).
[ View ]
#510 interdiff.txt3.99 KBandypost
#509 interdiff.txt5.06 KBlarowlan
#505 731724-comment-field-505.patch449.77 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 59,033 pass(es), 2 fail(s), and 1 exception(s).
[ View ]
#503 comment-mcfield.patch449.85 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 59,215 pass(es).
[ View ]
#493 drupal8.comment-module.731724-493.patch422.51 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch drupal8.comment-module.731724-493.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#461 Foo - d83.taco_.png220.39 KBlarowlan
#460 interdiff.txt835 byteslarowlan
#460 comment-field.reroll.459.patch420.85 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment-field.reroll.459.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#460 Foo | d83.taco_.jpeg127.37 KBlarowlan
#449 Screen Shot 2013-09-05 at 10.21.14 AM.png27.58 KBwebchick
#425 sandbox-merge-263.patch390.07 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 57,087 pass(es).
[ View ]
#418 decouple-comment-node-731724.418.patch389.13 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.418.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#414 sandbox-merge-210.patch389.67 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch sandbox-merge-210_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#406 decouple-comment-node-731724.406.patch392.13 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.406.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#404 decouple-comment-node-731724.404.patch381.98 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 56,434 pass(es).
[ View ]
#398 decouple-comment-node-731724.398.patch378.03 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 55,924 pass(es).
[ View ]
#392 decouple-comment-node-731724.helper.169.patch377.93 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 56,436 pass(es).
[ View ]
#392 xhprof-output.zip17.87 KBlarowlan
#375 731724-comment-decouple-375.patch374.95 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 731724-comment-decouple-375.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#372 cleanup_default_value.patch6.74 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cleanup_default_value_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#372 rename-nested-interdiff.txt30.29 KBandypost
#372 731724-comment-decouple-372.patch382.28 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 52,627 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#370 731724-comment-decouple-371.patch338.42 KBrodrigoaguilera
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#362 interdiff-362.txt3.73 KBandypost
#362 731724-comment-decouple-362.patch365.79 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51,788 pass(es).
[ View ]
#353 sandbox-70pre.txt3.59 KBlarowlan
#353 sandbox-70pre.patch374.75 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 51,787 pass(es).
[ View ]
#337 0001-Field-storage-column-changed-from-comment-to-status.patch36.42 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0001-Field-storage-column-changed-from-comment-to-status.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#337 0002-Fixed-all-tests-after-conversion-except-FileFieldWid.patch19.78 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0002-Fixed-all-tests-after-conversion-except-FileFieldWid.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#337 0003-Fix-access-to-files-and-clean-up-remains-of-ER-trans.patch9.41 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0003-Fix-access-to-files-and-clean-up-remains-of-ER-trans.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#337 731724-comment-decouple-337.patch362.88 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 51,134 pass(es).
[ View ]
#335 731724-interdiff-335.txt34.52 KBandypost
#335 731724-comment-decouple-335.patch367.56 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 50,452 pass(es), 15 fail(s), and 31 exception(s).
[ View ]
#326 731724-comment-decouple-326.patch370.04 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] 50,809 pass(es), 13 fail(s), and 37 exception(s).
[ View ]
#326 731724-comment-decouple-326.interdiff.txt7.28 KBdixon_
#319 731724-comment-decouple-319.patch369.08 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,789 pass(es).
[ View ]
#311 731724-comment-decouple-311.patch363.73 KBdixon_
FAILED: [[SimpleTest]]: [MySQL] 50,468 pass(es), 4 fail(s), and 4 exception(s).
[ View ]
#308 731724-comment_field-307.patch365.01 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,353 pass(es).
[ View ]
#308 731724-interdiff-307.txt1.25 KBandypost
#305 731724-comment_field-305.patch364.34 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 49,341 pass(es), 4 fail(s), and 0 exception(s).
[ View ]
#305 731724-interdiff-305.txt15.71 KBandypost
#303 731724-interdiff-303.txt41.26 KBandypost
#303 731724-comment_field-303.patch359.59 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 49,335 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
#297 731724-interdiff-295.txt1.44 KBandypost
#297 decouple-comment-node-731724.295.patch359.48 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 49,023 pass(es).
[ View ]
#296 comment_forms_field_name.png6.71 KBtsvenson
#296 comment_forms_field_tables.png5.95 KBtsvenson
#291 decouple-comment-node-731724.291.patch359.4 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 49,010 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#287 731724-interdiff-287.txt2.55 KBandypost
#287 decouple-comment-node-731724.287.patch359.85 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 50,111 pass(es).
[ View ]
#286 comment_entity_field_default_visibility.png2.42 KBtsvenson
#286 comment_settings_entity_hidden_missing.png5.38 KBtsvenson
#285 comment_entity_field_settings.png19.61 KBtsvenson
#284 structure_comment_bundles.png8.16 KBtsvenson
#284 structure_comment_bundles_list.png5.42 KBtsvenson
#284 comment_form_field_comment.png8.86 KBtsvenson
#284 comment_entity_field_help_text.png14.44 KBtsvenson
#284 comment_settings_entity_item_help_text.png5.33 KBtsvenson
#284 comment_entity_field_comments_shown.png7.79 KBtsvenson
#282 decouple-comment-node-731724.280.min_.patch359.6 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 49,743 pass(es), 3 fail(s), and 0 exception(s).
[ View ]
#280 decouple-comment-node-731724.280.interdiff.txt1.72 KBlarowlan
#280 decouple-comment-node-731724.280.patch1.26 MBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.280_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#278 731724-interdiff-278.txt23.85 KBandypost
#278 decouple-comment-node-731724.278.patch358.74 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 49,795 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#275 decouple-comment-node-731724.275.patch369.54 KBandypost
FAILED: [[SimpleTest]]: [MySQL] 49,656 pass(es), 43 fail(s), and 15 exception(s).
[ View ]
#273 decouple-comment-node-731724.273.patch353.91 KBandypost
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#271 decouple-comment-node-731724.271.interdiff.txt170.81 KBlarowlan
#271 decouple-comment-node-731724.271.patch355.25 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 50,929 pass(es), 43 fail(s), and 15 exception(s).
[ View ]
#263 efq-comment-ui-test-do-not-test.patch1.94 KBxjm
#259 decouple-comment-node-731724.259.patch346.6 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.259.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#256 decouple-comment-node-731724.256.interdiff.txt529 byteslarowlan
#256 decouple-comment-node-731724.256.patch346.6 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 50,450 pass(es).
[ View ]
#253 decouple-comment-node-731724.253.interdiff.txt7.34 KBlarowlan
#253 decouple-comment-node-731724.253.patch346.66 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 50,416 pass(es).
[ View ]
#251 decouple-comment-node-731724.251.interdiff.txt593 byteslarowlan
#251 decouple-comment-node-731724.251.patch340.63 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 50,220 pass(es), 24 fail(s), and 3 exception(s).
[ View ]
#249 decouple-comment-node-731724.249.interdiff.txt1.04 KBlarowlan
#249 decouple-comment-node-731724.249.patch340.62 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 50,144 pass(es), 94 fail(s), and 11 exception(s).
[ View ]
#247 decouple-comment-node-731724.247.interdiff.txt588 byteslarowlan
#247 decouple-comment-node-731724.247.patch340.13 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 49,972 pass(es), 314 fail(s), and 359 exception(s).
[ View ]
#245 decouple-comment-node-731724-245.patch340.24 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#245 decouple-comment-node-731724.245.interdiff.txt1.37 KBlarowlan
#242 newcomment.PNG44.7 KBmbrett5062
#242 reply.PNG17.1 KBmbrett5062
#240 content_type1.PNG9.52 KBmbrett5062
#240 content_type2.PNG7.5 KBmbrett5062
#240 comment_bundle2.PNG7.76 KBmbrett5062
#239 comment_bundle.PNG30.8 KBmbrett5062
#239 commentbundle_error.PNG33.15 KBmbrett5062
#236 decouple-comment-node-731724.236.patch340.13 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 49,685 pass(es).
[ View ]
#228 decouple-comment-node-731724.228.interdiff.txt12.25 KBlarowlan
#228 decouple-comment-node-731724.228.patch340.21 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.228.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#226 decouple-comment-node-731724.226.interdiff.txt7.54 KBlarowlan
#226 decouple-comment-node-731724.226.patch332.84 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#222 731724-220-222.interdiff.txt763 bytespenyaskito
#222 731724-222.patch337.2 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 49,095 pass(es), 5 fail(s), and 4 exception(s).
[ View ]
#220 731724-220.patch337.2 KBpenyaskito
FAILED: [[SimpleTest]]: [MySQL] 49,080 pass(es), 5 fail(s), and 4 exception(s).
[ View ]
#216 731724-216.patch346.6 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 731724-216.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#214 731724-214.patch346.34 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php.
[ View ]
#212 731724-212.patch347.83 KBfubhy
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 731724-212.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#207 decouple-comment-node-731724.207.patch337.45 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.207.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#204 decouple-comment-node-731724.204.patch347.92 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 48,968 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#202 decouple-comment-node-731724.202.patch338.65 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] 48,768 pass(es), 2 fail(s), and 2 exception(s).
[ View ]
#197 decouple-comment-node-731724.197.patch337.27 KBdagmar
Test request sent.
[ View ]
#197 interdiff.txt12.19 KBdagmar
#192 decouple-comment-node-731724.192.interdiff.txt11.74 KBlarowlan
#192 decouple-comment-node-731724.192.patch338.66 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 48,638 pass(es), 21 fail(s), and 88 exception(s).
[ View ]
#187 decouple-comment-node-731724.187.patch336.34 KBdagmar
PASSED: [[SimpleTest]]: [MySQL] 48,517 pass(es).
[ View ]
#187 interdiff.txt701 bytesdagmar
#185 decouple-comment-node-731724.185.patch310.96 KBdagmar
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]
#185 interdiff.txt701 bytesdagmar
#183 decouple-comment-node-731724.183.interdiff.txt2.25 KBlarowlan
#183 decouple-comment-node-731724.183.patch337.97 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 48,469 pass(es), 20 fail(s), and 0 exception(s).
[ View ]
#174 decouple-comment-node-731724.174.interdiff.txt1.25 KBlarowlan
#174 decouple-comment-node-731724.174.patch337.91 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 48,449 pass(es).
[ View ]
#171 decouple-comment-node-731724.171.interdiff.txt2.97 KBlarowlan
#171 decouple-comment-node-731724.171.patch337.98 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 48,474 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
#167 decouple-comment-node-731724.167.patch337.62 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 48,358 pass(es).
[ View ]
#165 decouple-comment-node-731724.164.patch337.58 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.164.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#158 comment_in_comment.png33.65 KBnevergone
#155 d8_comment_decouple.png22.77 KBnevergone
#151 decouple-comment-node-731724.150.interdiff.txt3.11 KBlarowlan
#151 decouple-comment-node-731724.150.patch337.87 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 48,059 pass(es).
[ View ]
#147 decouple-comment-node-731724.147.interdiff.txt58.28 KBlarowlan
#147 decouple-comment-node-731724.147.patch335.87 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 47,998 pass(es), 6 fail(s), and 4 exception(s).
[ View ]
#137 decouple-comment-node-731724.137.patch328.92 KBlarowlan
PASSED: [[SimpleTest]]: [MySQL] 46,620 pass(es).
[ View ]
#137 decouple-comment-node-731724.137.interdiff.txt1.48 KBlarowlan
#135 decouple-comment-node-731724.135.interdiff.txt17.68 KBlarowlan
#135 decouple-comment-node-731724.135.patch328.92 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 46,599 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#133 decouple-comment-node-731724.133.patch326.37 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 46,454 pass(es), 0 fail(s), and 18 exception(s).
[ View ]
#133 decouple-comment-node-731724.133.interdiff.txt942 byteslarowlan
#131 decouple-comment-node-731724.131.interdiff.txt29.3 KBlarowlan
#131 decouple-comment-node-731724.131.patch326.36 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 46,383 pass(es), 14 fail(s), and 18 exception(s).
[ View ]
#128 decouple-comment-node-731724.128.interdiff.txt4.54 KBlarowlan
#128 decouple-comment-node-731724.128.patch322.92 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#126 decouple-comment-node-731724.126.interdiff.txt622 byteslarowlan
#126 decouple-comment-node-731724.126.patch322.48 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
#124 decouple-comment-node-731724.124.interdiff.txt11.27 KBlarowlan
#124 decouple-comment-node-731724.124.patch322.78 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.php.
[ View ]
#122 decouple-comment-node-731724.122.interdiff.txt18.33 KBlarowlan
#122 decouple-comment-node-731724.122.patch318.66 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 46,308 pass(es), 10 fail(s), and 43 exception(s).
[ View ]
#120 decouple-comment-node-737124.120.patch318.32 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 46,230 pass(es), 19 fail(s), and 39 exception(s).
[ View ]
#117 decouple-comment-node-737124.117.interdiff.txt119.48 KBlarowlan
#117 decouple-comment-node-737124.117.patch318.23 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-737124.117.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#115 decouple-comment-node-731724.115.interdiff.txt59.31 KBlarowlan
#115 decouple-comment-node-731724.115.patch283.64 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 45,515 pass(es), 101 fail(s), and 86 exception(s).
[ View ]
#112 decouple-comment-node-731724.112.patch250.07 KBlarowlan
FAILED: [[SimpleTest]]: [MySQL] 45,502 pass(es), 178 fail(s), and 790 exception(s).
[ View ]
#109 decouple-comment-node-731724.do-not-test.patch193.35 KBlarowlan
#103 decouple-comment-731724.do-not-test.patch155.39 KBlarowlan
#54 comment.731724-11.patch85.78 KBthehong
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment.731724-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#53 comment.731724-10.patch67.07 KBthehong
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment.731724-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#50 comment.731724-6.patch51.52 KBthehong
PASSED: [[SimpleTest]]: [MySQL] 46,227 pass(es).
[ View ]
#47 comment.3.patch12.92 KBthehong
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment.3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#46 comment.2.patch9.04 KBthehong
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment.2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Comments

Excellent suggestion, but I don't think this is suitable for D7.

This is something that will have to be done in contrib this cycle.

> Is there a third or better option?

I'm wondering whether a hypothetical entity_comment module could just provide a table with (cid, entity_type) and then abuse the {comment} table to use the nid field as the eid, with a join to get the entity type... You'd need to hack the CommentController class, but with hook_entity_alter you could just give it a different controller...

Version:7.x-dev» 8.x-dev
Category:task» feature
Priority:Minor» Normal

Bumping this is d8 material.

More people are interested in this, including me. See also http://drupal.org/node/893854 which is marked as a duplicate of this request. Anyone interested in developing some module for this?

I'd be happy to test any dev releases :)

Category:feature» task
Issue tags:+Framework Initiative, +API clean-up

Might make sense to wait for #375397: Make Node module optional to get in first, but then again, there's no hard-dependency on that.

Title:Add entity_type column to comment tableDecouple comment.module from node

I'am game, and able to develop if given enough time. But first up, what are the suggestions on how D8 (and hopefully backport to D7 ;) should handle comments.

To me personally, expanding the fieldable entity from D7 is the only way to go and selectable trough either a relation (using relations module) or perhaps as a block instead of piggy-backed onto node.

And yes I do think that comments can be moved out of core and into contrib for more flexibility and dependency on other modules.

So where to start?

So personally I think comment should probably stay in core, but at any rate that would need to be discussed by the whole community. If comments were to stay in core then you probably couldn't rely on Relation to do the linking, but then I'm not sure you would need to.

Comment module won't be removed.

I don't think that removing Comment module was ever considered to be an option here. This issue is precisely about decoupling it from Node module. Doing so will make Comment module the one and only API consumer in core that is an entity, which is "attached" in a "list" to another entity -- that's a pretty unique scenario and use-case, which on its own relies on lots of hard-coded stuff right now.

Let's get back to focus here. Decoupling Comment from Node requires us to figure out

  1. whether we can still store all comments on all other entities in a single table, or whether we need a separate table per commented-on entity; e.g., {comment_node}, {comment_user}, etc.
  2. where and how to implement comment settings. In light of that, @yched proposed to make comments an extra_field -- so current comment settings on the node type settings form would move to the settings of an extra field on the Manage Fields page. That said, "settings for extra fields" is a non-existing feature currently, if I'm not mistaken, so inventing that would be a prerequisite.
  3. lastly, a upgrade path, but that should be trivial, AFAICS.

So with regard to number 1.

We need each different entity bundle to be able to have a different comment bundle with different fields attached. You could store comments a single table and then use all the entity bundles as the bundle keys for the comment entity.

I think that's as messy as it gets, can you think of anything that would make it more complicated? Would you ever need one bundle to have multiple comment types?

Alternatively, as comments are things that are directly attached to other entities and a don't make sense on there own, is it better to have a 'Comment API' system that makes a different table for each comment-able (so add a 'commentable' property to hook_entity_info())

I have a thought to put out there, that I haven't seen discussed much. @sun mentioned in #18 that Comment is a very unique use case and relies on a lot of hard-coding in core. Comments are an entity that need to be tied to one or more content types, and not always with the same fields per content type. So why are we stuck on the concept of Comment as it exists today? All needs historically served by comments can be served by nodes, with the simple use of a node reference field. I've done it on several D7 sites because the core Comment module is unwieldy and not subject to core's access control system. Nodecomment *sort of* filled this role in D6, but the UX was a bit confusing.

My day job has been at "enterprise" proprietary CRM software companies for the last 7 years and I can spot when there is a trend towards over-architecting. Has anyone seriously considered the simplest solution? Add a new setting on content types that says "I'm available to be used for comments", and change the comment settings on content types a bit to say "Use this other content type for comments" alongside the existing flat/threaded, asc/desc settings? With that solution, Comments:
-Are essentially decoupled from nodes
-Get all the features of nodes for free
-Live as a top-level object
-Are easier for smart people to do interesting things with in contrib
-May even simplify the workload of Forum as @larowlan takes it over

I may be missing something hugely obvious to everyone else though, so please pick holes in this idea as a way of decoupling the two.

yes, there is the nodecomments module which does that.

With the entity system, using nodes as comments would be taking a step backwards into the days of D6 - entities were made exactly so that things that were not nodes could do everything that nodes could do. As entities comments are:
- Already essentially decoupled from nodes
- Have all the features of nodes (in that they are fieldable, have authors and post dates etc).
- Kind of live a top-level object (although I'm not sure we actually want this)

What sort of interesting things are you thinking of, out of interest? Comments have all of the standard entity hooks already - so there isn't that much you can't do/won't be able to do with the updated entity system in D8.

I'm trying to think of use cases where this would be genuinely helpful - and its places where you don't want there to be any difference between a node and its comments. So forums are kind of one of them - a reply to a post is also a post for example. But in most cases you want comments to be different and not loadable on there own. Maybe a better way to do Forums for example would be to have a Topic node without a body field, and have all of the posts as comments on that topic.

I think narayanis was maybe thinking of things like flag, nodequeue, voting -- things that contrib modules do to nodes that are cool. But for D7 all those will move to being on entities, surely :)

And I agree entirely with rlmumford's comment above.

> Maybe a better way to do Forums for example would be to have a Topic node without a body field, and have all of the posts as comments on that topic.

That's an interesting idea!

Yes, @Joachim, I was thinking of Flags, Voting API, Rules, Forums, simpler Views, simpler theming, etc. Even writing modules would be simplified since the object would be uniform. But the biggest one is workflow and moderation, which hinge on permissions. Currently comments are a global permission; you can view/post, or you can't. Turning comments into nodes gives you all the standard permissioning features, plus Content Access, Field Permissions, ACL, etc. from contrib.

If comments are nodes, you can control them by role. Consider a common use case: a site with view access to everything, but limited commenting until you gain enough reputation (i.e. a role). You can comment on X, but you can't comment on Y, or comment on other users, or moderate comments until you're considered "trusted".

The Stack Overflow family is a good example, see their privilege system. New users can answer questions, but you need more reputation to comment on different kinds of content. This is easy through the front end if comments are nodes, but otherwise impossible unless you add code and complication to extend both the comment system and the access control system.

This solution:
- reduces core code size
- simplifies permissioning while making it more powerful and granular
- simplifies the db table structure
- reduces the number of hooks a user must learn, thus lowering the learning curve
- allows contrib to do more with less code
- opens the door for things we haven't dreamed of yet

I still think there's nothing in nodes that you won't get from entities. Flags 3 is having "flag any entity", the new rules is all about entities, and I'm sure there's an entity_access module on the way. I think everything in your use case could be achieved out of the box with comment bundles already in Drupal 7, even before the entity API gets to core in D8. Aside from that D8 is having its permissions system totally overhauled anyway (see Drupalcon core conversation on granular permissions).

While its true that having them as nodes probably would reduce core code size (although it might not because of all the fiddlyness around permissions on node-nodes filtering down to permissions on comment-nodes) I don't think this is a biggy.

I totally don't think it simplifies the DB table structure (and even if it did, it would be by an insignificant amount compared to the complexity already)

Entity API has already reduced the number of hooks, or at least standardised them (there's a big API clean up initiative underway). With Entity API in core hooks will be even simpler with comments as an Entity! So I don't think it allows contrib to do more with less code and I don't think it will make hooking drupal any easier for first time module builders (you already kind of need books to build any non-trivial modules anyway!).

Finally, I'm sure that having comments as nodes can only be more restrictive than having separate entities.

The use case definitely can't be solved in D7 without custom code, which is my point of reference. I'm not 100% up-to-date on D8 so I wasn't aware of several of those things; consider me educated. I guess the Entities concept is what I've always treated Nodes as historically when building webapps and sites on Drupal.

Now that my solution has been sufficiently picked apart, it sounds like Entity solves all the issues of the current system, without adding complexity.

Again, the "make everything nodes" idea is obsolete with D7. We've introduced entities instead. Entities are the new nodes.

The new paradigm is "make everything entities". Any related or attached logic that's currently hard-coded or otherwise tampered into nodes needs to be abstracted to work with any kind of entity instead.

And that's what this issue is about. With regard to comments. There are other issues for the other remaining things only available to nodes already.

Back on topics, what is the preferred method for comments on entries?

1. One table with all comments (so we have an eid and a cid), or
2. Separate table per entity type (so multiple tables, such as comment_node, comment_user, when a cid is per node and two comments could potentially have the same cid bot not in the same entity type) etc?

For now, I'd take the easy path and just add an 'entity_type' column to {comment}, and rename 'nid' to 'entity_id'.

That is, because Drupal core does not provide a reliable facility for creating "dynamic" database tables yet (which would have to be captured in hook_schema() somehow, and maintained/updated in the long run). Also, separate tables are kinda "premature performance optimization" at this point in time - we actually don't have data to backup that idea currently.

Having a entity_type column in the comment table is all well and good, but what if you wanted different comment types (bundles) to be available for different node types? Presumably you would need an entity_type and entity_bundle(s) field wherever you stored your different comment types.

You mean the type of the node (or whatever) the comment is attached to? That deducible surely from the entity type and id. We do need comment bundles though -- d.org alone is a use case for this, with comments on issues requiring different fields from comment in the forum.

Assigned:Unassigned» tstoeckler

I'm assigning this to me for now.
I've started some work on this. I haven't gotten far yet, mostly touched the trivial parts.
I don't want to prevent anyone working on this, but it would be a waste to start from scratch.
So if someone is interested I'd post a work-in-progress patch.

When working on this a question arose with regards to property naming.
Currently we have:
$comment->nid
So, as was stated above, what comes to mind as the node-uncoupled version of that is:
$comment->entity_type
$comment->entity_id // Insert bikeshed about entity_id vs. eid here, or see http://drupal.org/node/1233394

But in #1018602: Move entity system to a module and #1042822: Developers need an $entity->entity_type property it seems we will be adding generic 'entity_type' and 'entity_id' properties to entities, so that $comment->entity_type would then be 'comment' and not 'node' (or whatever...).

So the question is what to call these properties.
I first thought 'parent_type' (or 'parent_entity_type') and 'parent_id' (or 'parent_entity_id'), but when you have threaded comments the parent of a comment can also be another comment. We currently have 'pid' (which is the comment ID of the parent comment) for that.

For now I'm going to go with 'host_type' and 'host_id'. It's not exactly beautiful, and there's also some
mild overlap with the 'hostname' property which is used when anonymous users comment, but it's the best I could come up with.

Alas, Discuss! :)

Since comments are attached to a node, the node is the attacher. So 'attacher_entity_type' could make sense. But host_type' seems ok to me too.

And here is a link that reply project which does seem like a much modernized comment.module.

@moshe weitzman: Thanks for the pointer, I'll keep an eye on that. Reply.module is a complete rewrite though and breaks with a couple of design decisions of comment.module (both API and UI). And while that is not bad in itself, I think we should try to change as little as possible in this issue and not do a complete rewrite.

One other thing that has come up in working on this, is the {node_comment_statistics} table. Comment module keeps a record for each node, how many comments it has, when it was last posted to, etc. The question is, how can we have something similar that is entity-type-agnostic? More specifically, how can we get to a point where we can do:

<?php
$entity
->status;
$entity->created;
$entity->updated;
$entity->uid;
?>

If $entity is a node, all of these work fine, but otherwise it doesn't.

That also begs the conceptual question, whether all these properties make sense for any entity, or whether they are node specific (comment_node_statistics.module?). $node->uid is the user who created the node, but $user->uid is the user himself, no matter who "created" him. And there is no $taxonomy_term->status, should there be? Etc.

FWIW, I'd answer all questions in #39 with "yes". I think we should have and badly need a standardized "base schema" for all entity types. Created, updated, status, and author ID are needed for most operations and use-cases, regardless of entity type, and are going to be mandatory if we intend to dive further into the revisioning, wysiwyg, and enterprise content management territory. Implementation details are subject to change, but calling out a standard base schema on status quo would already be a big step forward.

That's very good. I had kind of thought the same thing, but wasn't really sure if there are enough use-cases or if comment would be the only consumer. But you just sold me :).

I opened #1295148: Maintain common properties/statistics about entities.

I think comment's table per entity, if it requires one, made drupal more platform like, with each entity responsible for their own comment table.

Also there usually just going to be one entity+comment per page (request), another pair would be too much to take for the visitors.

> I think comment's table per entity, if it requires one, made drupal more platform like, with each entity responsible for their own comment table.

That means you have the database schema dependent on the entity info.
I encountered some *interesting* problems when I tried doing that in a sandbox recently...

Status:Active» Needs work
StatusFileSize
new9.04 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment.2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

This patch
- move 'comment' column in node/node revision defintion to comment.
- rename variables from comment_{feature}_{node_type} to comment_{feature}_{entity_type}_{bundle_type}.
There are still a lot of things to do.

StatusFileSize
new12.92 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment.3.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Continue #46, patch in #49:

  1. move 'comment' column in node/node revision defintion to comment.
  2. rename variables from comment_{feature}_{node_type} to comment_{feature}_{entity_type}_{bundle_type}.
  3. Rename {node_comment_statistics} to {comment_entity_statistics}, add entity_type field, rename nid to entity_id
  4. convert hook_node_load() to hook_entity_load
  5. rename constants COMMENT_NODE_* to COMMENT_ENTITY_*

StatusFileSize
new51.52 KB
PASSED: [[SimpleTest]]: [MySQL] 46,227 pass(es).
[ View ]

working on comment_form().

Assigned:tstoeckler» thehong
StatusFileSize
new67.07 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment.731724-10.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

- Better install functions, schema alter.
- Various fixes.

StatusFileSize
new85.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch comment.731724-11.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Basic comment, links worked.

I don't want to prevent anyone working on this, but it would be a waste to start from scratch.
So if someone is interested I'd post a work-in-progress patch.

Dude, that was a massive duplication of effort!!! :(
I didn't review your patch in detail, but glancing at it, I had most of the changes in your patch on my local dev-branch.
Sorry, if I didn't make that clear enough, but that's kind of why I assigned the issue to me.
In general I think it would make sense to split this into a sandbox branch, instead of posting a bunch of patches in one issue. Especially because this depends on a dozen other entity-related patches.

Can you submit your patch or link me to your sandbox?

tstoeckler, Please post the work-in-progress patch or link to the sandbox. It doesn't do anyone else any good if your code remains on your local dev-branch. Thanks.
thehong, Perhaps you should start a sandbox instead of posting the stream of patches? Although, I like to see the bump with each patch.

Well I could Set up a sandbox, I first have to look at the patches above in detail whether my local work or thehong's would've be a better starting point...

Fully decoupling comment could just be done by moving the feature into a new "comment" field type. Making it work almost as taxonomy works (in UI point of view). This would allow:
* Configuration at the field level possible
* Configuration at the instance level possible
* Configuration at the entity that carries the field level possible
* No variables being used at all in the system
* No need for "extra" fields (this feature is weird anyway)
* Attachable to anything, re-usability maximisation
* No more hardcoded SQL table, but a field schema instead, also easier to use on another storage engine (MongoDB?)

As cons, I'd say it'd need:
* More portable UI (currently attached to node)
* A lot of refactor
* Porting comments from nodes to the new field schema

It doesn't sounds that difficult, mostly a long work. One of the biggest task in this would probably UI.

EDIT: But someone must already have thought that, my guess is there is a lot of other problems behind I don't see.

I think comments should remain an entity with fields tbh, since a lot of people use comments for more then just a single multiline textbox. Think of fivestar ratings, adding files or photo's, updating node fields in for example issue queues and the list could go on...

I do think comments should be able to be linked to more then just nodes though, so instead of just a nid, use a type-identifier and the relevant id, or work with entities... I'm not really into the code itself, though downgrading comments to a single field wouldnt do it for a lot of people...

just my 2c

I'd also like comments to remain an entity. What we really need is Entity reference or Relation in core (and Entity tree, ofc) :)

Comments can both provide a field, for attaching the feature to a certain other entity, but still continue to use the "Comment" entity for storing individual comments: this doesn't sound incompatible. The difference would be that comment form + comment thread display then becomes the field formatter instead of relying on page/content/form alteration.

Right - thats what reply module does (as mentioned in #38 . It is a good model.

Yes, I'm quite sure it would be really valuable to have the comment module doing the same, this would mean no alteration of core anymore -at all- except for comment administration (approval queue) page.

Really need this in D7, as more and more data structures become entity.

Is this possible to back port to D7?

#61 / #64 sound like a good idea.
I like what Reply module did (though the code itself is not exactly nice).

That is basically a rewrite of Comment.module. How do we do something that big? Decouple first, introduce the field later?
Or just create a nice big sandbox around which a hundred bike sheds can be built and painted?

@bojanz: "though the code itself is not exactly nice" - explain.

@Bojanz #68 : I think that something "this big" can be achieved by one or two developers within a few weeks: the usual core anti-big commits complex is the constant that makes this refactor slow in the end. I'd proceed this way:
* Name one to three official "comment micro-initiative" maintainers;
* Make them do a nice sandbox cloned and merged often over core's one;
* Long period of architecture and planning with everyone involved;
* Fast two-brain max coding a few weeks with then incremental patches and improvements;
* Parallelize with two-brain max UI coding once the core architecture is in place.

This sounds ideal and in real life it never happens this way, but it doesn't hurt to do some planning I guess.

For the code itself:
* I'd start by modeling the entities and field, keeping actual comment module compat by proying stuff;
* Then UI guys can start working on moving UI bits slowly over field formatters and stuff as soon as this first step is finished, thus slowy removing all core alteration;
* Parallelize that with original developers that modeled the entities doing the the storage side refactor blindly, using only unit test and not relying on the UI in a parallel branch;
* Once UI guys finished, the original developers can merge back their storage changes and continue working on master until all tests pass.

Still sounds really ideal, but it may work: the first iteration can be meta-fields that actually just provide formatters and stuff and proxying data from the original comment storage by transforming it at runtime, fields makes an excellent interface that would allow both storage and UI being worked out in parallel in different branches as soon as the fields and entities structures (the field interface) is stable.

@ivanjaros
Sorry, that sounded harsher than I intended.
Not exactly nice == needs coding standard fixes, proper comments (especially in the field file. Standards not followed, sometimes written in a foreign language).
The reply_load_instance duplicates field_info_instance() for no reason, reply_save has that odd field validation call at the top (shouldn't the controller do that).
Nothing that can't be fixed with an OCD mindset and an hour of work.

I think creating a new comment field is easier than rewriting the core comment module. :)

vinoth.3v, if you read the whole issue, you'll see that the situation is not as simple as that.

I guess we need an issue summary.

We have here an architecture paradigm that we need to surpass.
Because even though comments are entities and are separated, it is coupled in a hardcoded way into node.module.

vinoth.3v, that's basically it.

BTW: Any help with finishing Reply module is welcome.

Hi,
Would love to see a sort of comment/reply module for D7 based on entities as such that it can also be used as product review module (drupa commerce).
Hopefully not off topic my comment.

Greetings, Martijn

Hi all,

I tried to read the entire issue discusion and think that decouplling is a must. I know a little module that is a sandbox and it's called "Entity wall". This module attach a fieldable comment system to any possible entity (I think). Of course that doesn't have the whole functionality that comment module has but it can be a model to see how this is acomplished, and perhaps a way to get some ideas.

Sorry because I have not enough skills to help the comunity hardcoding this, but in my mind, the entity wall is a good place to start thinking about a comment system entity based and decoupled from node. As I can see now, seems strange that this node-decoupled-entity-based comment system is not planned before, as Drupal tendence is going in to entities.

Hope that this can help someone.

Assigned:thehong» larowlan

Transferring in from #1790764: [meta] Convert comment module to field_api which was marked as duplicate of this issue.

There was a mention about module http://drupal.org/project/reply
It looks pretty similar approach in http://drupal.org/sandbox/larowlan/1790736

Status:Needs work» Active

Committed first pass at upgrade path to my sandbox for this issue:
http://drupalcode.org/sandbox/larowlan/1790736.git/commit/6f541e7

Committed updates to comment.pages.inc and comment.token.inc to sandbox see #80.
Work depends on #1026616: Implement an entity render controller and #1696660: Add an entity access API for single entity access.
Tagging

We've got a simple entity reference field in core with #1696640: Implement API to unify entity properties and fields. That will make it easier to make comment module entity agnostic.

More sandbox commits

08a0026 Cleanup of reply links in theme preprocessing
961a43d Ported storage controller to use entities and cleaned up some leftover node hooks in comment.module
21b0c84 Comment and CommentFormController updated to use entity instead of node
0d07e25 Updated comment.admin.inc to use entities
4e846cf First pass over comment.module complete

Ready to start testing and porting tests

Um. this issue is tagged "avoid commit conflicts", but there is no patch newer than October of 2011 and the issue is not even in "needs review". So I don't know how to avoid commit conflicts. Please either attach a newer patch that I can avoid conflicting with, or remove the tag so I don't have to think about this when making commits. Thanks! See
http://drupal.org/node/1207020#avoid-commit-conflicts

Until this is clarified, I'm removing the tag. If there is something specific (a patch) that I can avoid conflicting with, feel free to point it out and add the tag back.

More sandbox commits

* e9c5495 More work on comment bundle field ui
* 95461a3 Comment display and field settings UI functional
* c2079e1 Comment template cleanup
* 4c7352f Comments now save and can be viewed
* 286551a Updated forum module's use of comment api
* 07163aa Updated rdf module's use of comment api
* ca09532 Updated tracker module's use of comment api
* 1cb1190 Updated search module's use of comment api
* fb46062 Create body field on comment field, testing comment replies, updated node schema to remove comment column
* 005406a Standard install profile updated to use comment field, Drupal installs

This is now largely functional except for the menu handling of the comment field ui stuff when multiple comment fields exist and porting of tests. Those are the next two tasks

@larowlan: Wowza! Can you use git diffup to keep us updated? :)

I started a sandbox for a D7 module that does exactly this a few weeks ago: http://drupal.org/sandbox/sumsi/1778262

I would like to assist with this issue. Can you grant me access to your sandbox @larowlan? Would be great to discuss this further on IRC too!

How about replacing comment module with Reply module? Me and James Williams have done quite a bit of work on it(althoughr I dont have much time right now to work on it) so why reinvent the wheel? I'm absolutely sure that upgrade path from comment module to reply module will not be an issue. Otherwise we've just wasted a lot of time working on something that won't be used.

I was little bit afraid of this 'empty' peridod, if it is worth to create something until D8 comes out or just wait. But I've decided to start working on it anyway. So any contributions(patches)/maintainers are welcome.

@sun, have done
@fubhy, you've got write access.
@ivanjaros - James already had write access - you now do too.

The remaining tasks are listed on the sandbox project page - if you tackle a chunk, please create and issue in the project page issue queue, assign it to yourself and update the project page with the link.

James/Ivan if I can't resolve the field_ui menu handling to work how I'd like it - we may have to revert the bundling handling to how it's handled in reply module - ie you manage comment bundles elsewhere to the entity they're attached too and then select the bundle you want from the field config.

I wanted it to work like this:

tabs for 'article' node type:
'edit': edit the article node type
'manage fields' manage fields on article node type
'manage display' manage display for article node type
'comment fields' parent tab for when article node type has one or more comment fields
--'comment field 1' parent tab
----'manage fields' manage fields for comment field 1
----'manage display' manage display for comment field 1
--'comment field 2' parent tab
----'manage fields' manage fields for comment field 2
----'manage display' manage display for comment field 2

I'm part way through attempting to create this structure using comment_menu_alter.
I've got as far as adding the 'comment fields' parent tab and will continue with it later in the week in #1805932: Resolve menu UI issues on 'Comment fields' tab of bundle field ui pages.

More commits:

e9d476d Merge branch '8.x' into comment-fieldapi
ec42003 Remove node_revision.comment column too
9cee06e Cleanup handling of upgrade path to avoid collision between old comment property and new comment field

Should have said that ideally I'd like a working (green) patch of this ready by next Monday to allow time to get this in before freeze.

Is this going to provide a solution for D7 as well? If not, I really like ivanjaros' suggestion to put some effort into Reply module so people running D7 sites will actually also have a chance to have commentable entities. I personally need this functionality really badly and am willing to chip in for it, as I can't contribute code yet.

This is purely D8, for D7 you'll need reply or similar.

Okay, so let's get back to the code in the sandbox. I reviewed it this morning...

1. We should use the field collection pattern instead of forcing different comment bundles per host field, host entity type and host entity bundle. That way we can share the same comment entity bundle across many different host entity types and bundles. If a sitebuilder wants to provide separate comment bundles he/she would simply add a new field instead of re-using an existing one. To me that would make much more sense. I already started copying that over from my sandbox (that's how it is workign there currently). But I am open for discussion on this of course.

2. Taking 1. into consideration I am not entirely sure that it would still make sense to keep the 'Comment fields' / 'Comment display' tabs on the host entity field UI pages. This would be confusing considering that the same fields and display settings might be shared across several host entity types and bundles. Also, this would not even work out-of-the-box and would require custom field UI code because the field UI generated from the bundle information is unique and does not allow us to place the same field UI in multiple places. So... I would vote for a separate 'comment' tab in a 'admin/structure' sub-path which would then be our comment Field UI with additional information about which entity types and bundles are currently using a certain comment bundle in an additional table column. Again, this is how the code in my Sandbox currently does that for D7. So I am probably extremely biased in this regard. However, I came up with the same ideas back when I was writing that code and still think that it's the best solution for us. I am open for discussion on this too, obviously :).

The following are probably follow-up issues:

3. Once we are done with the initial patch we should think refactoring the 'Author' and 'Subject' code. These are currently hard-coded properties with, especially in regard to the 'Author' property, a lot of strange hard-coded logic. I am nearly 100% sure that we should convert the 'Subject' property to a field. I am not 100% sure about the Author property... We could still default to having the subject field automatically attached whenever a new comment bundle is being created. I am just thinking that we can do better than forcing our sitebuilders to use the subject field. Also, consider if we want to use the pimped comment entity in a next-generation forum implementation. Most of the forums out there don't use subjects for every single post. And I wouldn't want that for my forum as well... I'd just want a simple 'body' field there.

4. I'd like to ditch the settings for where the comment form should be displayed and, instead, have an extra field for that as well. That way the sitebuilder can freely decide how he/she wants to arrange the comment-related parts on the host-entity display. In general, we should make much more use of extra fields for this. We could highly benefit from them. I would suggest to implement the following extra fields:

  • Commenting links (host entity) -> (reply, jump to latest comment, jump to comment form, etc.)
  • Comment form (host entity)
  • Comment links (comment entity) -> (reply, approve, etc.)
  • Author (comment entity)

The comments itself should be rendered in the actual comment field. That way we would be able to move them around on the host entity display as well. I think that we can easily default to having the comments on the same page as the host entity display. If the sitebuilder wants to have the comments on a separate page that would work as well by simply disabling the comment-related fields in the host entity display and creating a separate page with views that lists the comments :).

5. Let's look at http://drupal.org/project/entity_tree and http://drupal.org/project/tree to see how we can maybe improve our code for threading. I think we should have a simple API for building tree-structures in core.

6. Building proper per-user view statistics support is something we should do in a follow-up as well. I don't like that current code for that. I don't have a solution either tho :P.

I will post more soon!

@fubhy - I agree with most of your points.
The refactoring of the bundle logic to mimic field_collection makes sense and solves the issues I was discussing at #92.
If you've got code for this, even better.

The sandbox does currently render the comments as the field.
I intend to move the form location, number per page etc to formatter settings.
Although I hear what you're saying about separating the two for the purposes of being able to move the parts about. However this might be moot if blocks and layout brings a panels-like approach to entity rendering, as you could add the same field twice but with different formatter settings.

Lee

Regarding Author, perhaps we can keep in mind #355513: Create an anonymous user API. That issue has stalled, but it would be nice if Author were just a standard uid reference field like other Entities. I wish for for a comment module that simplifies everything - only logged in users and no threading. One could put a quick registration Field on the comment entity (a Field API version of http://drupal.org/project/comment_registration) and thus we conveniently transform commenters into user accounts during comment submission. This is scope creep, but good creep!

Thanks moshe... You are basically saying what I didn't dare to say. Unrelated to this issue or not, comment module needs to be simplified. The author stuff is a good starting point. After all, any Sitebuilder can already add that functionality at will by adding custom fields to the comment entity that are only exposed to anonymous visitors. Having all that stuff hard-coded into the comment module is simply not good.

Threading is something that I would like to see implemented on top of the comment module and not baked into it as well. But I am sure that some people would disagree that removing it without a replacement in core is a good idea.

Marked #1776076: Convert comment module configuration to CMI as postponed in favour of this issue, if feature freeze beats us, will reset that issue to active.

Friendly reminder: Perfection is the perfect enemy of making progress.

This issue is labeled "Decouple Comment from Node." Not "Rewrite Comment module to kick ass." ;)

Most of the recently discussed ideas in here are actually irrelevant for the task at hand. Most of them could be applied to Comment module without decoupling it from Node. But that's the exact goal and task of this issue.

I'd strongly recommend to evaluate every possibly suggested change on the basis of whether it strictly has to happen for this issue — if not, file a follow-up issue. That's the recipe for getting things done.

Yep, the mentioned suggestions would definitely be follow-up issues. I think we just wanted to point them out. :)

StatusFileSize
new155.39 KB

More progress in sandbox

* 907359a Working on test coverage, CommentAnonymousTest will pass once remaining formatters are completed
* 154631c Fixes incorrect setting name in tests
* b63dddf Revert attempt to use testing profile
* 96bdde2 WIP for CommentTestBase
* f5ca9c9 Change bundle key for comment entities
* 222ace4 Change description of menu item for admin/structure/comments
* f8cad2e Cleanup comments in comment.admin.inc

Attaching WIP patch.
Comment tests starting to look a lot better. Need to finalise porting comment_node_view stuff like links etc to formatters as there are tests for those. That is next task before returning to tests.

This issue inherently fixes #1426062: Cannot uninstall comment when node is disabled., but that might land before this. Just a heads up.

Thanks TIm, will follow that one.
Latest commits

854aa8e Add missing type hinting to comment.api.php
d0bbf17 More work on test failures

Starting to see more green in test results :)

tim-e has come on board to help with the formatters, thanks Tim.

More sandbox commits
No more fatals for Comment module test suite

3015 passes, 414 fails, 534 exceptions

Slowly getting there!
78ab628 Updating to use new entity render controller, no fatals during testing

More sandbox commits

2f8ca57 CommentCSSTest does not cause fatals
7e7f330 CommentTokenReplacement now passes
371a8c9 CommentStatistics now passes
f9460c7 CommentRssTest passes, moved hook_node_view links, search, rss features to hook_entity_view

Down to 61 fails in Comment suite, forum and tracker tbd.

All comment module tests now passing

3270a56 All comment module tests now pass

All core modules now passing except for views, however have already resolved views fatals.
Views testing results:
2769 passes, 2 fails, 22 exceptions, and 786 debug messages

* b494df6 Views tests no longer throw fatals
* 5d03f58 WIP views
* de4d760 Language and views tests updates
* a8361da Tracker tests now pass
* 40564fa Forum module tests pass

Most of these are down to missing {comment}.nid

@dawehner has offered to take a look at the code and report what's missing from the views integration. Thanks Daniel

StatusFileSize
new250.07 KB
FAILED: [[SimpleTest]]: [MySQL] 45,502 pass(es), 178 fail(s), and 790 exception(s).
[ View ]

Let's see what the bot says.

Still to do:
Tests that don't use node.
Remove last few references to node table in queries that add node_access tags.
Upgrade path tests.

Follow ups:
Update to use new entity access api when it lands
Update to use new history module for other than nodes when it lands

Thanks to dawehner, fubhy and tim-e for help with this.

Status:Active» Needs review
StatusFileSize
new283.64 KB
FAILED: [[SimpleTest]]: [MySQL] 45,515 pass(es), 101 fail(s), and 86 exception(s).
[ View ]
new59.31 KB

d4bc47a Fixes some test failures, removes joins against {node} unless wrapped in module_exists, adds CommentUserTest
c09b92a Adds first attempt at comments on user tests

Expecting failures, just want to see how many

Still to do:
*Upgrade path
*Finish CommentUserTest

Status:Needs review» Needs work
StatusFileSize
new318.23 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-737124.117.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new119.48 KB

Let's see what the bot says now, expecting about 12-16 failing test classes.
Work from the weekend:

b4df36b Remove reference to node_comment_statistics in comment_update_index
dbc39a9 Removed comment_modules_enabled(), no longer needed as node types don't automatically get comment fields
e27ad53 Cleaned up references to node_comment_statistics in node views plugins
eaf5180 Fix NodeAccessPagerTest
d374027 Fix NodeTitleTest
1e73610 Tracker module passing tests
889b8a8 Work on making more views tests pass
3bb429b RDF modules passing tests
f17a855 Work on passing search tests
fcb36fc Work on making upgrade tests pass
0e92b85 Making Module API, Entity filtering tests pass
8628dd8 Work on Comment views integration
cfb75c8 Make field_sql_storage update work with fields created during update process
4013d29 Making FileFieldWidgets pass tests
03d17f3 Make HTML Image filter Secure tests pass
367aa1f Work on making module api tests pass
ba66ae4 Work on making user tests pass

Still to do:
*Upgrade path tests
*Finish CommentUserTest
*Get views stuff working (Comment User UID tests throwing fatals at the moment)
*Resolve issue with module api test and table field_data_comment_forum

Status:Needs work» Needs review
StatusFileSize
new318.32 KB
FAILED: [[SimpleTest]]: [MySQL] 46,230 pass(es), 19 fail(s), and 39 exception(s).
[ View ]

Re-roll

StatusFileSize
new318.66 KB
FAILED: [[SimpleTest]]: [MySQL] 46,308 pass(es), 10 fail(s), and 43 exception(s).
[ View ]
new18.33 KB

Work from last night and this morning:

0e7c216 Ensure h2 wrapper output for all entity types
c09366d SearchCommentTest now passes
83497c8 Work on CommentUserTest and some issues identified with comment_entity_insert

@dawehner has provided patches for views issues, will apply and post those tonight.

StatusFileSize
new322.78 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/views/lib/Drupal/views/Tests/DefaultViewsTest.php.
[ View ]
new11.27 KB

Once again with patches from dawehner and some extra calls to comment_add_default_comment_field in views tests so we get at least one relationship.

StatusFileSize
new322.48 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new622 bytes

groan, long day

StatusFileSize
new322.92 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new4.54 KB

meh

Issue tags:+VDC-integration
StatusFileSize
new326.36 KB
FAILED: [[SimpleTest]]: [MySQL] 46,383 pass(es), 14 fail(s), and 18 exception(s).
[ View ]
new29.3 KB

Should be down to just exceptions now.
Upgrade path is tested in LanguageUpgradePath as it tests for comments.

StatusFileSize
new942 bytes
new326.37 KB
FAILED: [[SimpleTest]]: [MySQL] 46,454 pass(es), 0 fail(s), and 18 exception(s).
[ View ]

updates bundle for forum tests to match removal of field_attach_bundle_rename from upgrade path

StatusFileSize
new328.92 KB
FAILED: [[SimpleTest]]: [MySQL] 46,599 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
new17.68 KB

fingers crossed...

StatusFileSize
new1.48 KB
new328.92 KB
PASSED: [[SimpleTest]]: [MySQL] 46,620 pass(es).
[ View ]

this time?

After database update (empty site, fresh Drupal 8 and SQLite):

comment module
Update #8004

Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1 table comment_0 has no column named nid: CREATE INDEX main.comment_0_comment_num_new ON comment_0 (nid, status, created, cid, thread); ; Array ( ) in Drupal\Core\Database\Connection->query() (line 548 of /media/data/Virtualhost/drupal.test.loc/core/lib/Drupal/Core/Database/Connection.php).

Was just skimming through to look what you're doing with the preprocess functions, see #1810710: Remove copying of node properties wholesale into theme variables

+++ b/core/modules/comment/templates/comment.tpl.phpundefined
@@ -30,7 +30,8 @@
- *   - by-node-author: Comment by the author of the parent node.
+ *   - by-{entity-type}-author: Comment by the author of the parent entity,
+ *   eg. by-node-author.
  *   - preview: When previewing a new or edited comment.
  *   The following applies only to viewers who are registered users:

I'm not sure if it makes sense to have a dynamic css variable. If at all, it should still have a catch-all, so that you can easily theme comments of a user-author (weird thing, know) and a node-author?. Maybe both parent-author and comment-author? Also, parent entity could also be a parent comment? Do you have a name for "the thing the comment is attached to"?

Looked a bit through the code, some random feedback, not sure if the code is actually meant to be reviewed like this already :)

And ouch at converting this to CommentNG, that's going to be a fun task ;) But could also allow for some really cool stuff, like $comment->parent->entity->uri() :)

+++ b/core/modules/comment/comment.admin.incundefined
@@ -82,23 +127,24 @@ function comment_admin_overview($form, &$form_state, $arg) {
-  $query->join('node', 'n', 'n.nid = c.nid');
-  $query->addField('n', 'title', 'node_title');
-  $query->addTag('node_access');
+  $query->addTag('entity_access');

There is no entity_access query alter support yet and if there were, this wouldn't be enough to make it work. We need to atleast provide meta info about which entity type it is.

Previously, we were able to only list comments of nodes that the user was able to see. If removing that means we're not getting any test failures then we're missing test coverage :)

+++ b/core/modules/comment/comment.admin.incundefined
@@ -82,23 +127,24 @@ function comment_admin_overview($form, &$form_state, $arg) {
   // We collect a sorted list of node_titles during the query to attach to the
   // comments later.
   foreach ($result as $row) {
+    $entity_ids[$row->entity_type][] = $row->entity_id;
     $cids[] = $row->cid;

Old comment :)

+++ b/core/modules/comment/comment.api.phpundefined
@@ -91,7 +95,7 @@ function hook_comment_view(Drupal\comment\Comment $comment, $view_mode, $langcod
- * @param $build
+ * @param array $build
  *   A renderable array representing the comment.
  * @param Drupal\comment\Comment $comment
  *   The comment being rendered.
@@ -99,7 +103,7 @@ function hook_comment_view(Drupal\comment\Comment $comment, $view_mode, $langcod
@@ -99,7 +103,7 @@ function hook_comment_view(Drupal\comment\Comment $comment, $view_mode, $langcod
  * @see comment_view()
  * @see hook_entity_view_alter()
  */
-function hook_comment_view_alter(&$build, Drupal\comment\Comment $comment) {
+function hook_comment_view_alter(array &$build, Drupal\comment\Comment $comment) {

Careful with making a huge patch even bigger with unecessary doc fixes.

+++ b/core/modules/comment/comment.api.phpundefined
@@ -170,5 +174,31 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
+function hook_comment_entity_access(Drupal\entity\Entity $entity) {
+  $type = $entity->entityType();
+
+  if ($type == 'comment') {
+    return COMMENT_ENTITY_ACCESS_DENY;

I assume this is just a temporary measure until with have generic entity access support, which is close to being commited?

+++ b/core/modules/comment/comment.installundefined
@@ -14,69 +17,69 @@ function comment_uninstall() {
+  $comment_fields = comment_get_comment_fields();
+  $entity_info = entity_get_info();
+  foreach ($comment_fields as $field_name => $info) {
+    foreach ($info['bundles'] as $entity_type => $bundles) {
+      foreach ($bundles as $bundle) {
+        $entity_detail = $entity_info[$entity_type];
+        if (!empty($entity_detail['base_table']) &&
+            !empty($entity_detail['entity_keys']['id'])) {
+          $table = $entity_detail['base_table'];
+          $schema = drupal_get_schema($table);
+          // Insert records into the comment_entity_statistics for entities that
+          // are missing.
+          $query = db_select($table, 'e');
+          // Filter by bundle.
+          $query->condition($entity_detail['entity_keys']['bundle'], $bundle);
+          $query->leftJoin('comment_entity_statistics', 'ces', 'ces.entity_id = e.' . $entity_detail['entity_keys']['id'] . " AND ces.entity_type = '$entity_type'");
+          if (!empty($schema[$table]['fields']['created'])) {
+            $query->addField('e', 'created', 'last_comment_timestamp');

I'm not sure that's something we can do on hook_enable(), this operation could take forever. Imagine executing this on drupal.org.

Maybe needs a separate backend task that executes this as a batch?

+++ b/core/modules/comment/comment.installundefined
@@ -14,69 +17,69 @@ function comment_uninstall() {
+  // Set default value of comment_maintain_entity_statistics.
+  state()->set('comment_maintain_entity_statistics', TRUE);

Is that really state and not config? state() should imho only be automatically managed data that the user does need to see/know about. This sounds like a setting that can be enabled/disabled?

+++ b/core/modules/comment/comment.installundefined
@@ -103,12 +106,26 @@ function comment_schema() {
-      'nid' => array(
+      'entity_id' => array(
         'type' => 'int',
         'unsigned' => TRUE,
         'not null' => TRUE,
         'default' => 0,
-        'description' => 'The {node}.nid to which this comment is a reply.',
+        'description' => 'The entity_id to which this comment is a reply.',
+      ),
+      'entity_type' => array(
+        'type' => 'varchar',
+        'not null' => TRUE,
+        'default' => 'node',
+        'length' => 255,
+        'description' => 'The entity_type to which this comment is a reply.',

An entity reference field with a dynamic type, this is something we talked about in Barcelona when working on the Entity Field/Property API. That's going to be an interesting one to implement :)

+++ b/core/modules/comment/comment.installundefined
@@ -382,6 +430,228 @@ function comment_update_8003(&$sandbox) {
/**
+ * Update the comment_node_statistics and comment tables to new structure.
+ */
+function comment_update_8004(&$sandbox) {
+  // Remove the comment_node foreign key.
+  db_drop_index('comment', 'comment_node');
+  // Remove the comment_nid_langcode index.
+  db_drop_index('comment', 'comment_nid_langcode');
+  // Add the entity_type and field_name columns to comment.
+  db_add_field('comment', 'entity_type', array(

Needs to be converted to a batch :)

To start, I suggest to split this into three update functions.

1. Add new fields
2. Convert data
3. Remove stuff

That's more or less the standard and will make it much easier once you start implementing the conversion as a batch.

+++ b/core/modules/comment/comment.moduleundefined
@@ -62,19 +66,37 @@
  */
-const COMMENT_NODE_HIDDEN = 0;
+const COMMENT_ENTITY_HIDDEN = 0;

I'm not sure if we need also these "entity" in the names.... e.g. just comment_statistics as table name, and maybe COMMENT_STATUS_* or something for these constants.

+++ b/core/modules/comment/comment.moduleundefined
@@ -282,80 +411,73 @@ function comment_menu() {
+function comment_entity_reply_access(Drupal\Core\Entity\EntityInterface $entity) {

Same here, comment_reply_access() sounds much better to me.

+++ b/core/modules/comment/comment.moduleundefined
@@ -499,17 +623,25 @@ function comment_permalink($cid) {
+ * @todo entity access for other entity types?
  */
function comment_get_recent($number = 10) {
   $query = db_select('comment', 'c');
-  $query->innerJoin('node', 'n', 'n.nid = c.nid');
-  $query->addTag('node_access');
+  $query->addTag('comment_entity_access');

As said above. This also uses a different tag than the one above.

+++ b/core/modules/comment/comment.moduleundefined
@@ -499,17 +623,25 @@ function comment_permalink($cid) {
+    $query->condition(db_or()
+      ->condition('n.status', NODE_PUBLISHED)
+      ->condition('n.status', NULL, 'IS NULL')

You can also use isNull().

+++ b/core/modules/comment/comment.moduleundefined
@@ -2018,8 +1964,16 @@ function comment_rdf_mapping() {
+      $function = $entity->entity_type . '_access';
+      if (function_exists($function)) {
+        $entity = entity_load($entity->entity_type, $entity->entity_id);
+        return $function('view', $entity);

Ah, here is the @todo for that...

+++ b/core/modules/comment/comment.moduleundefined
@@ -2044,3 +1998,177 @@ function comment_library_info() {
+function comment_get_comment_fields($entity_type = NULL) {
+  return array_filter(field_info_fields(), function ($value) use($entity_type) {
+    if ($value['type']  == 'comment') {
+      if (isset($entity_type)) {
+        return in_array($entity_type, array_keys($value['bundles']));
+      }
+      return TRUE;
+    }

Can you attach the same field to different entity types? does that still work? :)

Note that this feature might be removed if we move field CRUD into the entity controllers.

Thanks berdir, yes this is ready for this type of review.
Will reroll on Monday with your changes and some pointed out by chx in irc.

Some responses to questions above

Careful with making a huge patch even bigger with unecessary doc fixes.

I asked Lars Toomre to postpone #1808478: Add missing type hinting to Comment module docblocks so I didn't have to re-roll this - he asked me to add it in while I was at it, seemed reasonable :)

I assume this is just a temporary measure until with have generic entity access support, which is close to being commited?

Yep, will add a @todo

I'm not sure that's something we can do on hook_enable(), this operation could take forever. Imagine executing this on drupal.org.

Maybe needs a separate backend task that executes this as a batch?

This is very similar to what is already there, but will rewrite to be a queued task.

Needs to be converted to a batch :)

No worries, I had that in earlier patches that were using node_save (which I realise is a no-no in update hooks)

Can you attach the same field to different entity types? does that still work? :)

I believe so

Very nice work. I really like the flexibility behind this, even if it will create a lot of new tables.
field_update_instance still store values in the database, could move it to CMI.

@julien - see #1735118: Convert Field API to CMI - CMI will manage the field - win!

StatusFileSize
new335.87 KB
FAILED: [[SimpleTest]]: [MySQL] 47,998 pass(es), 6 fail(s), and 4 exception(s).
[ View ]
new58.28 KB

Reroll for changes from @berdir and @chx on irc

+++ b/core/modules/comment/comment.admin.inc
@@ -127,7 +127,11 @@ function comment_admin_overview($form, &$form_state, $arg) {
+  if (module_exists('node')) {
+    // Special case to ensure node access works.
+    $query->leftJoin('node', 'n', "n.nid = c.entity_id AND c.entity_type = 'node'");
+    $query->addTag('node_access');
+  }

Do we want to add a @todo here to replace the code with generic entity access code once that gets in? (There are other comments about this elsewhere in the code)

+++ b/core/modules/comment/comment.install
@@ -492,6 +492,14 @@ function comment_update_8004(&$sandbox) {
+ * Add new field_api fields.

Docblocks should start with a third person verb, in this case "Adds new...". This goes for more (update) functions in the patch.

I'm not sure we have a generic entity access solution for queries, we do for single entities (nearly)

StatusFileSize
new337.87 KB
PASSED: [[SimpleTest]]: [MySQL] 48,059 pass(es).
[ View ]
new3.11 KB

Rerolled to fix failing tests from comment translation ui, included Xano's changes to update hook comments.

@larowlan: I applied the patch. Drupal detected that I would have to perform some database updates (provided by this patch). But, unfortunately, when I applied these updates the Database update utility showed this error:
Additional uncaught exception thrown while handling exception.
Original
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.access" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /Applications/MAMP/htdocs/repo/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).
Additional
Symfony\Component\DependencyInjection\Exception\InvalidArgumentException: The service definition "plugin.manager.views.access" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of /Applications/MAMP/htdocs/repo/core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php).

Also, in the status report I get these notices:
- Notice : Undefined index: backtrace in _drupal_log_error() (line 188 of core/includes/errors.inc ).
- Symfony\Component\DependencyInjection\Exception\InvalidArgumentException : The service definition "plugin.manager.views.access" does not exist. in Symfony\Component\DependencyInjection\ContainerBuilder->getDefinition() (line 690 of core/vendor/symfony/dependency-injection/Symfony/Component/DependencyInjection/ContainerBuilder.php ).

Leaving status to "needs review" so that others can review the patch.

Hi @falcon03 - this will need a re-install - there are changes made in the install profile that cannot survive a HEAD to HEAD update.

And it's green again - we're running out of time before freeze so we need as many reviews as possible now so we have time to re-roll.
thanks

Status:Needs review» Reviewed & tested by the community
StatusFileSize
new22.77 KB

I'm download the fresh Drupal 8 from git, apply the patch and install Drupal. Create comment for user, content, taxonomy term and works well.
Big thanks, nice works! :)

Re + // @todo - how to remove foreign keys? Drupal doesnt actually add the FK to the database, it just stores them in hook_schema so the answer is: you don't need to do anything. Someone could reroll the patch w/o that line but also we can leave that there for later removal in a trivial novice patch.

Status:Reviewed & tested by the community» Needs review

I think this needs more technical reviews before this is RTBC, this is too big for a single test to be enough :)

Will try to review this asap.

StatusFileSize
new33.65 KB

Small curiosity: comment in comment! :)

Issue tags:+accessibility

@larowlan: ok, I finally managed to apply the patch. Now the content type issue doesn't exist anymore, since these settings have been moved in a field. However, I would like to suggest a little improvement: comment field shouldn't be "hidden" for the "article" content type (but it is).

Another issue I met: going to node/add/article and scrolling to vertical tabs, I cannot hear comments' settings with the "comment tab" title....

+++ b/core/modules/comment/comment.admin.incundefined
@@ -127,7 +127,11 @@ function comment_admin_overview($form, &$form_state, $arg) {
+  if (module_exists('node')) {
+    // Special case to ensure node access works.
+    $query->leftJoin('node', 'n', "n.nid = c.entity_id AND c.entity_type = 'node'");
+    $query->addTag('node_access');

Still a but worried about this, especially if we introduce a generic access implementation, because that will make it more complex, not easier (supporting multiple entity types based on a column has never really been thought about I think and my head explodes just thinking about it, so I don't).

I fear that this could be a real blocker as we won't be committing something with known security implications. Some ideas on how to improve the situation:

- As a possible initial workaround: Only display a non-linked comment subject, completely hide/replace the parent entity label/link with "No access" or so? We will need to check with the security team if that's ok. Will try to get someone in here.

- In case we actually manage to introduce a real generic entity access API for queries/lists, maybe instead of trying to figure out access to the parent in this query directly, we could try to inherit permissions/grants of the parent entity and duplicate them for every attached comment. But that's going to cause problems with nodes with multiple hundred comments :(

I guess the whole thing could also cause some headaches to the views people who plan to replace this with a view, maybe @timplunkett or @dereine could comment on that part.

+++ b/core/modules/comment/comment.api.phpundefined
@@ -185,14 +185,19 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
-function hook_comment_entity_access(Drupal\entity\Entity $entity) {
+function hook_comment_access(Drupal\entity\Entity $entity) {

No idea why this works (is it actually called in the tests anywhere?).

It's Drupal\Core\Entity\Entity now, and this should actually type hint the interface.

+++ b/core/modules/comment/comment.pages.incundefined
@@ -150,3 +150,23 @@ function comment_approve($cid) {
+/**
+ * Page callback: Legacy handler to redirect old comment reply urls to new url.

We recently created (and then moved to contrib) a legacy url handler for the ?q=path vs. index.php/path problem. Wondering if that one could handle this as well? Or why is this needed?

+++ b/core/modules/comment/comment.pages.incundefined
@@ -150,3 +150,23 @@ function comment_approve($cid) {
+ * @param Drupal\node\Plugin\Core\Entity\Node $node
+ *   The node to which the comments are attached.

Should be @param \Drupal\... . Probably wrong in other places too.

Created http://groups.drupal.org/node/267143 to discuss that.

As a possible initial workaround: Only display a non-linked comment subject, completely hide/replace the parent entity label/link with "No access" or so?

Yes had thought of that too, I think it's a way forward

No idea why this works (is it actually called in the tests anywhere?).

Yes this api doc was pre the changes to entity (into core from a module) (that's how long I've been working on it) - but it's just the .api.php doco so that's why it works

We recently created (and then moved to contrib) a legacy url handler for the ?q=path vs. index.php/path problem. Wondering if that one could handle this as well? Or why is this needed?

@chx asked for this so links in emails etc would keep working

Should be @param \Drupal\... . Probably wrong in other places too.

ie needs the leading \? should be able to fix those fairly easily with sed

@falcon03 let's work on the tab issue in #1829338: comment tab settings aren't announced in comment tab title, I'll update the default value for the article content type in the standard profile in the next re-roll.

Issue tags:+JavaScript

chasing HEAD
removes @todo as per @chx's request
resets default behaviour of article comments in standard profile as per @falcon03's request
fixes issues with PSR-0 namespaces pointed out by @berdir

no interdiff sorry, accidentally polluted the 8.x merge in my branch with some of the fixes

StatusFileSize
new337.58 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.164.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

hmm lost attachment

StatusFileSize
new337.62 KB
PASSED: [[SimpleTest]]: [MySQL] 48,358 pass(es).
[ View ]

chasing HEAD

+++ b/core/modules/comment/comment.admin.incundefined
@@ -9,6 +9,51 @@
+        if (module_exists('field_ui') && $path = _field_ui_bundle_admin_path($entity_type, $bundle)) {

You are using a private function from field ui module there. I already opened an issue for that a while ago.

#1824244: Tests for: Turn _field_ui_bundle_admin_path() into a public function (field_ui_bundle_admin_path())

I applied the patch and created comments for taxonomy, user and node, here are my notes

  • The required option it's confusing, it doesn't force me to submit comments when I create the entity
  • submitting comments on terms gives me a page denied error
  • I cannot delete a comment field from the edit field form i get this error The requested page "/admin/structure/types/manage/user/fields/field_comentarios/delete" could not be found.
  • from the field list I can delete the field but I get this error Drupal\Core\Entity\Query\QueryException: not found in Drupal\field_sql_storage\Entity\Tables->ensureEntityTable() (line 100 of /var/www/drupal8/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.php).
  • The number of values is not limiting the number of comments. If this limits the number of comments, should be set to a sane default of unlimited
  • there is a "hidden" state along with "open" and "closed" when you create the field but then in the entity there is not, even when you have some comments submitted
  • The last four points above were tested creating comments on users

Does anyone know if this needs to be in before the feature freeze or is it exempt because its marked as a task. I'm offline for two weeks from this Friday.
@rodrigoaguilera I'll go over your list of issues shortly.

StatusFileSize
new337.98 KB
FAILED: [[SimpleTest]]: [MySQL] 48,474 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new2.97 KB

The required option it's confusing, it doesn't force me to submit comments when I create the entity

Have extended comment_form_field_ui_field_edit_form to hide the required checkbox (and force cardinality to one).

submitting comments on terms gives me a page denied error

This is more inconsistency in core, taxonomy_term_access doesn't support a 'view' op, have added more juggling code until #1696660: Add an entity access API for single entity access lands

I cannot delete a comment field from the edit field form i get this error The requested page "/admin/structure/types/manage/user/fields/field_comentarios/delete" could not be found.
from the field list I can delete the field but I get this error Drupal\Core\Entity\Query\QueryException: not found in Drupal\field_sql_storage\Entity\Tables->ensureEntityTable() (line 100 of /var/www/drupal8/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.php).

I'll open a new ticket for this, once I confirm it's unrelated

The number of values is not limiting the number of comments. If this limits the number of comments, should be set to a sane default of unlimited

This limits the number shown on a page, not the number entered.

there is a "hidden" state along with "open" and "closed" when you create the field but then in the entity there is not, even when you have some comments submitted

fixed, was handling comment count incorrectly

just to confirm that

I cannot delete a comment field from the edit field form i get this error The requested page "/admin/structure/types/manage/user/fields/field_comentarios/delete" could not be found.
from the field list I can delete the field but I get this error Drupal\Core\Entity\Query\QueryException: not found in Drupal\field_sql_storage\Entity\Tables->ensureEntityTable() (line 100 of /var/www/drupal8/core/modules/field/modules/field_sql_storage/lib/Drupal/field_sql_storage/Entity/Tables.php

is a pre-existing bug in HEAD
will file new issue with tests

StatusFileSize
new337.91 KB
PASSED: [[SimpleTest]]: [MySQL] 48,449 pass(es).
[ View ]
new1.25 KB

Whoops, didn't mean to alter cardinality on all fields, just comment ones.

Status:Needs review» Needs work

Attached are some comments from starting to review this patch. The most important is the use of x_y as class members instead of xY. I did not want to go much further until this was addressed.

+++ b/core/modules/comment/comment.admin.incundefined
@@ -9,6 +9,51 @@
/**
+ * Page callback for displaying the comment bundle admin overview page.

Should be something like 'Pagecallback: Displays...'. This docblock is missing a @return directive.

+++ b/core/modules/comment/comment.admin.incundefined
@@ -82,23 +127,28 @@ function comment_admin_overview($form, &$form_state, $arg) {
-  $cids = array();
+  $cids = $entity_ids = $entities = array();
-  // We collect a sorted list of node_titles during the query to attach to the

My understanding is that this type of multiple initialization in a line is not a Drupal standard. Apparently, each variable should initialized on a separate line.

+++ b/core/modules/comment/comment.admin.incundefined
@@ -107,9 +157,9 @@ function comment_admin_overview($form, &$form_state, $arg) {
+    $comment->entity_title = $entities[$comment->entity_type][$comment->entity_id]->label();

Small optimization... I would suggest using temporary variable to minimize function calls into $comment since $entities[][] is repeated.

Also throughout this patch, there is an issue of how object properties are used. My understanding is that, for instance, entity_instance should be entityInstance (camel case).

+++ b/core/modules/comment/comment.api.phpundefined
@@ -170,5 +174,36 @@ function hook_comment_delete(Drupal\comment\Comment $comment) {
/**
+ * Controls access to commenting on an entity where no {%entity_type}_access
+ * function exists for the given entity.

Needs to be a one-line summary less than 80 characters.

+++ b/core/modules/comment/comment.field.incundefined
@@ -0,0 +1,291 @@
+<?php
+/**
+ * @file
+ * Enables comments with moderation on entities

Should be a blank line after '<?php'.

Status:Needs work» Needs review

Thanks Lars, will include those in next re-roll - some questions though:

My understanding is that this type of multiple initialization in a line is not a Drupal standard. Apparently, each variable should initialized on a separate line.

Ok, will try and find a reference to that in the standards.

Small optimization... I would suggest using temporary variable to minimize function calls into $comment since $entities[][] is repeated.

Also throughout this patch, there is an issue of how object properties are used. My understanding is that, for instance, entity_instance should be entityInstance (camel case).

I don't get what you mean here. All the other comment properties are underscored, to correspond with their db column names, What function calls into $comment are you referring to?

Changing status back to encourage as many reviews as possible, particularly of architecture

Sorry for changing the status @lorowlan. I often forget that dreditor changes the status. Sorry about that.

Re: multiple assignments in one line, see #1798732-75: Convert install_task, install_time and install_current_batch to use the state system and following comments. Also has come in other recent issues.

Re: object member names. My understanding from [#608152] is that all member names like $comment->entity_title should be changed to use camelCase (ie $comment->entityTitle).

Thanks, will address on next re-roll.

Re: object member names. My understanding from [#608152] is that all member names like $comment->entity_title should be changed to use camelCase (ie $comment->entityTitle).

Not if they map to a column in the table of the entity, as they do in this case, then entity_type is correct.

Not if they map to a column in the table of the entity, as they do in this case, then entity_type is correct.

Where is that explained in the coding standards? As object properties are accessed much more often, I'd rather have consistent property names than DB column names.

StatusFileSize
new337.97 KB
FAILED: [[SimpleTest]]: [MySQL] 48,469 pass(es), 20 fail(s), and 0 exception(s).
[ View ]
new2.25 KB

Re-roll to include Lars Toomre's comments, left it as ->entity_title for consistency.

Chasing HEAD where history.module has been committed, win!

StatusFileSize
new701 bytes
new310.96 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

StatusFileSize
new701 bytes
new336.34 KB
PASSED: [[SimpleTest]]: [MySQL] 48,517 pass(es).
[ View ]

I forgot to add the new files.

Status:Needs review» Needs work

+++ b/core/modules/comment/comment.admin.inc
@@ -82,23 +134,30 @@ function comment_admin_overview($form, &$form_state, $arg) {
+  if (module_exists('node')) {
+    // Special case to ensure node access works.
+    $query->leftJoin('node', 'n', "n.nid = c.entity_id AND c.entity_type = 'node'");
+    $query->addTag('node_access');
+  }

Can we implement this using a hook? Maybe hook_query_TAG_alter.

+++ b/core/modules/comment/comment.field.inc
@@ -0,0 +1,292 @@
+          // Unpublished comments are not included in $node->comment_count, so show

$node->comment_count?

+++ b/core/modules/comment/comment.module
@@ -499,17 +642,24 @@ function comment_permalink($cid) {
+  if (module_exists('node')) {
+    // Special case to filter by published content.
+    $query->leftJoin('node', 'n', "n.nid = c.entity_id AND c.entity_type = 'node'");
+    $query->addTag('node_access');
+    $query->condition(db_or()
+      ->condition('n.status', NODE_PUBLISHED)
+      ->isNull('n.status')
+    );
+  }
+  $comments = $query->orderBy('c.created', 'DESC')

Same here. We are decoupling comment module from node, but there are a lot of 'special' cases for nodes. Could we move those special cases into node module?

+++ b/core/modules/comment/comment.tokens.inc
@@ -193,17 +200,33 @@ function comment_tokens($type, $tokens, array $data = array(), array $options =
+        case 'node':
+          if ($comment->entity_type== 'node') {
+            $entity = entity_load($comment->entity_type, $comment->entity_id);
+            $title = $entity->label();
+            $replacements[$original] = $sanitize ? filter_xss($title) : $title;
+          }
+          else {
+            $replacements[$original] = NULL;
+          }

Could you explain why this is required?

+++ b/core/modules/comment/lib/Drupal/comment/Tests/CommentFieldsTest.php
@@ -51,56 +50,14 @@ function testCommentDefaultFields() {
-  /**
-   * Tests that comment module works when enabled after a content module.
-   */
-  function testCommentEnable() {

Why this test is removed?

+++ b/core/modules/system/lib/Drupal/system/Tests/Module/DependencyTest.php
@@ -170,28 +170,39 @@ function testUninstallDependents() {
-    // Disable forum and comment. Both should now be installed but disabled.
+    // Disable forum, should now be installed but disabled.
     $edit = array('modules[Core][forum][enable]' => FALSE);
     $this->drupalPost('admin/modules', $edit, t('Save configuration'));
     $this->assertModules(array('forum'), FALSE);
-    $edit = array('modules[Core][comment][enable]' => FALSE);
-    $this->drupalPost('admin/modules', $edit, t('Save configuration'));
-    $this->assertModules(array('comment'), FALSE);
-    // Check that the taxonomy module cannot be uninstalled.
-    $this->drupalGet('admin/modules/uninstall');
-    $checkbox = $this->xpath('//input[@type="checkbox" and @disabled="disabled" and @name="uninstall[comment]"]');
-    $this->assert(count($checkbox) == 1, 'Checkbox for uninstalling the comment module is disabled.');

I think this is similar to issues with convert url paths into fields.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/LanguageUpgradePathTest.php
@@ -47,6 +47,7 @@ public function testLanguageUpgrade() {
+    $node = node_load(50);

Seems not necessary.

This patch is huge, and I cannot say 'fix this and this, and it is ready to go', however, all pre-existent test are working and as far I can see the changes makes sense.

I will play a bit more with this patch in the following days to ensure all works as expected, specially with the upgrade path and provide more feedback.

BTW, amazing work!

Here are some observations from reading through first portion of this patch.

+++ b/core/modules/comment/comment.installundefined
@@ -36,50 +36,52 @@ function comment_uninstall() {
+  // Set default value of comment_maintain_entity_statistics.
+  state()->set('comment_maintain_entity_statistics', TRUE);

Curious why this is a state variable. Seems like it should be config. If it remains in state, at minimimum it should be namespaced like 'comment.maintain_entity_statistics'.

+++ b/core/modules/comment/comment.installundefined
@@ -188,9 +204,14 @@ function comment_schema() {
+      'comment_num_new' => array('entity_id', 'entity_type', 'field_name', 'status', 'created', 'cid', 'thread'),
       'comment_uid' => array('uid'),

I would format this like 'comment_entity_langcode' just below.

+++ b/core/modules/comment/comment.moduleundefined
@@ -62,19 +66,37 @@
+define('COMMENT_NEW_LIMIT', REQUEST_TIME - 30 * 24 * 60 * 60);

Shouldn't this value be set by a Config file?

+++ b/core/modules/comment/comment.moduleundefined
@@ -98,48 +120,150 @@ function comment_help($path, $arg) {
+ * Menu callback for loading the actual name of a comment field.
  */
-function comment_node_type_load($name) {

I think this function needs @param and @return directives.

+++ b/core/modules/comment/comment.moduleundefined
@@ -269,93 +397,106 @@ function comment_menu() {
+ * @return Drupal\entity\Entity or FALSE if not found.

This needs to be split into type hint and then description line.

+++ b/core/modules/comment/comment.moduleundefined
@@ -269,93 +397,106 @@ function comment_menu() {
+ * Returns a menu title which includes the number of unapproved comments.

This needs a @return directive.

+++ b/core/modules/comment/comment.moduleundefined
@@ -527,15 +677,18 @@ function comment_get_recent($number = 10) {
  *   Number of comments.
  * @param $new_replies
  *   Number of new replies.
- * @param Drupal\node\Node $node
- *   The first new comment node.
+ * @param \Drupal\Core\Entity\EntityInterface $entity
+ *   The first new comment entity.
+ * @param string $field_name
+ *   The field name
  *
  * @return
  *   "page=X" if the page number is greater than zero; empty string otherwise.

Can you add type hints to rest of this docblock? Odd to see them only on the the changed @params.

+++ b/core/modules/comment/comment.moduleundefined
@@ -609,163 +769,8 @@ function theme_comment_block() {
+ * Returns a rendered form to comment the given entity.

To 'comment on' the given enitity?

+++ b/core/modules/comment/comment.moduleundefined
@@ -609,163 +769,8 @@ function theme_comment_block() {
- *
- * @param Drupal\node\Node $node
- *   The node entity to be commented.
  * @param int $pid
  *   (optional) Some comments are replies to other comments. In those cases,
  *   $pid is the parent comment's comment ID. Defaults to NULL.

Looks like @param directives for $entity and $field_name are missing.

+++ b/core/modules/comment/comment.moduleundefined
@@ -782,8 +787,10 @@ function comment_add(Node $node, $pid = NULL) {
  * @param $mode
  *   The comment display mode; COMMENT_MODE_FLAT or COMMENT_MODE_THREADED.

Again if adding or changing a @param directive, please add type hints to whole docblock.

+++ b/core/modules/comment/comment.moduleundefined
@@ -2083,3 +2065,176 @@ function comment_library_info() {
+ * @param string $entity_type
+ *   (optional) Specify a entity type if you want to just return fields which
+ *   are attached on a certain entity type.

Should state what the default value is.

+++ b/core/modules/comment/comment.moduleundefined
@@ -2083,3 +2065,176 @@ function comment_library_info() {
+/**
+ * Decide on the type of marker to be shown for a comment.
+ */
+function comment_mark(Comment $comment) {

Missing @param and @return directives.

+++ b/core/modules/comment/comment.pages.incundefined
@@ -14,40 +15,49 @@
  * @param $pid
  *   (optional) Some comments are replies to other comments. In those cases,
  *   $pid is the parent comment's comment ID. Defaults to NULL.
  *
  * @return array
  *   An associative array containing:
- *   - An array for rendering the node or parent comment.
- *     - comment_node: If the comment is a reply to the node.
+ *   - An array for rendering the entity or parent comment.
+ *     - comment_entity: If the comment is a reply to the entity.
  *     - comment_parent: If the comment is a reply to another comment.
  *   - comment_form: The comment form as a renderable array.
  */
-function comment_reply(Node $node, $pid = NULL) {
+function comment_reply(Drupal\Core\Entity\EntityInterface $entity, $field_name, $pid = NULL) {

Please add type hint to $pid. Must this be an integer or can it be a string like uuid?

+++ b/core/modules/comment/comment.pages.incundefined
@@ -124,7 +145,31 @@ function comment_approve($cid) {
+  }
+  throw new NotFoundHttpException();

Look like missing @throws directive in docblock for this function.

Assigned:larowlan» Unassigned
Status:Needs work» Needs review
StatusFileSize
new338.66 KB
FAILED: [[SimpleTest]]: [MySQL] 48,638 pass(es), 21 fail(s), and 88 exception(s).
[ View ]
new11.74 KB

$node->comment_count?

Fixed

Can we implement this using a hook? Maybe hook_query_TAG_alter.

Can these go into follow-ups?

Could you explain why this is required?

As a pseudo upgrade path for sites with existing tokens of that format

Why this test is removed?

Comment module currently has a lot of logic around node type creation to handle bundle and field creation, we don't need it anymore - we react on the field creation instead

Seems not necessary.

Correct, carry over from test debugging.

Curious why this is a state variable. Seems like it should be config. If it remains in state, at minimimum it should be namespaced like 'comment.maintain_entity_statistics'.

This isn't something a user can disable, only for modules like migrate that don't want the cost of the inserts - disabling it will render comment functionality fairly useless (most of the comment module code before and after this patch relies on a comment_count before much is done) so this is a programmer only thing - hence state

I would format this like 'comment_entity_langcode' just below.

fixed

Shouldn't this value be set by a Config file?

added a @todo to remove once #1029708: History table for any entity lands

This needs to be split into type hint and then description line

fixed

This needs a @return directive

fixed

Can you add type hints to rest of this docblock? Odd to see them only on the the changed @params.

fixed

To 'comment on' the given enitity?

fixed

Looks like @param directives for $entity and $field_name are missing.

fixed

Again if adding or changing a @param directive, please add type hints to whole docblock.

fixed

Should state what the default value is.

fixed

Missing @param and @return directives.

fixed

Please add type hint to $pid. Must this be an integer or can it be a string like uuid?

fixed

Look like missing @throws directive in docblock for this function.

fixed

Tried to re-roll with the new entity access api, but fubhy advises it only works for test entities at the moment, not node, user etc so kept the @todo's in.

Handing over to fubhy to see this over the line.

Status:Needs review» Needs work

Note: Drupal\system\Tests\Transliteration\TransliterationTest 2 fails is unrelated.

Assigned:Unassigned» dagmar

I'm going to provide a patch to fix the failing tests.

@dagmar, all references to COMMENT_ENTITY_HIDDEN need to become COMMENT_HIDDEN thx

Assigned:dagmar» Unassigned
Status:Needs work» Needs review
StatusFileSize
new12.19 KB
new337.27 KB
Test request sent.
[ View ]

Rerolled. Thanks @larolwan. I get locally only one fail related to the TranslationUI.

@dagmar you should be able to ping @dawehner for help with the views fails, the translation fail must be new tests in that big group of commits from Wed, they were passing above.

Status:Needs review» Needs work

Testbot reported for #197:

FAILED: [[SimpleTest]]: [MySQL] 48,688 pass(es), 2 fail(s), and 2 exception(s).

=> needs work

Dagmar, for sure is one of the few people which don't need help with views,
but hey @dagmar if you need help ... ping.

Status:Needs work» Needs review
StatusFileSize
new338.65 KB
FAILED: [[SimpleTest]]: [MySQL] 48,768 pass(es), 2 fail(s), and 2 exception(s).
[ View ]

I Just renamed the file

/core/modules/comment/lib/Drupal/comment/Plugin/views/field/NcsLastUpdated.php

into

/core/modules/comment/lib/Drupal/comment/Plugin/views/field/StatisticsLastUpdated.php

StatusFileSize
new347.92 KB
FAILED: [[SimpleTest]]: [MySQL] 48,968 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Views issues are caused by bad named classes.

Renamed all the Ncs* classes into StatistcsLastComment*

@dagmar
With the config from http://drupal.org/node/1542048 you can force git to show renames as renames in the diff,
so it's way easier to review the patch. If you look at previous versions of the patch like http://drupal.org/files/decouple-comment-node-731724.112.patch you will see that the renaming already happened. Could it be that you failed in applying the patch corrently so the renames got lost?

StatusFileSize
new337.45 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.207.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

I think this should fix issues from #204.

@andypost yes, #1848904: Bundles cannot be specified in Entity Translation tests and some changes in Drupal\comment\Tests\CommentTranslationUITest::createEntity fixed the translation issue.

Rerolled after
#1848904: Bundles cannot be specified in Entity Translation tests
#1168246: Freedom For Fieldsets! Long Live The DETAILS.

StatusFileSize
new347.83 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 731724-212.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll.

StatusFileSize
new346.34 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/user/lib/Drupal/user/Tests/UserCancelTest.php.
[ View ]

StatusFileSize
new346.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 731724-216.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

StatusFileSize
new337.2 KB
FAILED: [[SimpleTest]]: [MySQL] 49,080 pass(es), 5 fail(s), and 4 exception(s).
[ View ]

Rerolling patch, applies well but I'm confused because its size is lower than #216.

Status:Needs review» Needs work
StatusFileSize
new337.2 KB
FAILED: [[SimpleTest]]: [MySQL] 49,095 pass(es), 5 fail(s), and 4 exception(s).
[ View ]
new763 bytes

Because of [#1848180], I changed the call to field_ui_bundle_admin_path (needed for manually testing this).

I only have been able of testing the first test failing, and it seems to be working ok, so maybe the problem is in the test itself creating the translatable comment.

Assigned:Unassigned» larowlan
Status:Needs work» Needs review
StatusFileSize
new332.84 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]
new7.54 KB

Patch from #222 with fixes for failing tests and re-roll against HEAD.
Also includes renames from #216 missed in #222.
Lets see what bot says.

StatusFileSize
new340.21 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.228.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new12.25 KB

Hmm, git identified the CommentUserTest as a rename of CommentTestBase, attached fixes it. Probably why the patch size dropped.

Green, I like this! :) But I no longer dare to RTBC… :S

I don't understand why you don't dare, but certainly I hope you'd be confident to do local testing and let us know what you find of your results.

Marking something RTBC is a big thing. Good to make sure you do it right, but if you've done due diligence it's usually not a problem. Someone can just set it back and let you know what tests you missed for next time.

Just so you know, I've an agreement with @fago that #1778178: Convert comments to the new Entity Field API will go in before this.

possibly worth looking at for inclusion http://drupal.org/project/better_comments_admin

StatusFileSize
new340.13 KB
PASSED: [[SimpleTest]]: [MySQL] 49,685 pass(es).
[ View ]

Re-rolled

Test #1:
- Download Drupal 8 from git
- Apply patch and install
- Create some node types and add comment
- Add comment the user, taxonomy term

Works well! :)

Test #2:
- Download Drupal 8 from git
- install Drupal and apply patch
- run update.php
- Database error:

comment module
Update #8005

Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[HY000]: General error: 1 table comment_0 has no column named nid: CREATE INDEX main.comment_0_comment_num_new ON comment_0 (nid, status, created, cid, thread); ; Array ( ) in Drupal\Core\Database\Connection->query() (line 554 of /var/www/core/lib/Drupal/Core/Database/Connection.php).

In both cases, the type of database SQLite.

--- a/core/modules/comment/lib/Drupal/comment/Plugin/views/row/Rss.php
+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/row/Rss.php
+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/row/Rss.php
+++ b/core/modules/comment/lib/Drupal/comment/Plugin/views/row/Rss.php
@@ -151,7 +151,7 @@ function render($row) {
@@ -151,7 +151,7 @@ function render($row) {
       $item_text .= drupal_render($build);
     }
-    $item = new stdClass();
+    $item = new \stdClass();
     $item->description = $item_text;
     $item->title = $comment->label();
     $item->link = $comment->link;

This change already committed in http://drupal.org/files/drupal-1821612-10.patch

StatusFileSize
new33.15 KB
new30.8 KB

Have done a thorough test of this, only one gripe. The label of an added comment bundle should not appear by default, in fact I see no reason for any option here, maybe I am wrong. See below picture.

label shown when not needed.

Also, if when configuring content type, you add a custom comment field, then when configuring the field, you get to 'widget type', the button says 'continue'. If you press it you get the following error.

    Notice: Undefined index: widget_type in field_ui_widget_type_form_submit() (line 752 of core\modules\field_ui\field_ui.admin.inc).
    Notice: Undefined index: module in field_ui_widget_type_form_submit() (line 753 of core\modules\field_ui\field_ui.admin.inc).
    Notice: Undefined index: widget_type in field_ui_widget_type_form_submit() (line 755 of core\modules\field_ui\field_ui.admin.inc).
    Notice: Undefined index: module in _field_write_instance() (line 615 of core\modules\field\field.crud.inc).
    Notice: Undefined index: in Drupal\field_ui\FieldOverview->form() (line 143 of core\modules\field_ui\lib\Drupal\field_ui\FieldOverview.php).

And the widget is now gone from field configuration.

Also see this picture.

Content type comment bundle error.

StatusFileSize
new7.76 KB
new7.5 KB
new9.52 KB

Also, when editing comment bundles the breadcrumb and title of the overlay should be updated, see content types. Pictures below.

Content type edit screen.Content type fileds edit screen.

I know what I am editing above. However :

Comment bundle edit field screen.

What am I editing here, I forgot.

#240 sounds like a great follow-up feature request.

StatusFileSize
new17.1 KB
new44.7 KB

OK, the 'reply' functionality seems broken. I will try to explain, but not easy.

First off, with reference to the following image.

Only local images are allowed.
(Having a problem with this image, check link below)

1) The 'new' tag should be capitalized and probably styled. Red I would suggest.

2) The links at the bottom of each comment, paths are a bit screwed, and when visiting the 'add new comment' I get nothing but the front page with no styling (see image below). And visiting the reply link I get the following error message.

Warning: call_user_func_array() expects parameter 2 to be array, string given in Drupal\Core\EventSubscriber\LegacyControllerSubscriber->Drupal\Core\EventSubscriber\{closure}() (line 52 of core\lib\Drupal\Core\EventSubscriber\LegacyControllerSubscriber.php).

New Comment.

The actual links are as follows:

a) Reply link => "localhost/drupal8/comment/reply/node/4/field_comment_label_test/8"

b) New comment link => "localhost/drupal8/comment/reply/comment/8/field_t3#comment-form"

I hope that is of some help. Sorry for the large pictures.

@mbrett5062, can you let me know what comment fields you have here?
Sounds like you have field_label_test on node type and field_t3 on comment bundle. Which comment bundle?

Lee

StatusFileSize
new1.37 KB
new340.24 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Re-rolled against latest HEAD, updated summary with link to the sandbox and testing instructions + follow up as per #240

Issue summary:View changes

Updated issue summary with api changes

StatusFileSize
new340.13 KB
FAILED: [[SimpleTest]]: [MySQL] 49,972 pass(es), 314 fail(s), and 359 exception(s).
[ View ]
new588 bytes

views_fetch_data now gone the way of the DIC
We were force reloading the views cache in hook_field_create_instance, but views module should do this for us in views_field_create_instance().
If not, must be a timing thing, possibly need a hook_module_implements_alter() to get our comment_field_create_instance ahead of views.

StatusFileSize
new340.62 KB
FAILED: [[SimpleTest]]: [MySQL] 50,144 pass(es), 94 fail(s), and 11 exception(s).
[ View ]
new1.04 KB

Fixes for new entity display changes

StatusFileSize
new340.63 KB
FAILED: [[SimpleTest]]: [MySQL] 50,220 pass(es), 24 fail(s), and 3 exception(s).
[ View ]
new593 bytes

Wrong bundle in display settings.

StatusFileSize
new346.66 KB
PASSED: [[SimpleTest]]: [MySQL] 50,416 pass(es).
[ View ]
new7.34 KB

Should fix remaining fails from entity translation and views changes.

Tryed fresh install via drush.
Added "comment settings" field on user entity. Got comment form on user but posting a comment got no reply from core same for node article. Trying to delete "comment settings" field from user has error:
Fatal error: Call to undefined function views_fetch_data() in /home/andypost/www/core8/core/modules/comment/comment.module on line 2280

Suppose comment settings should have different delault label for diesplay fields screen.

Will try to mek a deeper review later

Green again
@andypost will try and ping you on irc on wed morn my time, Tue night yours
Patch above fixes display settings (using new entity display object) which might be why comments not displayed.
Also fix for views_fetch_data is in patch above, unless I've missed one...

StatusFileSize
new346.6 KB
PASSED: [[SimpleTest]]: [MySQL] 50,450 pass(es).
[ View ]
new529 bytes

Missed one call to views_fetch_data.
Latest code is in sandbox too.

Tested this on latest sandbox in branch comment-fieldapi4.

* Installed D8 using standard profile

Testing Article:

  • Create a new article
  • Created a test authenticated user account
  • Logged in as authenticated user and added a comment ok
  • Added comment as admin user ok
  • Replied as authenticated user ok

Testing User Comments:

  • Added a comment field to users under admin/config/people/accounts/fields
  • Set comments to 'open' (default was closed)
  • Set permissions for authenticated users to view user profiles
  • Admin and authenticated user was able to add comments ok
  • Admin user was able to view comment thread ok
  • Authenticated user was not able to view comment on profile page - permissions issue?

The following errors are appearing in apache logs:

[Tue Jan 01 14:40:41 2013] [error] [client 127.0.0.1] PHP Fatal error:  Call to undefined method Drupal\\field\\FieldInstance::getFormatter() in /Users/kim/dev/comment_as_field/core/modules/edit/lib/Drupal/edit/MetadataGenerator.php on line 61

Not sure if it's related.

StatusFileSize
new346.6 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.259.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Re-roll after blocks as plugins went in.
Merged cleanly (woot!) but expecting fails.

#1845372: Deleting a field from a non-node entity bundle results in an Entity Field Query Exception indicates that this issue is supposed to fix that issue for comments, but I don't see my test from that issue in the patch here, unless the method was renamed. Should I create a separate followup for adding the test coverage for that?

@xjm, I didn't get an answer regarding including that test here - happy to do so. Will add on next re-roll.

StatusFileSize
new1.94 KB

Existing test attached for your convenience. :)

thanks @YesCT. I'm half way through the epic re-roll. screenshot here: http://imgur.com/h1fuX

progress update, have re-rolled after EntityNG but getting some local failures, latest code is in sandbox, will get to it over the weekend

Didn't get to this at the weekend, working on #1871772: Convert custom blocks to content entities instead

StatusFileSize
new355.25 KB
FAILED: [[SimpleTest]]: [MySQL] 50,929 pass(es), 43 fail(s), and 15 exception(s).
[ View ]
new170.81 KB

Latest patches.
Still failing a fair few comment tests, checking what else is failing.
Interdiff is a mess because of merge conflicts, did my best using git show

StatusFileSize
new353.91 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: failed during invocation of run-tests.sh.
[ View ]

Merged HEAD + fix some views plugins

StatusFileSize
new369.54 KB
FAILED: [[SimpleTest]]: [MySQL] 49,656 pass(es), 43 fail(s), and 15 exception(s).
[ View ]

Another try to merge tests

Status:Needs review» Needs work

Thanks @andypost.
I've got a sandbox for this, if you want commit access -please let me know.
I've got the tests in a slightly better shape in the sandbox (comment-fieldapi4 branch).

LR

Status:Needs work» Needs review
StatusFileSize
new358.74 KB
FAILED: [[SimpleTest]]: [MySQL] 49,795 pass(es), 8 fail(s), and 0 exception(s).
[ View ]
new23.85 KB

Fixed last tests
Added test for 3rd level of threading
Rework some code comments

PS: Patch done with -M25% to minify changes in views handlers
Some views handlers still needs work

StatusFileSize
new1.26 MB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.280_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new1.72 KB

This should fix some of the fails, the disable/enable of comment module can't be done with existing fields, so dropped that.

StatusFileSize
new359.6 KB
FAILED: [[SimpleTest]]: [MySQL] 49,743 pass(es), 3 fail(s), and 0 exception(s).
[ View ]

here's the -M25% one

Being late to the party here. Just checked out latest D8 and applied the patch in #282 cleanly. I have just begin testing things here. My focus will be primary on UX and usability, not to find bugs. I will of course report any problems I find.

To start with, I am gobsmacked about the amazing amount of work done here. Decoupling the comments is something very needed so a mountain of karma-points to everyone who has been working on this.

Since this is a rather humongous patch, it is a little difficult to know exactly what is in scope and not. I will try and limit to the differences I know of (comparing with the D7 version). Please point out when a new issue should be filed for anything.

structure_comment_bundles.png

"bundles" doesn't really say anything to me as a sitebuilder. I suggest we use "Comment forms" here, it would also make more sense sine "Contact form" is used below and selecting any of the two takes me to a similar configuration page.

structure_comment_bundles_list.png

Since one of the big reasons behind this patch is to decouple it from the nodes so it can be used on any entity type, this list can be long for complex sites. Simply changing "Article" to "Article (Content Type)" to indicate what entity type it is would make a big UX improvement.

It would also be preferred if the two first columns changed place so that USED IN comes first. In most cases that is what most users will look for first. Both those columns should actually be sortable too.

comment_form_field_comment.png

Default Text processing should be "Plain text". This because when a filtered text format is added, the sitebuilder rarely remembers that it will affect comments too. They are most likely needing it for a content text area of some sort. So, this is a security issue too.

I'm not sure what the Placeholder text is supposed to do. My immediate guess is that it is a text shown if an entity item doesn't have a comment yet.

After entering something I notice it is a ghosted text displayed in the text area field and disappears when the user starts typing in text. Label/description of this field will need to be improved. I'll get back to this unless anyone else comes up something better.

comment_entity_field_help_text.png

The help text isn't actually showing up on the comment edit form any more:

comment_settings_entity_item_help_text.png

Not really sure how this happened...

comment_entity_field_comments_shown.png

Have a feeling this probably is out of scope, but isn't it time there is an "Unlimited" option at the bottom?

OK, that was my quick UX/usability review of this amazing work. Not really anything big, but all these small things do add up for users and if it can be improved from start everyone wins.

Feel free to ping me on IRC if there is any other specific UX/usability things needed to be looked at.

StatusFileSize
new19.61 KB

Found one more thing:

comment_entity_field_settings.png

Should it really be possible to change this setting? Personally I can't come up with a use case where this is useful.

Can this be turned off and instead have a text saying something like "Only one instance of the comment filed can exist".

This should probably be a system generated text that is always used for field that only can exist in one instance on entities.

Seems I found a problem too:

comment_entity_field_default_visibility.png

No matter what I change the drop-down to it reverts back to "Hidden" the next time I open the field in edit mode.

Also, the "Hidden" option is missing on the individual entity item settings.

comment_settings_entity_hidden_missing.png

Personally I am not to fond of the term "Hidden". "Disabled" is probably a better choice. It should also have an optional text visible on the content such as "Comments have been disabled on this page". That text should also be possible to override on individual items.

Status:Needs work» Needs review
StatusFileSize
new359.85 KB
PASSED: [[SimpleTest]]: [MySQL] 50,111 pass(es).
[ View ]
new2.55 KB

Here's patch with fixed tests:
1) Partially reverted #280 to test that comment module could not be disabbled when there's active comment bundles
2) Fixed Translation tests - there was a simple error

EDIT
@tsvenson Thanx a lot for UI review!!!
I filed #1901110: Improve the UX for comment bundle pages and comment field settings in sandbox to address fixes

Status:Needs review» Needs work

@andypost Just happy to help out. Following the sandbox issue now to help keep me updated on the progress.

Issue summary:View changes

Added sandbox link and updated testing instructions

Status:Needs work» Needs review

Follow up issue:
Created #1903138: Move global comment permissions to field level as a follow up to this after discussion with @larowlan on IRC yesterday.

StatusFileSize
new359.4 KB
FAILED: [[SimpleTest]]: [MySQL] 49,010 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Merged 8.x and added fixes from #1903138: Move global comment permissions to field level

EDIT The changes from #1807776: Support both simple and editorial workflows for translating entities could be used to allow setup permissions from comments more granularly.

Change notices http://drupal.org/node/1903124 http://drupal.org/node/1903128

Fixing tests I found that comment widget uses 'administer comments' to shot settings on entity form - suppose this assumption is wrong

Status:Needs review» Needs work

@andypost: Is it really the correct issue you mention on the first line in #291? Maybe you mixed it up with #1901110: Improve the UX for comment bundle pages and comment field settings?

Status:Needs work» Needs review
Issue tags:+Needs usability review

Suppose we need usability review for #284 and others, "Comment bundles" now "Comment fields"

Status:Needs review» Needs work

@andypost "Comment forms" you mean? Tried the latest patch in #291 and it looks good now.

Besides that text I can only confirm that the Default comment setting in #286 now is saved properly.

Anything else that has been committed?

StatusFileSize
new5.95 KB
new6.71 KB

One thing I noticed, using the Standard profile, is that in the the field name for the comment form on the Article content type is just "comment", while anyone I add manually automatically gets the prefix "field_".
comment_forms_field_name.png

This also gets reflected in the table names:
comment_forms_field_tables.png

Some of the other fields seems to miss the field_prefix to, such as the both the comment_body and body fields, while others such as image and tags does have the prefix.

I did patch D8 before performing the install.

Status:Needs work» Needs review
StatusFileSize
new359.48 KB
PASSED: [[SimpleTest]]: [MySQL] 49,023 pass(es).
[ View ]
new1.44 KB

@tsvenson Yes other UI changes will go in follow-up patches because all of them require form_alter's

Last broken test fixed.

Status:Needs review» Needs work

Hi @tsvenson
This is standard in core already, see for example the body field on the node content type.
The field_ prefix is added to all fields added via the ui.
Modules can create fields without this prefix.
The upgrade path for D7 to D8 moves the existing comment bundles to fields named comment_article and comment_page to respect any custom fields that are present on the existing comment bundles (which are per node-type in Drupal 7).
If a user wants to re-use the existing comment bundle (field name is comment) created in standard, they can do so just like any other field - they just use the 'use existing field' section of the field ui instead of creating a new one.

Lee

This is pretty amazing already. Some first remarks, will do more in depth review and testing this weekend.

+++ b/core/modules/comment/comment.field.incundefined
@@ -0,0 +1,294 @@
+function comment_field_formatter_view(EntityInterface $entity, $field, $instance, $langcode, $items, $display) {
+  $element = array();
+
+  switch ($display['type']) {
+    case 'comment_default':

I wonder whether we need to add more formatter options or not so that we can actually use them in hook_entity_view because adding the links there is now hard coded. Maybe we should add a formatter called 'links' which can be configured on the teaser view (or any view mode if wanted).

Just a brainstorm, not sure if this is actually practical or not. This shouldn't hold the actual patch from going btw. What we'll also probably need is a separation between 'Manage fields' and 'Manage display' label, because the default 'Comment settings' label on 'Manage display' is confusing. There is an issue for that already IIRC and should not block this patch either.

+++ b/core/modules/comment/comment.field.incundefined
@@ -0,0 +1,294 @@
+              !in_array($entity->content['#view_mode'], array('search_result', 'search_index'))) {

Why is this hardcoded ? This might confuse people again in case they would add the comment field on the view mode on the manage display screen. Granted, they don't have any use in search_index, but still.

+++ b/core/modules/comment/comment.installundefined
@@ -276,6 +311,26 @@ function comment_schema() {
+function comment_field_schema($field) {
+  $columns = array();
+  if ($field['type'] == 'comment') {

You don't need to check on $field['type'], the only field that comes in here is the comment field, this hook is invoked directly on the module owning the field.

+++ b/core/modules/comment/comment.installundefined
@@ -394,6 +449,278 @@ function comment_update_8004() {
+      'description' => '',
+      'display' => array(
+        'default' => array(
+          'label' => 'above',
+          'module' => 'comment',
+          'settings' => array(),
+          'type' => 'comment_default',
+          'weight' => '1',
+        ),
+        'rss' => array(
+          'type' => 'hidden',
+          'label' => 'hidden',
+        ),
+        'teaser' => array(
+          'type' => 'hidden',
+          'label' => 'hidden',
+        ),
+        'search_index' => array(
+          'type' => 'hidden',
+          'label' => 'hidden',
+        ),
+        'search_result' => array(
+          'type' => 'hidden',
+          'label' => 'hidden',
+        ),

This can be removed, display settings are not stored anymore in the instance, happens also in the _comment_body_field_create().

+++ b/core/modules/comment/comment.moduleundefined
@@ -99,6 +124,124 @@ function comment_help($path, $arg) {
/**
+ * Implements hook_entity_view().
+ */
+function comment_entity_view(EntityInterface $entity, EntityDisplay $display, $view_mode, $langcode) {
+  $fields = field_info_instances($entity->entityType(), $entity->bundle());
+  foreach ($fields as $field_name => $instance) {
+    $links = array();
+    $field = field_info_field($instance['field_name']);
+    if ($field['type'] != 'comment') {
+      continue;

This worries me a little because it might cause a possible performance impact as it's also kind of hardcoded. With the EntityDisplay patch we tried to minimize loading of fields and/or instances for an entity that are not needed.

The $display should contain the field imo (see also a bit above where I suggest adding a second formatter called 'links') so you don't have to loop through the fields.

Does not mean to change tags

There's UI changes to discuss in #1901110: Improve the UX for comment bundle pages and comment field settings - the idea to move some settings to formatter level and this also affects upgrade path to move old variables to apropriate "field-entity"

Status:Needs work» Needs review
StatusFileSize
new359.59 KB
FAILED: [[SimpleTest]]: [MySQL] 49,335 pass(es), 0 fail(s), and 2 exception(s).
[ View ]
new41.26 KB

Let's see results of merge

StatusFileSize
new15.71 KB
new364.34 KB
FAILED: [[SimpleTest]]: [MySQL] 49,341 pass(es), 4 fail(s), and 0 exception(s).
[ View ]

Another round of fixes:
- broken ER tests
- removed display definition
- fixes in upgrade path

Status:Needs review» Needs work

Note I removed all of the 'System Message' comments to keep this under 300 and avoid the pager. There were 33 of these.

Status:Needs work» Needs review
StatusFileSize
new1.25 KB
new365.01 KB
PASSED: [[SimpleTest]]: [MySQL] 49,353 pass(es).
[ View ]

Display still needed to convert view-modes

+++ b/core/modules/comment/comment.installundefined
@@ -568,6 +568,31 @@ function comment_update_8006(&$sandbox) {
+      'display' => array(
+        'default' => array(
+          'label' => 'above',
+          'module' => 'comment',
+          'settings' => array(),
+          'type' => 'comment_default',
+          'weight' => '1',
+        ),
+        'rss' => array(
+          'type' => 'hidden',
+          'label' => 'hidden',
+         ),
+         'teaser' => array(
+           'type' => 'hidden',
+           'label' => 'hidden',
+         ),
+         'search_index' => array(
+           'type' => 'hidden',
+           'label' => 'hidden',
+         ),
+         'search_result' => array(
+           'type' => 'hidden',
+           'label' => 'hidden',
+         ),
+       ),

We should be able to use entity_get_display() here eg

entity_get_display('node', 'node_type, 'default')
      ->setComponent('comment_node_' . $node_type, array(
        'label' => 'hidden',
        'type' => 'comment_default',
        'weight' => 1,
        'settings' => array(
          // Settings go here.
        )
      ))
      ->save();

Repeating for each view mode.

In general this looks good, IMHO.
But in order to get this patch landed I think we have start doing some performance tests, since there's quite a few things that can have an effect here.

Also, we need to start creating follow-up issues to keep further reviews focused on getting this landed.

I haven't found time to do a detailed review yet, but looking through the patch I found some potential follow-up issues we might need to create:

+++ b/core/modules/comment/comment.module
@@ -1010,233 +1045,125 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
+function comment_entity_load($entities, $entity_type) {
+  // Load comment information from the database and add it to the entity's
+  // comment_statistics property, which is an array keyed by field_name.
+  $result = db_select('comment_entity_statistics', 'ces')
+    ->fields('ces')
+    ->condition('ces.entity_id', array_keys($entities))
+    ->condition('ces.entity_type', $entity_type)
+    ->execute();
+  foreach ($result as $record) {
+    if (empty($entities[$record->entity_id]->comment_statistics)) {
+      $entities[$record->entity_id]->comment_statistics = array();
     }

Doesn't really belong to this issue, but I'm still noting;
One of the fundamental goals with the new Entity API is to only have fields on the $entity object, no arbritary data structures. But again, that's a separate issue.

+++ b/core/modules/comment/comment.module
@@ -2062,8 +2051,16 @@ function comment_rdf_mapping() {
-      $node = $entity->nid->entity;
-      return node_access('view', $node);
+      // @todo replace this with entity access controls once generic access controller lands,
+      // @see http://drupal.org/node/1696660
+      $function = $entity->entity_type->value . '_access';
+      if (function_exists($function)) {
+        $commented_entity = entity_load($entity->entity_type->value, $entity->entity_id->value);
+        return $function('view', $commented_entity);
+      }
+      // Return NULL as there is no entity access callback for this entity
+      // type, we rely on other modules intervening.

Let's create a follow-up issue for this since generic Entity Access API has landed.

+++ b/core/modules/comment/comment.tokens.inc
@@ -16,13 +16,14 @@ function comment_token_info() {
-  $node['comment-count'] = array(
+  // @todo make this work per field.

Let's create a follow-up issue about this.

StatusFileSize
new363.73 KB
FAILED: [[SimpleTest]]: [MySQL] 50,468 pass(es), 4 fail(s), and 4 exception(s).
[ View ]

Here's a simple re-roll to chase HEAD.

Issue summary:View changes

Added links to sandbox

Status:Needs review» Needs work

Hm, I had to do a manual merge when re-rolling the patch due to some changes in the Views integration. But I seems to have failed somewhere. I'll try to take a closer look at that later today.

@dixon_ We have a helper issue to test patches because this one is >300 comments

I've spend this weekend to re-work upgrade path and convert widget and formatters #1907960-10: Helper issue for "Comment field"
I can't reproduce failures in tests locally but bot tells random errors in upgrade tests (4-6) Probably we should work with plain config for entity_get_display()

From architectural side there's some problems:
1) Field created from Field UI does not get default value, suppose we need help from FiedlAPI team to fix this properly. So I changed all reset($items) to isset($items[0]['comment']) and fallback to default COMMENT_OPEN. The default (list) formatter use "comment_entity_statistics" to decide a render of comments.

2) Removal of hook_entity_view() require second formatter (introduced comment_links with settings). This formatter needs to know is the default display of entity using threading to count a page number (I use "default" because core entities have different display names for full entity-view, ugly but works) So "Comment links" formatter still need some love to work properly with node's search_index view mode.

3) Tracker module. In case of more then one comment field assigned to node the render of tracker could be funny.

4) comment_entity_statistics - state() works globally. This could be addressed in follow-up - add settings per field/instance to track statistics

5) FieldAPI conversion to config entities now grean #1906218-67: Helper issue for "Field CMI" so patch will require another merge and changes in upgrade path.

Also I'm in progress to convert field instance settings to widget settings #1907960-14: Helper issue for "Comment field" (Mostly done but needs to fix tests)
The new UI introduced in #1901110-6: Improve the UX for comment bundle pages and comment field settings
This change leaves some old-variables for BC stored in instance settings.

* Field instance settings "comment_default_mode", "comment_default_per_page"
* and "comment_form_location" are preserved to allow migrate contrib modules.

@dixon_, @andypost - can we make a battle plan for the remaining issues?
Which ones can we leave for follow ups, which ones have to go in this week?
We really want to see this get in, so much work has been done it would be wasted if this was pushed back to Drupal 9.

#311: 731724-comment-decouple-311.patch queued for re-testing.

Re-roll and go-go-go in Drupal 8! :)

Now green again #1907960-29: Helper issue for "Comment field"

Changes from sandbox:
Formatter and widget are now plugins
Upgrade path fixed to displays and default value moved to field default
Some code-cleanup

EDIT
Menu renamed to Comment forms

Needs architectural review & fixes some work on views plugins

TBD

:
- Move threading and per page settings to Formatter settings
- Probably needs change upgrade path to attach field only for node types that have comments or enabled commenting
- Remove hook_entity_view (probably better introduce second formatter for links)
- Small API changes to make views plugins work when settings are moved to formatter
- Decide on tracker and history modules integration

I think the TBD's listed at #319 qualify as follow ups, but I'm biased!

Awesome work on this andypost and everyone else!
For me, the todo's in #319 are fine to deal with in follow-ups.

Personally, I want to walk through the patch completely before I can give a word on it. I'll try to do that tonight. It'd be great if I could get some support on IRC tonight, because it's quite a big patch :)

Skimmed through ~50% the patch, some feedback below.

+++ b/core/modules/comment/comment.moduleundefined
@@ -136,22 +270,20 @@ function comment_entity_bundle_info() {
+ * @param string $arg
+ *   The field name with underscores as hyphens.
...
+ *   The actual field name with hyphens returned to underscores.
...
+function comment_field_name_load($arg) {
+  if (($field = field_info_field($arg)) && $field['type'] == 'comment') {
+    return $field['field_name'];
   }
+
+  return FALSE;

This function doesn't seem to do much other than the field type check, the underscore/hyphen comments are no longer true it seems...

+++ b/core/modules/comment/comment.moduleundefined
@@ -281,93 +417,105 @@ function comment_menu() {
+ * @param array $args
+ *   The menu args passed from the %map load argument.
+ *
+ * @return mixed
+ *   Returns Drupal\entity\Entity or FALSE if not found.
...
+function comment_entity_reply_load($entity_id, $args) {
+  list(, ,$entity_type, $entity_id, $field_name) = $args;
+  return entity_load($entity_type, $entity_id);

@param and actual arguments don't match.

Also, the @return type should state \Drupal\Core\Entity\EntityInterface, that's how entity_load() is documented too.

Wondering how much we actually gain from this function and if we should't just load it in the page callback. This might or might not be more complicated with the new routing system.

+++ b/core/modules/comment/comment.moduleundefined
@@ -813,32 +831,38 @@ function comment_add(Node $node, $pid = NULL) {
-function comment_get_thread(Node $node, $mode, $comments_per_page) {
+function comment_get_thread(\Drupal\Core\Entity\EntityInterface $entity, $field_name, $mode, $comments_per_page) {

Shouldn't inline the namespace.

+++ b/core/modules/comment/comment.moduleundefined
@@ -813,32 +831,38 @@ function comment_add(Node $node, $pid = NULL) {
+    ->addTag('entity_access')
...
+    ->addTag('entity_access')

This still uses the entity_access tag, unlike other places.

In general, this is still the thing that I'm most worried about but I haven't heard back from the security team.

+++ b/core/modules/comment/comment.moduleundefined
@@ -813,32 +831,38 @@ function comment_add(Node $node, $pid = NULL) {
-  if ($mode === COMMENT_MODE_FLAT) {
+  if ($mode == COMMENT_MODE_FLAT) {
     $query->orderBy('c.cid', 'ASC');

Unecessary change?

+++ b/core/modules/comment/comment.moduleundefined
@@ -921,17 +947,21 @@ function comment_view(Comment $comment, $view_mode = 'full', $langcode = NULL) {
-function comment_links(Comment $comment, Node $node) {
+function comment_links(Comment $comment, \Drupal\Core\Entity\EntityInterface $entity, $field_name) {

Another inline namespace

+++ b/core/modules/comment/comment.moduleundefined
@@ -1010,233 +1044,125 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
+    // Force cardinality to one.
+    $form['field']['cardinality']['#options'] = drupal_map_assoc(array(1));

Wasn't this also #access false before?

Wondering if this might be confusing as users might think that you can only add a single comment. Why not just hide it?

I'd also totally support an issue to add support for a forced cardinality to field types.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1010,233 +1044,125 @@ function comment_view_multiple($comments, $view_mode = 'full', $langcode = NULL)
+    if (empty($entities[$record->entity_id]->comment_statistics[$record->field_name])) {
+      $entities[$record->entity_id]->comment_statistics[$record->field_name] = new \StdClass();

As others AFAIK already said, I'm quite sure this will break with NG.

It also adds an additional query to every entity load, including config entities I think? Should probably at least check if that entity is fieldable and has comments fields before doing that?

I'm fine with doing that in a follow-up.

+++ b/core/modules/comment/comment.moduleundefined
@@ -1283,7 +1219,7 @@ function comment_node_update_index(Node $node, $langcode) {
-  state()->set('comment.node_comment_statistics_scale', 1.0 / max(1, db_query('SELECT MAX(comment_count) FROM {node_comment_statistics}')->fetchField()));
+  state()->set('comment.node_comment_statistics_scale', 1.0 / max(1, db_query('SELECT MAX(comment_count) FROM {comment_entity_statistics}')->fetchField()));

The state variable should probably be renamed, but again, can be a follow-up.

I'd say it could be as short as comment.statistics_scale but we already discussed that when converting to state.

+++ b/core/modules/comment/comment.moduleundefined
@@ -2063,8 +2053,16 @@ function comment_rdf_mapping() {
-      $node = $entity->nid->entity;
-      return node_access('view', $node);
+      // @todo replace this with entity access controls once generic access controller lands,
+      // @see http://drupal.org/node/1696660
+      $function = $entity->entity_type->value . '_access';
+      if (function_exists($function)) {
+        $commented_entity = entity_load($entity->entity_type->value, $entity->entity_id->value);
+        return $function('view', $commented_entity);

Is there a reason this doesn't use the function that I've seen somewhere above?

Comments are currently being converted to the access controller, the view implementation should probably take care of this once that is in.

+++ b/core/modules/comment/comment.pages.incundefined
@@ -14,40 +15,48 @@
  */
-function comment_reply(Node $node, $pid = NULL) {
+function comment_reply(Drupal\Core\Entity\EntityInterface $entity, $field_name, $pid = NULL) {

Another namespace thing.

Looks like you actually add a use but just for "Entity", not the interface? Is that actually needed?

+++ b/core/modules/comment/comment.pages.incundefined
@@ -14,40 +15,48 @@
-  drupal_set_breadcrumb(array(l(t('Home'), NULL), l($node->label(), 'node/' . $node->nid)));
+  // @todo - test behaviour when entities don't have uris.
+  $uri = $entity->uri();
+  drupal_set_breadcrumb(array(
+    l(t('Home'), NULL),
+    l($entity->label(), $uri['path'], $uri['options']))

I need to check again but I think entities now always have a URI, I think such a patch was recently committed to support serialization references or something like that.

+++ b/core/modules/comment/comment.pages.incundefined
@@ -121,7 +144,34 @@ function comment_approve($cid) {
+ * @throw Symfony\Component\HttpKernel\Exception\NotFoundHttpException

Missing leading \

Assigned:larowlan» dixon_
StatusFileSize
new7.28 KB
new370.04 KB
FAILED: [[SimpleTest]]: [MySQL] 50,809 pass(es), 13 fail(s), and 37 exception(s).
[ View ]

Here's a patch that fixes all Berdir's comments.

Wondering how much we actually gain from this function and if we should't just load it in the page callback. This might or might not be more complicated with the new routing system.

Not much, but let's fix/remove that when we are converting over to the new routing system for the menu items, no?

Wasn't this also #access false before?

Don't think so, but I added it since it makes sense. I also clarified the comments around that as well.

As others AFAIK already said, I'm quite sure this will break with NG.
I'm fine with doing that in a follow-up.

Yes, I consider this to be a pure conversion. So let's deal with that deeper issue in a follow-up.

The state variable should probably be renamed, but again, can be a follow-up.

Yes, let's do that in a follow-up since it doesn't have anything to do with this issue really.

Is there a reason this doesn't use the function that I've seen somewhere above?

I converted this to the proper entity access API.

+++ b/core/modules/comment/comment.moduleundefined
@@ -2053,16 +2058,10 @@ function comment_rdf_mapping() {
     if (user_access('access comments') && $entity->status->value == COMMENT_PUBLISHED || user_access('administer comments')) {
-      // @todo replace this with entity access controls once generic access controller lands,
-      // @see http://drupal.org/node/1696660
-      $function = $entity->entity_type->value . '_access';
-      if (function_exists($function)) {
-        $commented_entity = entity_load($entity->entity_type->value, $entity->entity_id->value);
-        return $function('view', $commented_entity);
+      $commented_entity = entity_load($entity->entity_type->value, $entity->entity_id->value);
+      if ($commented_entity instanceof AccessibleInterface) {
+        return $commented_entity->access('view');
       }
-      // Return NULL as there is no entity access callback for this entity
-      // type, we rely on other modules intervening.
-      return;

Hm.

If this doesn't fail then this simply means we have no test coverage for private files on comments, which is likely. This currently returns a hardcoded FALSE for everything but users and terms.

What I meant is that we should use comment_reply_access() here, which contains the same check + additional special cases for other entity types. Then we can replace that once all entity types have a correct access controller.

Also, EntityInterface extends from AccessibleInterface. There is no need to check this interface here, all entities implement it.

Issue summary:View changes

Added links to helper issue

Issue summary:View changes

Updated issue summary to be more expressive.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Corrected html.

Status:Needs review» Needs work

function comment_update_8006(&$sandbox) :
+    // Prepare defaults for the default and full view modes.
+    $formatter_settings = array(
+      'default_mode' => $instance_settings['comment_default_mode'],
+      'per_page' => $instance_settings['comment_default_per_page'],
+      'form_location' => $instance_settings['comment_form_location'],
+    );
+    $display_options_default = array(
+      'label' => 'hidden',
+      'type' => 'comment_default',
+      'settings' => array(),
+      'weight' => 1,
+    );

$formatter_settings is never actually used ? Don't you want to just inline it within $display_options_default ?

Awesome work. I mostly looked at the Field API related stuff so far, here are my remarks :

class CommentDefaultFormatter extends FormatterBase {
+    // @todo Alter default field instance settings to
+    // disallow select more then 1 value cardinality.

Looks like this was done and the @todo can be removed ?

class CommentDefaultFormatter extends FormatterBase
+    $commenting_status = isset($items[0]['comment']) ? $items[0]['comment'] : COMMENT_OPEN;

- 'comment' is not too accurate as a column name. 'status' or 'comment_status' maybe ?
- isn't that where you'd typically use a short ternary : $items[0]['comment'] ?: COMMENT_OPEN
- Also, that very same variable is named $default_value in CommentWidget ::formElement(), while it seems it has nothing to do with a 'default value' ?

CommentDefaultFormatter ::formElement():
+    // Integrate with advanced settings, if available.
+    if (isset($form['advanced'])) {
+      $element += array(
+        '#type' => 'details',
+        '#group' => 'advanced',
+        '#access' => user_access('administer comments'),
+        '#collapsed' => TRUE,
+        '#attributes' => array(
+          'class' => array('comment-node-settings-form'),

- if the form has no 'advanced' vertical tabs, we don't check user_access('administer comments') ?
- class comment-*node*-settings-form ?

+ * @return array
+ *   An array of comment field definitions, keyed by field name. Each field has
+ *   an additional property, 'bundles', which is an array of all the bundles to
+ *   which this field belongs, keyed by entity type.
+ *
+ * @see field_info_fields().
+ */
+function comment_get_comment_fields($entity_type = NULL) {

The phpdoc for the return value probably dates from a previous state of the patch when the function was based on field_info_fields(), but is incorrect now that it's (rightly) based on field_info_field_map().

function comment_field_info()
+      'instance_settings' => array(
+        'comment' => array(
+          // Save this settings to allow other modules to migrate their data.
+          // The "comment_default" formatter settings are used instead.
+          'comment_default_mode' => COMMENT_MODE_THREADED,
+          'comment_default_per_page' => 50,
+          'comment_form_location' => COMMENT_FORM_BELOW,
+          // Comment form settings per field bundle.
+          'comment_anonymous' => COMMENT_ANONYMOUS_MAYNOT_CONTACT,
+          'comment_subject_field' => 1,
+          'comment_preview' => DRUPAL_OPTIONAL
+        )
+      ),

- Why are the instance settings nested under a (seemingly useless) 'comment' level ? Is that because of the structure of the corresponding settings form (nested under a $form['comment'] = array('#type' => 'details')). That's awkward.
[edit: also, the 'comment_' prefix on each individual setting name is superfluous and needlsessly verbose]
- missing comma after a closing parenthesis towards the end :-)

+/**
+ * Implements hook_field_widget_info_alter().
+ *
+ * @todo convert this to a WidgetPlugin annotation.
+ */
+function comment_field_widget_info_alter(&$info) {
+  $info['comment_default']['behaviors'] = array(
+    'multiple values' => FIELD_BEHAVIOR_CUSTOM,
+    'default value' => FIELD_BEHAVIOR_NONE,
+  );
+}

'behaviors' & FIELD_BEHAVIOR_* no longer exist in widget_info entries, and yup, the @todo should be done :-)

/**
+ * Implements hook_field_schema().
+ */
+function comment_field_schema($field) {
+  $columns = array(
+    'comment' => array(
+      'description' => 'Whether comments are allowed on this entity: 0 = no, 1 = closed (read only), 2 = open (read/write).',
+      'type' => 'int',
+      'not null' => TRUE,
+      'default' => 0,
+    ),
+  );
+  return array(
+    'columns' => $columns,
+  );
+}

Minor, but why a separate $columns variable ? Why not just directly return array('columns' => array(...) ?

function comment_update_8006(&$sandbox)
+  foreach ($node_types as $node_type) {
+    $field = array(
+      // We need one per content type to match the existing bundles.
+      'field_name' => 'comment_node_' . $node_type,

Not that I have given this much thought, just curious : why one separate field per node type, instead of one single field with instances in each node type ?

function comment_update_8006(&$sandbox)
+    // Update all other entity displays with custom settings.
+    $display_options = array(
+      'type' => 'hidden',
+      'label' => 'hidden',
+      'settings' => array(),
+    );
+    foreach (entity_get_view_modes('node') as $view_mode => $view_mode_settings) {
+      if ($view_mode != 'full') {
+        $display = _update_8000_entity_get_display('node', $node_type, $view_mode);
+        $display->set('content.' . $field['field_name'], $display_options)
+          ->save();
+        update_config_manifest_add('entity.display', array($display->get('id')));
+      }
+    }

This is not needed, fields not present in the EntityDisplay are considered hidden.

comment_entity_view()
+    $values = field_get_items($entity, $field_name);
+    if ($values && is_array($values) && ($value = reset($values)) &&
+        !empty($value['comment']) &&
+        $value['comment'] != COMMENT_HIDDEN) {

Wow. Does that if() really needs to be that convoluted ?

comment_entity_view()
+ $values = field_get_items($entity, $field_name);
+ if ($values && is_array($values) && ($value = reset($values)) &&
+ !empty($value['comment']) &&
+ $value['comment'] != COMMENT_HIDDEN) {

Wow. Does that if() really needs to be that convoluted ?

Until we have EntityNG for all entities, yes ;(.
Once we have EntityNG we can go $node->{$field_name}->value for any of these.
I guess we should add a @todo to that effect.

Status:Needs work» Needs review
StatusFileSize
new367.56 KB
FAILED: [[SimpleTest]]: [MySQL] 50,452 pass(es), 15 fail(s), and 31 exception(s).
[ View ]
new34.52 KB

@yches thanx for nice idea about status is field! The only trouble here that I can't assign "default_value" COMMENT_OPEN for field instance now so all over a code patch does $commenting_status = isset($values[0]['status']) ? $values[0]['status'] : used to not throw notices.
#334 shows a way to simplify it but we also need better solution for field_UI - filed follow-up #1919834: Field instance got no default value when created in field UI

Patch also fixes a lot places where drupalCreateNode() and deleted direct settings for field default value. Because we have this defaults added in itself.

Formatter settings are removed. This hunk was from follow-up issue to move some settings to widget and formatter #1901110: Improve the UX for comment bundle pages and comment field settings

Upgrade path cleaned-up and now updated comment_entity_statistics table too.

StatusFileSize
new362.88 KB
PASSED: [[SimpleTest]]: [MySQL] 51,134 pass(es).
[ View ]
new9.41 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0003-Fix-access-to-files-and-clean-up-remains-of-ER-trans.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new19.78 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0002-Fixed-all-tests-after-conversion-except-FileFieldWid.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
new36.42 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 0001-Field-storage-column-changed-from-comment-to-status.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Should be green

So current state:
- Field storage column changed from comment to status
- Fixed all tests after conversion
- Fix access to files and clean-up remains of ER transition

So what else is needed here?
We have entity reviews from @berdir.
We have field api reviews from @yched.
We have ui reviews.

We have a list of follow up tasks
- Move threading and per page settings to Formatter settings #1920044: Move comment threading and per page settings to formatter settings
- Remove hook_entity_view (introduce second formatter for links) #1920046: Remove comment_entity_view and introduce second formatter for links
- Small API changes to make views plugins work when settings are moved to formatter #1920044: Move comment threading and per page settings to formatter settings
- Decide on tracker and history modules integration #1029708: History table for any entity
- Move part of instance settings to formatters #1919834: Field instance got no default value when created in field UI
- Permissions follow up #1903138: Move global comment permissions to field level
- UI fixes #1901110: Improve the UX for comment bundle pages and comment field settings

I think this is at the point where we ship it and deal with anything left in follow ups.

This lets us drop a lot of architectural baggage and move forward.

Thoughts?

Status:Needs review» Reviewed & tested by the community

Unanswered from #333 :

function comment_field_info()
+      'instance_settings' => array(
+        'comment' => array(
+          // Save this settings to allow other modules to migrate their data.
+          // The "comment_default" formatter settings are used instead.
+          'comment_default_mode' => COMMENT_MODE_THREADED,
+          'comment_default_per_page' => 50,
+          'comment_form_location' => COMMENT_FORM_BELOW,
+          // Comment form settings per field bundle.
+          'comment_anonymous' => COMMENT_ANONYMOUS_MAYNOT_CONTACT,
+          'comment_subject_field' => 1,
+          'comment_preview' => DRUPAL_OPTIONAL
+        )
+      ),

Why are the instance settings nested under a (seemingly useless) 'comment' level ? Is that because of the structure of the corresponding settings form (nested under a $form['comment'] = array('#type' => 'details')). That's awkward.
Also, the 'comment_' prefix on each individual property name is superfluous and needlsessly verbose.

That 'comment' nesting level is useless and de-facto means "there is one single setting, which is an array of properties".
- This doesn't work well for default values (the array is considered the default value as a whole, so if a value is present for any of the keys, the others won't be filled)
- This will be painful when we have to provide metadata (as is "configuration metadata") for those.

Please drop that 'comment' level and go with "N individual settings".

Also :
Not that I have given this much thought, just curious : why one separate field per node type, instead of one single field with instances in each node type ?

Other than that, minor remarks, could be adressed in a followup :

+        '#attributes' => array(
+          'class' => array('comment-' . $element['#entity_type'] . '-settings-form'),
+        ),

No good for entity_type names composed of underscore-separated words.
Why do we even need the entity type in there ? Class "comment-settings-form" should be enough ?

+ * @return array
+ *
+ * @see field_info_fields().
+ */
+function comment_get_comment_fields($entity_type = NULL) {

The phpdoc was updated, but the @see field_info_fields() mention is still stale. Probably just needs to go away.

Why are the instance settings nested under a (seemingly useless) 'comment' level ? Is that because of the structure of the corresponding settings form (nested under a $form['comment'] = array('#type' => 'details')). That's awkward.

If that's the reason, you guys can just copy what entity_reference does for this exact situation:

<?php
  $form
['handler'] = array(
   
'#type' => 'details',
   
'#title' => t('Reference type'),
   
'#tree' => TRUE,
   
'#process' => array('_entity_reference_form_process_merge_parent'),
  );
...
function
_entity_reference_form_process_merge_parent($element) {
 
$parents = $element['#parents'];
 
array_pop($parents);
 
$element['#parents'] = $parents;
  return
$element;
}
?>

Also, I assume that in the future this field will extend entity reference, right? But no need to worry about it for now :)

It's funny that I've implemented a variant of #343 in half of the contribs I've written (usually by adding a #fieldset key, meaning "add me to this element without reflecting it in #parents", done in pre render). Views for D8 might still have that trick too, since it was originally written for the "new" Views UI back in D7.

Might be something worth standardizing in core.

why one separate field per node type, instead of one single field with instances in each node type ?

To simplify the upgrade path. D7 comments already have "comment_node_$type" as it's bundle to provide fields assigned for each bundle.
For example comments for article have only body field but comments for book page have a media file attached....

So instead of changing bundle names for existing comments we inherit their bundle names.

Why are the instance settings nested under a (seemingly useless) 'comment' level ? Is that because of the structure of the corresponding settings form (nested under a $form['comment'] = array('#type' => 'details')). That's awkward.

We should fix this in follow-up issue where this settings will be scattered around formatters see #1901110-6: Improve the UX for comment bundle pages and comment field settings

Also this change require a lot of find/replace but the same is needed for formatters which are in-progress with upgrade path tests.

Why do we even need the entity type in there ? Class "comment-settings-form" should be enough

Yep, this needs special discussion because "themers" always needs something other :)

Main trouble with the patch now is a node_row_node_view_preprocess_node() that used to display a list of comments but the node-preprocess is not a right place to fetch and render comments

Just to note that @catch has confirmed this is a task and can be resolved post feature freeze.
Awesomesauce!

- (yched) why one separate field per node type, instead of one single field with instances in each node type ?
- (andypost) To simplify the upgrade path. D7 comments already have "comment_node_$type" as it's bundle to provide fields assigned for each bundle.
For example comments for article have only body field but comments for book page have a media file attached....
So instead of changing bundle names for existing comments we inherit their bundle names.

Oh ok, that's because the patch does "one comment bundle per existing 'comment field' ". OK, that's probably a sound approach, and I see it's been discussed at length above.
So, right, that means upgraded D7 sites, where there was one comment bundle per node type, need to get one different comment field per node type, so that the existing collections of fields on existing comment bundles can be preserved.

+        '#attributes' => array(
+          'class' => array('comment-' . $element['#entity_type'] . '-settings-form'),
+        ),

- (yched) Why do we even need the entity type in there ? Class "comment-settings-form" should be enough
- (andypost) Yep, this needs special discussion because "themers" always needs something other :)

I'd think the surrounding markup provides enough context on what entity type you're editing to hang your CSS, but, why not.
But then at least 'comment-' . drupal_html_class($element['#entity_type']) . '-settings-form' ?

Setting back 'instance_settings' to a flat array of settings : OK with deferring that to #1901110: Improve the UX for comment bundle pages and comment field settings.

What surprises me is that $node->content['comments'] does not exist anymore within node_view(). Instead you get $node->content['comment_field_foo'], like for any other field, which is logical.
But yet the patch does not touch node.tpls, where the common pattern is print render($content['comments']); , while $content['comments'] does not exist anymore ?

Issue tags:+RTBC Feb 18

@yched see changes commited to sandbox #1907960: Helper issue for "Comment field" I think mostly all addressed

The last issue to solve here architecturally is about settings for comment form.

Currently we have to make changes at each content type about commenting:: default value (open/closed), "threading+per_page" and form settings (subject, preview button, anonymous).

Any of changes will require a lot of test-fixes so should be done in follow-up.

So any suggestions about "scattering" of comment settings please post to #1901110: Improve the UX for comment bundle pages and comment field settings

There's a some cases to take into account:
1) Most of sites have global and separate settings for commenting form!
1.a) this setting could live on per field level - moving to field annotation?
1.b) introduce config entity to provide defaults a-la image-style?
2) threading and per page goes to CommentList formatter settings - there's some issue to fix tests. I'll attach patch latter in #1901110: Improve the UX for comment bundle pages and comment field settings
3) Place to display the Comment Form - suppose this move to CommentList formatter settings too
4) comment_entity_view() - introduce second CommentLinks formatter with some hack to move comment links to node links

There are some more use cases in the #1903138: Move global comment permissions to field level follow up issue as well.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Added follow-ups

I've added all the follow-ups to the issue summary.

StatusFileSize
new374.75 KB
PASSED: [[SimpleTest]]: [MySQL] 51,787 pass(es).
[ View ]
new3.59 KB

Adding patches from #1907960: Helper issue for "Comment field" where @andypost has been working around the clock to get this green again.
This addresses upgrade path and changes to node.tpl.php
Also deleted 3 system messages to try and avoid the pager.

Title:Decouple comment.module from nodeDecouple comment.module from node by turning comments into a field

Ugh. I really dislike coming in at 300+ comments and shooting things down but... what..? Fixing the issue title so the next person is not as stunned as I am that this is the proposed resolution for this feature.

Can someone point out how and why the decision to turn comments into a field get made? Typically, fields are for storing data. Not for attaching arbitrary functionality to an entity. They typically map to attributes of an entity itself, of which there can be multiple on the same entity: Link, Boolean, Text, Date. Not for things that in 99.999% of cases only ever make sense to add to an entity once. It clutters up the list of field types and makes this confusing.

From the issue title, I had expected that the approach we were going to take here was similar to what's described in the OP/earlier responses: namely, expanding the comment table with entity ID/type, rather than nid. Then I assumed once Entity References made it in, that Comments would simply get an entity reference field which could point to a node, a user, or another comment (in the case of threaded comments). And then the display of comments below a node/user would be a view. Help? Am I on crazy-glue?

Issue tags:+Performance

Conversation from irc

webchick
So… can someone explain to me the logic in these "X as a field" issues? specifically comments and URL aliases
On what planet do you ever want more than one URL alias or "comment" field on a node?
That feels a lot like "This hammer is so very nice for pounding all of these screws!"
larowlan
webchick: more than one comment on a node - for and against
webchick: or admin comments / user comments
webchick
larowlan: why is it not just a entity reference field..?
webchick
Sorry, I really don't want to come in at 350! replies but
I never imagined from the issue title that that's what this issue was about.
heyrocker
URL aliases as a field makes more sense that url aliases as entities imo
webchick
URL alias should just be an entity property.
Like node title
there's only ever one of them
heyrocker
that's not true at all
for better or worse
webchick
And it totally gunks up the UI to see things like "URl alias" in there with "Link, Boolean, Date"
webchick
heyrocker: Well, I tested that patch with 2 URL alias fields and it kills the input of one of them completely.
larowlan
but comments is different to url alias, it does make sense to see it in there imo
webchick
larowlan: So again, why not an entity reference field..?
heyrocker
webchick i havent actually tested the patch, im just saying conceptually
comments as a field boggles my mind
larowlan
webchick: as in on each comment?
webchick
Well, as in comment entities have a entity reference to node (or user, or whatever they're referring to)
webchick
or comment, in the case of threading
heyrocker
how does that work when you want to list all the comments for a node?
are entity references bi-directional?
larowlan
webchick: I guess that would work but how would you tell the node to render the list/form, with this arrangment the field is on the host, not the comment
heyrocker
the old nodereferences werent
webchick
Well I assumed that'd be a view.
larowlan
webchick: so you can move that placement around like any other field, and with our follow ups, you can control the # per page etc
that was our thinking
so no need for hiding comments in tpl and rendering them elsewhere, just move around in display settings
heyrocker
so its just a theming issue really in the end
theming and layout
larowlan
well the first driver was cmi
pre config entities
larowlan
the issue was comment config was related to node type
heyrocker
right
larowlan
there was an issue 'update comment variables to cmi'
heyrocker
i remember this discussion
yes
webchick
So a Entity reference field on node referencing a View config entity
heyrocker
YO DAWG
webchick
Still don't understand why we make comments fields
webchick
I can't think of anywhere else we put a 'feature' into a field.
Fields are for storing data
heyrocker
I still don't either, I mean the layout explanation makes sense in the abstract
but its feels like square peg / round hole
webchick
Any idea where in the 350 comments there's a discussion about this? I don't mind reading up.
larowlan
this is where it started http://drupal.org/node/1776076#comment-6445480
larowlan
that convo on irc was with heyrocker
webchick
thanks
lol well that's not much to go on.
larowlan
webchick: this was my starting point http://drupal.org/node/1790736
webchick
I have never in my entire life seen that sandbox.
larowlan
when I started (in sep last year) there was no ER
webchick
Right, that makes sense.
larowlan
so contributors so far have been dawehner, andypost, fubhy and a few others
webchick
Right, there are a lot of smart people doing this which is why I'm hoping I'm missing a HUGE cluestuck
larowlan
it was first green in Nov last year but stalled for ~ 2 months until commentNG got in (I had verbal agreement with fago that his would go in first)
heyrocker
right that part i remember
larowlan
tsvenson did a nice write up on it (albeit pointing out the flaws with permissions) http://www.tsvenson.com/blog/2013/02/dont-make-drupal-the-new-microsoft-office
do you think it could be done with ER?
heyrocker
i didnt actually know that comments as field was a real thing
larowlan
it would need hook_entity_view to attach the comments and form to the host
heyrocker
i remember you mentioning it then but i didnt know it went anywhere
this is what happens when i only ever look at cmi issues
larowlan
oh, I worked on it for 2hrs a day for 6 weeks in Sep-Nov  kind of in isolation until it was nearly green when I started asking chx for help on upgrade path, which prompted http://drupal4hu.com/node/342
larowlan
webchick: so with the ER route, we need hook_entity_view to attach view and form to the host or hook_extra_fields which is normally frowned upon, doesn't give views integration etc, ie the driver is no longer the host, with this route, the field is on the host entity and you can get (once we finalise the formatter follow ups) things like 'comment links', 'comment form (w/ formatter settings to control placement etc', 'comment thread (w/ formatter settings to control qty etc)' as formatter options available on display tab, in views etc
larowlan
webchick: not sure I'm making a good case but perhaps talk to dawehner, timplunkettAFK, chx, fubhy, andypost etc for their thoughts too   
larowlan
webchick: from comment 78 is where I came in
larowlan
webchick: one final thing, it is very similar to reply module from d7

Clarification: Whilst I was in 'kind of isolation' I did have help from dawehner with views integration and fubhy with field ui integration before then.

I've only been casually watching this issue, so with webchick's title change I have to reiterate: WAT? Comments as a Field? That doesn't make the slightest bit of sense. At all.

In the log above, heyrocker seems confused by using Views and an ER from Comment to the host entity (node). I'm not sure why, though. The Views Attach module in Drupal 6 and its Drupal 7 successor, EVA, have been doing exactly that for somewhere around 5 years. (I think heyrocker still worked at Palantir when I write Views Attach originally. :-) ) It's actually a really, REALLY powerful way to build related displays. Even most of my college students last term got it. And if you're taking a full SCOTCH/Panels design for your pages (how much of that will end up in contrib I'm not sure, but I'm still assuming that will be the primary layout tool), that's the easiest way to do it.

Even if you didn't want to use Views, EFQ should be up to the task by now. It's not a difficult query at all.

Using Fields here is a symptom of a problem I've pointed out multiple times before: We have a data API, but not metadata API. That means we keep trying to use our data API (Fields) as if it were a metadata API, because we have nothing better. That is sometimes an acceptable hack, but in other cases (such as this, and IMO aliases as well but that's a separate issue) it's a horrible, horrible hack.

Sorry, larowlan, I know you've been working on this a long time but comments-as-a-field is not a viable approach. :-(

Some more background on this and why this is useful
Comments are still entities
Each new comment field = new comment bundle
The comment form (for the comment entity) is rendered as part of the formatter if you select 'visible on node page'
The field tracks the entity comment status (COMMENT_OPEN, COMMENT_CLOSED, COMMENT_HIDDEN)
This value was previously in in {node}.comment which is defined in node_schema (and makes no sense as node does not rely on comment).
So, if you *do* have more than one comment field they are separate fields and separate formatters
For example you could theme the node.tpl to have two cols, for comments on left (using field 1) against on right (field 2)
Or you could have two sets of comments - admin comments (hidden from users) and public comments so one could be public facing comments and another could be internal discussion

Just to reiterate from my conversation with @webchick on irc

We still have comment table for tracking comments.
It contains field_name (bundle), entity_id and entity_type to track the host entity and field.

But we need to track the status of the entity's comments - eg is it open, closed or hidden?

But we don't want to replicate {node}.comment by adding {user}.comment, {custom_block}.comment etc etc.

So we need a new table to keep track of the comment status on an entity (status being open, closed or hidden).

But the keys of this table would be entity_id, entity_type and revision_id.

Which is what we already have in the field api tables.

So we're tasked with creating a table that looks just like a field api table but we need to handle CRUD operations with new code - which would duplicate what we can do with field api.

And then we'd have to work out rendering on the entity view - which field api also does.

And then settings for the render (number per page, show the form on the entity or on its own page) - basically all the comment_{node_type} variables in D7 - but again field api lets us do this with formatter settings.

And so the overlap with field api becomes more and more evident.

That is why this decision was made.

Tremendous thanks to @webchick for asking the hard questions and making me realise why we did it this way again, it was so long ago I was starting to forget!
And to @chx for also pleading my case.

I think there was a misunderstand how and for what Field API is used here. I was a bit confused as well in my first review but I support the architecture here and think it makes sense.

Here is what Field API is used for:
- It gives us a conceptual tool/UI that allows to attach comments to an entity.
- It gives us the possibility to configure display settings per view mode, when to show/hide comments, how many and so on. Yes, maybe most of that will become views but I'm actually not so sure yet if that will happen by default for comments as it's not that trivial and currently has easy to configure options that we'd lose. Want to replace it with a view yourself? No problem at all, just hide the comments field and display a view.
- The field itself is used, as @larowlan explained, to store comment *settings*. Again, it gives us a user interface for it, pluggable storage, is automatically available when the parent entity is loaded *without* having to have a column in the parent entity table as we have right now.

I think that's a ton of functionality that would be rather stupid to duplicate with custom code (the only thing that we could replace with views are the display settings), doesn't really matter if it's just for one field per entity.

What we do *not* use Field API for:
- Storing comments.

Comments stay a separate entity and comments reference to the parent entity, not the other way round. How exactly that reference should happen is a tricky question, but with NG, the entity reference module is only responsible for dynamic, field.module provided fields, a lot of the logic for an entity reference field is in \Drupal\Core and required relationships will define that directly, without entityreference.module dependency. So there's no need for that.

A limitation of the current module and the entity reference item is that it only supports a single entity target type per field (entity properties are also fields). So we can not yet expose that reference as an entityreference field properly, which is a limitation of the new Entity API and something I want to discuss with @fago.

Title:Decouple comment.module from node by turning comments into a fieldDecouple comment.module from node by turning comment settings into a field
Status:Needs work» Needs review
StatusFileSize
new365.79 KB
PASSED: [[SimpleTest]]: [MySQL] 51,788 pass(es).
[ View ]
new3.73 KB

I think Berdir explained rationale about field itself so we need to update issue summary to remove confusion.

I think this title is better.

Removed @todo and remains of comment on node module.

Status:Needs review» Needs work

@webchick:

Can someone point out how and why the decision to turn comments into a field get made? Typically, fields are for storing data.

Bluntly, I would say that applying that logic means that in core the wrong decision was made to make Field UI fields of:

  • File
  • Image
  • Taxonomy term

All of them are nothing but a wrapper for data/entities managed elsewhere.

There are plenty of example of contrib modules that exposes fields for the Field UI to be able to more easy organize the various items by using drag and drop.

From an UX perspective, this is an incredible improvements. For sitebuilders it gives much better overview (all components on the same page to start with) and control directly in the UI, which in turn also makes it easier form themers since they don't have to move things around as much.

This improvement will also mean that a lot of custom coding and other hackish solutions can be avoided. Resulting in stuff that needs to be maintained and makes the sites unnecessary complex to add new things to.

Lastly, as a sitebuilder I don't care much for what is "under the hood", what things are called (entities, bundles, etc) or what the technically correct way of using it is.

The UI is not put in place to match the need of the machine (the code/APIs) its there for humans to be able to get things done by focusing on their real tasks. Unfortunately, in my experience, we keep doing it the wrong way around.

Status:Needs work» Needs review

#1907960-74: Helper issue for "Comment field" shows no performance regression

Issue summary:View changes

Updated issue summary. Added link to upgrade path issue

Issue tags:-Performance

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -277,9 +286,9 @@ public function baseFieldDefinitions() {
-    $properties['nid'] = array(
-      'label' => t('Node ID'),
-      'description' => t('The ID of the node of which this comment is a reply.'),
+    $properties['entity_id'] = array(
+      'label' => t('Entity ID'),
+      'description' => t('The ID of the entity of which this comment is a reply.'),
       'type' => 'entity_reference_field',
       'settings' => array('target_type' => 'node'),

This isn't correct.

The hardcoded node target type might work because there is no validation yet but this will fail as soon as we start to actually use it. And other things will get confused by this as well, EFQ relationships I guess.

Should probably be an int for now + a follow up to make this more dynamic.

It might even make sense to expose entity_type and entity_id as a single property, something like $comment->something->id, $comment->something->type, $comment->something->entity. not sure what something should be entity is a bit strange, parent could be misleading (parent is currently a parent comment). host? Do you have a name for this?

The difference between comments and taxonomy terms, for instance, is directionality. A taxonomy reference (or shortly just entity reference) is a part of the object to which it is attached; it is a reference TO some other object, with some meaning attached to that reference. That is quite different than "a place to shove settings". Comments point TO a node. Nodes do not point TO comments. (Doing so would certainly not scale; this issue is sufficient evidence of that. :-) )

I'm sure this is not the first time someone has tried to (ab)use the Field API for configuration storage. Organic Groups group membership feels wrong to make an intrinsic part of the entity, for instance, but that's how OG works in Drupal 7. That doesn't mean it's right.

Why do I care so much about this? Because one of Drupal's most important strengths in the market right now is its extremely robust data modeling and content assembly tools: That is, Entity API and Views. The sort of data modeling you can do in Drupal is fantastic, especially in Drupal 8, and we need to play that up. However, "configuring comment settings" is not data modeling. By using our data modeling tool as "place to stick stuff", we're actually undermining one of our strongest features.

Comments point TO a node. Nodes do not point TO comments. (Doing so would certainly not scale; this issue is sufficient evidence of that. :-) )

Just to be ultra clear - in no way does this patch make nodes point to comments. In any way. All it does is store whether comments are enabled for an entity (see comment 360). There is no performance regression (see comment 365).

I'm sure this is not the first time someone has tried to (ab)use the Field API for configuration storage. Organic Groups group membership feels wrong to make an intrinsic part of the entity, for instance, but that's how OG works in Drupal 7. That doesn't mean it's right.

The comment status is already a part of the node in HEAD. But in a non-Drupal way. Node module defines this schema field on behalf of comment. That is not the Drupal way. We certainly don't want to replicate that for every other entity type. Those modules do not and should not know of comment module's existance.

Why do I care so much about this? Because one of Drupal's most important strengths in the market right now is its extremely robust data modeling and content assembly tools: That is, Entity API and Views. The sort of data modeling you can do in Drupal is fantastic, especially in Drupal 8, and we need to play that up. However, "configuring comment settings" is not data modeling. By using our data modeling tool as "place to stick stuff", we're actually undermining one of our strongest features.

Please see my comment at 360. If we don't use field api for entity CRUD + entity form integration we have to build something custom. That something custom would like nearly identical to field api as follows.

  • A table with entity type, entity id and revision id keys
  • A method of flagging whether an entity type and bundle needs comment support. Yes we can form alter the node type form but how do we form alter every other entity bundle form to configure comment settings? Does every entity type even have a bundle form?
  • A method of attaching the form additions to the entity form so we can collect the comment status of that entity when it is edited.
  • CRUD code to handle load, submission, insert, update of this value

And to that point, this patch gives you even more flexibility with data modelling and content assembly. See my comment at 359. This opens up a world of additional uses for commenting, some we already have work arounds for in contrib, some we can't begin to imagine.

Please understand we are not storing anything about the comments on the node. In fact node module knows nothing at all about comment module with this patch. Currently this is not the case.

All we are storing is the host entity's comment status. Ie are comments open, closed or hidden.

It is the weekend here but I will pop into irc to see if I can catch you to discuss this further in real time.

StatusFileSize
new338.42 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Just a re-roll of #362

It's my first re-roll from a drupal ladder we are doing here in Barcelona so it may be wrong
The setUp function was removed in core/modules/views/lib/Drupal/views/Tests/ViewExecutableTest.php from HEAD

Status:Needs review» Needs work
StatusFileSize
new382.28 KB
FAILED: [[SimpleTest]]: [MySQL] 52,627 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
new30.29 KB
new6.74 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch cleanup_default_value_0.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

@rodrigoaguilera please use sandbox code and #1907960: Helper issue for "Comment field" to test patches.

The current state of the code in sandbox - patch against 8.x
- added helper to get default value #1907960-76: Helper issue for "Comment field"
- parameters renamed as yched suggested and moved to first level in instance settings #1907960-87: Helper issue for "Comment field"

Status:Needs work» Needs review
StatusFileSize
new374.95 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 731724-comment-decouple-375.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Looked amazing, but I got into some problems here with that last patch::
http://x.s2.simplytest.me/user/1
http://x.s2.simplytest.me/node/1

Error message
InvalidArgumentException: Unable to set a value with a non-numeric delta in a list. in Drupal\Core\Entity\Field\Type\Field->offsetSet() (line 190 of /home/s9a5ef3d22cf0d37/www/core/lib/Drupal/Core/Entity/Field/Type/Field.php).

@crell:

I'm sure this is not the first time someone has tried to (ab)use the Field API for configuration storage.

For Drupal 6, and earlier, the Field UI was a contrib module called Content Construction Kit (CCK). I also remember your excellent Drupal is not a CMS post from last year where you give many examples of things we don't do in code any more.

However, for Drupal to become a real CMF, we must start doing things right. That, for me, means to design the UI from a user perspective and not to match the API's.

The Field UI is a perfect example of where this hasn't been done right. It has the potential to become the holy grail for Drupal. All the basic ingredients are there, only some small adjustments remains to make. All those adjustments are easy to identify by simply applying a proper UX evaluation and strategy.

Manage Fields
This tab contains all the fields and other components/features that are directly needed/relevant for the content object (Content Type, Taxonomy Term, User Account, etc entity type). It also allows me to control the edit layout (will at some stage need a separate tab to allow for different edit layouts depending on device).

Manage Display
In this tab I have the ability to tailor the output of the above fields for various view modes. I have great flexibility to turn on view modes as needed and even create new ones. Thus be able to completely tailor the content display to work perfectly on any number of display needs.

However, for the above to work properly then the sitebuilder needs all the individual components that is used for the content object to be properly exposed and made available on both these tabs. Only then will the full potential of this be possible to use.

Every component that is not properly exposed here will without exception screw things up. See Drupal Limitations and Bottlenecks for some examples of this.

For every component that is not properly exposed in the Field UI, you lose control and add complexity. You need to manually keep in mind and create good documentation about how things work (becomes exponentially worse the more people involved in building and running a site). The worst examples are when that component only have global, site wide, settings. Then you have very limited options to tailor its use for more than one place on the site. The comments are a perfect example of just that!

If this was the case for just a few exceptions it wouldn't be much of a problem. Unfortunately it is more the rule than the exception in Drupal.

What all these exceptions lead to is the need for tons of custom coding and hackish tweaking to fix it on each individual site. It is such a waste of time and valuable resources for everyone involved and makes it virtually impossible to create even an OK user experience for any role affected by it.

Why do I care so much about this? Because one of Drupal's most important strengths in the market right now is its extremely robust data modeling and content assembly tools:

And you have my complete respect about that and everything else you do in the community. However I care about this too, for different reasons.

I care about this because I see so much potential being wasted due to that we are not designing the UI's for the users. We are not using even the most basic UX principles early enough in the process to identify use cases for example.

In your post you write:

No one writes recipe modules anymore. They simply don't exist. The assumption of Drupal today is that you will build your own recipe out of the smaller, more atomic pieces that Drupal provides, and then use those same atomic pieces to also build your news and event content.

I fully agree, but for this to really work we need to embrace this completely. This means realizing that it is only possible to build this content using the exposed configuration options for those atomic pieces.

Especially if we want Drupal to be a CMF and not a CMS.

The current comment functionality is an out-of-the-box solution with very limited configuration options. Decoupling it from content types is the first step to transform it from a CMS feature into a CMF feature.

Step 2 will be to also #1903138: Move global comment permissions to field level.

Then we will have an incredible flexible site user response (comment) feature that can, on the same site, be tailored for each individual use case easily and without any need of custom coding or hackish tweaks.

The tools for us to identify and be able to create this CMS->CMF transformation is to make sure UX is involved from the beginning!

I am also convinced this will be required to allow us to shorten the core release cycles, add real improvements during a cycle and much more.

I'm stuck... with second formatter to render comment links #1907960-91: Helper issue for "Comment field".

Currently core displays a comment list and "Log in or register to post comments" link in full node view when user anonymous so it means that we need to display both formatters on the same view mode?!
If we proceed with current "node-links" kind of displaying links we got a confusion for entities (user, file, term) that have no links under them - I think it's not good idea to introduce this kind of entity elements.

Besides this the current patch has only one issue #367 pointed by Berdir

+++ b/core/modules/comment/lib/Drupal/comment/CommentStorageController.phpundefined
@@ -277,9 +286,9 @@ public function baseFieldDefinitions() {
-    $properties['nid'] = array(
-      'label' => t('Node ID'),
-      'description' => t('The ID of the node of which this comment is a reply.'),
+    $properties['entity_id'] = array(
+      'label' => t('Entity ID'),
+      'description' => t('The ID of the entity of which this comment is a reply.'),
       'type' => 'entity_reference_field',
       'settings' => array('target_type' => 'node'),

I found no way to affect the "settings" key to allow pointing to any entity. So the only solution here is get rid of ER for the "entity_id" column and simple change all it's usage to accordindly
-$comment->entity_id->target_id
+$comment->entity_id->value

I'm sure this is not the first time someone has tried to (ab)use the Field API for configuration storage. Organic Groups group membership feels wrong to make an intrinsic part of the entity, for instance, but that's how OG works in Drupal 7. That doesn't mean it's right.

Why do I care so much about this? Because one of Drupal's most important strengths in the market right now is its extremely robust data modeling and content assembly tools: That is, Entity API and Views. The sort of data modeling you can do in Drupal is fantastic, especially in Drupal 8, and we need to play that up. However, "configuring comment settings" is not data modeling. By using our data modeling tool as "place to stick stuff", we're actually undermining one of our strongest features.

'comments open' / 'comments hidden' / etc. is just as much a metadata property as 'node published' / 'node unpublished' - The only difference being: The comment one is in a field because it's optional. There is not much configuration going on there. It's status tracking. And yes, we don't have a better solution for that yet. So I am 100% for keeping it where it is now.

Using Fields here is a symptom of a problem I've pointed out multiple times before: We have a data API, but not metadata API. That means we keep trying to use our data API (Fields) as if it were a metadata API, because we have nothing better.

I'd say the distinction is between "content" and "metadata about the content". Yes, we right now mix both kind of data up and store all in an one entity. But that's not nothing new, we do that since we have Drupal. E.g., if you think of a node, all that stuff that is in the content-metadata area is stored as an entity field, and so would be the comment settings as soon as #1818556: Convert nodes to the new Entity Field API lands.

One could discuss whether it should be a configurable field, aka field API field, but that doesn't matter from a storage perspective - it's part of the entity in both cases.

The sort of data modeling you can do in Drupal is fantastic, especially in Drupal 8, and we need to play that up. However, "configuring comment settings" is not data modeling. By using our data modeling tool as "place to stick stuff", we're actually undermining one of our strongest features.

Data modelling involves modelling "content metadata" as well. How we model that best could be discussed - indeed, but that's another issue to me as this patch doesn't change it.

Issue summary:View changes

Updated issue summary.

Assigned:dixon_» Unassigned

@Crell, @webchick et al.

I'd like to reiterate on what other people already said here -- there's a big misconception of what this patch does. I've updated the issue summary to make it more clear.

As the maintainer of comment module in core, I boldly support this patch. I've followed it closely and conceptually I think it's the right way to go. And architecturally we're very close, IMHO.

We actually can look at this issue merely as a conversion.

We can not keep random things like $node->comment on an entity any more. That's one of the fundamental goals with the new Entity API as well as the REST functionalities we are building on.
Everything has to become a proper field (or property) handled by the Entity/Field API, everything else I consider a bug.

We can't think about fields as purely meant for "content" any more, at least not until we have a separate "metadata" or "metadata about the content" API . And we won't have that before Drupal 9.

@andypost I'll have a look at the latest patch sometime during today.

@andypost re #378 - could we have a 'show links' setting in the normal comment formatter? So if that was checked you got the form (if configured), thread (if configured) and links from the one formatter?

+++ b/core/modules/comment/comment.views.incundefined
@@ -338,6 +330,61 @@ function comment_views_data() {
+          'extra' => array(
+            array(
+              'field' => 'entity_type',
+              'value' => 'node',
+              'table' => 'comment'
+            ),

Shouldn't this be $type instead of the hardcoded 'node' for value?

Currently sandbox needs some work to merge current HEAD and see #1907960-154: Helper issue for "Comment field"

StatusFileSize
new17.87 KB
new377.93 KB
PASSED: [[SimpleTest]]: [MySQL] 56,436 pass(es).
[ View ]

Ok this should be about ready.

Also xhprof output is attached for viewing a node with 100 comments (mix of anon/admin, mix of pid/no pid):

Before:

Total Incl. Wall Time (microsec): 5,834,274 microsecs
Total Incl. CPU (microsecs): 5,744,359 microsecs
Total Incl. MemUse (bytes): 15,029,036 bytes
Total Incl. PeakMemUse (bytes): 15,960,536 bytes
Number of Function Calls: 469,702

After:

Total Incl. Wall Time (microsec): 5,817,289 microsecs
Total Incl. CPU (microsecs): 5,716,357 microsecs
Total Incl. MemUse (bytes): 13,609,980 bytes
Total Incl. PeakMemUse (bytes): 14,551,396 bytes
Number of Function Calls: 479,456

Status:Needs review» Reviewed & tested by the community

I think all addressed and patch ready to fly

@larowlan: Nice to see there is not only a big UX improvement, particularly for sitebuilders, but also a slight performance boost and a 10% memory saving too.

Awesome work mate.

Wow, that's great. super patch :) Thanks for all your hard works guys.

*optimistic*

StatusFileSize
new378.03 KB
PASSED: [[SimpleTest]]: [MySQL] 55,924 pass(es).
[ View ]

Back to RTBC, re-roll against head (unless bot disagrees).

Just tested that patch on simplytest.me and got a

Fatal error: Call to undefined function field_ui_bundle_admin_path() in /home/sf7d0e7a310a8716/www/core/modules/comment/comment.admin.inc on line 47 at /admin/structure/comments.

Was that the patch or something other?

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

Well. That means that comment_overview_bundles() has inadequate test coverage.

field_ui_bundle_admin_path() was removed in #1982088: Remove hook_entity_bundle_info()'s need for 'real path', use Drupal::entityManager()->getAdminPath().

Issue tags:-Needs tests

+++ b/core/modules/comment/comment.moduleundefined
@@ -209,6 +347,13 @@ function comment_theme() {
+  $items['admin/structure/comments'] = array(

Okay, seems like the patch creates the patch /admin/structure/comments.

Issue tags:+Needs tests

Thanks everyone for all of your hard work, I'm really excited about this change to Drupal. I've always wanted to be able to use comments across entities. Since I don't want to be a show-stopper, so I created a nice little follow up issue for everyone to discuss. :)

#1995944: Remove entity_id and entity_type from the comment table and replace with relationship tables

Thanks again everyone for the hard work!

Status:Needs work» Needs review
StatusFileSize
new381.98 KB
PASSED: [[SimpleTest]]: [MySQL] 56,434 pass(es).
[ View ]

So here's a fixed patch with all suggestions - details see #1907960-170: Helper issue for "Comment field" till 175

Status:Needs review» Reviewed & tested by the community
Issue tags:-Needs tests, -Needs architectural review
StatusFileSize
new392.13 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch decouple-comment-node-731724.406.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

New patch from sandbox #1907960-187: Helper issue for "Comment field"

added tests
addressed all from #1907960-177: Helper issue for "Comment field"

PS: @nevergone new patches are filed to #1907960: Helper issue for "Comment field" and once sandbox ready they pushed here

Contents:

  1. Pull the fresh Drupal 8.x, and apply patch: OK
  2. Install Drupal: OK
  3. Create Article content and threaded comment: OK
  4. Change comment settings: flat mode, disable title and preview: OK
  5. Create Article with closed comment: OK
  6. Create page content: OK
  7. Add comment field for page content type: OK
  8. Comment previous page content: Not OK!
  9. Comment mode is open in content type and content, but comment form is not visible. If edit and save previous content, comment form is visible and working.

  10. Create more page and article content with and without comments: OK

Users:

  1. Create test users: OK
  2. Add comment field for user: OK
  3. Visit user page and comment: Not OK!
  4. Comment mode is open, but comment form is not visible. If edit and save user, comment form is visible and working.

  5. Close comment in user edit form, and visit user page: OK (comment form is not visible)
  6. Re-open comment in user edit form, and visit user page: OK (comment form is visible)
  7. Change comment settings in admin/config/people/accounts/fields, and visit user page: OK
  8. Create more users, with and without comments: OK

Pages