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

You can't get field values to be added to an existing node/user that has been already created without re-editing.
This doesn't happen with any existing fields type either.

Issue summary:View changes

Hopefully made the goal of this patch a little bit clearer.

Status:Reviewed & tested by the community» Needs review
StatusFileSize
new389.67 KB
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 ]

After a set of merge/re-rolls #1907960-211: Helper issue for "Comment field" is green again

StatusFileSize
new389.13 KB
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 ]

An once again

Status:Needs review» Needs work

#418 failed bot, so setting to needs work. Looks like the helper issue has a recent green patch. Is it ready for posting here?

I'm excited about this patch but worried about the usability impact.

@Dries: I did am UX review and usability test of this patch a few months back. In my opinion there is no cause for you to be worried. The usability and UX for sitebuilders will be exponentially improved.

But, it is depended on one important thing and that is that the #1903138: Move global comment permissions to field level follow up issue also makes it. Without moving those permissions to field level the usability will be severely limited.

There's a an visually-hidden element that needs to be swapped in for the element-invisible here.

I tried to do this, but the patch from June 4th really doesn't apply any more.

Status:Needs work» Needs review
StatusFileSize
new390.07 KB
PASSED: [[SimpleTest]]: [MySQL] 57,087 pass(es).
[ View ]

Latest patch from helper issue.

Status:Needs review» Reviewed & tested by the community

Accessibility review #1907960-263: Helper issue for "Comment field" actually there's no changes in markup so we need to get this in to have some days to re-roll and polish new formatter for links #1901110-6: Improve the UX for comment bundle pages and comment field settings and incorporate changes from #1029708: History table for any entity

Assigned:Unassigned» xjm
Status:Reviewed & tested by the community» Needs review

@andypost, you have put way too much work into this patch to RTBC it. :)

Also, I'm about 50% through my review, and there's definitely still some outstanding questions. My notes so far are in #1907960: Helper issue for "Comment field" but I will post a consolidated review here on the main issue when I'm done.

Settinh back to NR, and assigning it to myself to make it clear I'm working on reviewing this still.

Priority:Normal» Critical

I closed #1900132: [Meta] Comment as field favor of this issue. That issue was critical, so I'm going to promote this one. @alexpott and I discussed it today and we agreed that this issue should be release-blocking, as currently it's the only way to handle the node comment settings in CMI, in addition to the other conversions to the entity and field API in this patch.

Issue summary:View changes

types

Title:Decouple comment.module from node by turning comment settings into a fieldConvert comment settings into a field to make them work with CMI and non-node entities
Issue tags:+CMI

we agreed that this issue should be release-blocking, as currently it's the only way to handle the node comment settings in CMI

Tagging and retitling accordingly.

Issue tags:-CMI

Note that calls to theme have been removed from the comment module - see #2008980: Replace theme() with drupal_render() in comment module

I just noticed that #2047777: Helper issue for "Switch from Field-based storage to Entity-based storage" is moving Drupal towards storing fields based on entity rather than fields.

However, this issue introduces the same idea, but in reverse. Comments appear to be stored on the basis of a comment, rather than based on entity. However, It's probably impossible to do this since it would require Drupal to make a new comment entity for every current entity.

I created an issue a while ago to cover this:
#1995944: Remove entity_id and entity_type from the comment table and replace with relationship tables

I think it's a good solution since it allows the comments to be on a "per-entity" basis (like the fields), without having to create a bunch of entities (there would be a single comment entity).

Anyways, just thought I'd mention both of the issues to keep things heading in the same direction. :)

Thanks!

That issue doesn't change anything about the storage (at least not the default storage), it just moves the *responsibility* for storing fields from a separate field sql storage module/API to the entity storage controllers. But it stores them in the same per-field data tables as before. I don't think that's related.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

updated remaining tasks

Issue summary:View changes

Updated issue summary.

Hi All
There are 154 followers on this issue. We really need your help to get this in before the first beta and there is still lots of work to do. Please head over to https://drupal.org/node/1907960 and let us know if you can help. There are lots of simple things that even Novice contributors can help with, we even need help documenting (in a google doc) the remaining issues identified by reviewers. If you want this in core, then the time is now to pitch in, otherwise we'll need to wait another cycle.
Lee

Issue tags:+API change, +CMI

Added to the to-dos' in the issue summary

Issue summary:View changes

Fixed spelling of my name and getting ready for the UX review when time comes...

@larowlan: Saw I was mentioned in the summary for #6 about doing a new UX review when time. I'm fine, eager even, to do so. Thing is just that I at the moment don't have much time to follow things as close as I would like to, nor be on IRC that much.

However, I do read the comments here and also feel free to ping me on Twitter or email when it gets closer so I can set aside some time.

Thanks for the amazing work done with this hugely important sitebuilder UX improvement. There will be many happy puppies when it lands.

nothing to review

Assigned:xjm» Unassigned
Issue tags:-CMI+Needs usability review, +Configuration system, +sprint

Hmm, tag monster added back "Needs usability review." The patch will need usability review, but the workflows need to be documented in the summary first with screenshots.

Issue summary:View changes

Updated issue summary.

Issue tags:+beta blocker

As discussed with @alexpott and @xjm in IRC

Issue tags:-beta blocker

Could more details be provided? IMO this is still completely the wrong approach, so I'm not sure why we are blocking a beta on it.

Issue tags:+beta blocker

Well that wasn't intentional. Tag monster--;

I assume its a beta blocker because it changes data structures but I don't know, @xjm asked me to tag it so she didn't loose her new comment markers.
If there are still objections to this patch can we get a line in the sand either way as it is a huge time suck and to be honest I'd rather be spending my time working on other issues if this one is unlikely to succeed.

I seem to be the only one who thinks this is going to be a UX travesty, so if the other core maintainers want to overrule me, that's fine.

The issue summary doesn't explain why we're not going the route of an entity reference field here though? It just says "berdir says we don't need to." :P

The part that is "make comments work on non-node entities" sounds like a feature to me and i think it destabilizes the release to be adding features right now. This issue might technically be eligible still, but I don't think committing it is smart release management.

"Convert comment setting to CMI" is quite reasonable at this time.

My .02. Feel free to ignore.

bugger, just as I got motivated again

I would test again. Where can I find usable patch?

Issue summary:View changes

Updated issue summary.

Added link to latest patch in the issue summary

Issue summary:View changes

updated summary

Issue summary:View changes

added screenshots

Issue tags:+D8 upgrade path

This isn't just about the comment settings, it's also updating to the new Entity and Field API. Making something more generic in the process of refactoring isn't the same as adding features willy-nilly.

@webchick I can't see any feedback from you on this issue since February, could you explain concretely what exactly the UX travesty is here?

Issue tags:-D8 upgrade path
StatusFileSize
new27.58 KB

Sure.

Let's start by examining the list of field types available to you in the drop-down with this patch:

List of field types

Boolean, Date, Decimal, File, Float, Image, Integer, List*, Long text*, Term reference and Text all make sense as fields. They are things that you may want multiples of on your content. They are all intrinsic data properties of an entity. If you were migrating from Drupal to another system, or another system to Drupal, they're all things that would be in your big-ass CSV file of content.

"Comments" is not any of those things. It's merely a three-way flag that indicates if the node should be commentable, not commentable, or hidden. It only ever makes sense to have one of these per entity. And it opens the door, by way of setting an example in core, of contrib to completely decimate this nice list of field types with other one-off settings, which is going to make content structuring (one of the biggest reasons to use Drupal in the first place) much, much more difficult.

Furthermore, one of the big stated benefits of this patch is that it converts Comments to "first-level entities." That's a good and wonderful thing. But then why are we inventing a unique way to wire this particular entity type up, when in #1847596: Deprecate Taxonomy term reference field in favor of Entity-reference we're actively trying to genericize this relationship building, around the concept of entity relationship fields?

I tried to demonstrate the UX travesty that occurs when adding two "Comments" fields to the same entity, but I'm getting a fatal error: ( ! ) Fatal error: Call to undefined method Drupal\comment\Plugin\field\field_type\CommentItem::getInstance() in /Users/webchick/Sites/8.x/core/modules/comment/lib/Drupal/comment/Plugin/field/field_type/CommentItem.php on line 84 with the latest patch, so can't take the screenshots I'd like to. But I successfully demonstrated this over at #1751210-125: Convert URL alias form element into a field and field widget for path fields. It's basically the same problem.

Issue tags:+D8 upgrade path

Grrr.

Issue tags:-Needs screenshot

screens added to summary

@webchick I think that it fine to have 2 and 3 comment fields on entity. As I mentioned in IRC the example:
product display with 3 comment fields:
1) reviews - plain list of comments
2) discussions - threaded
3) support|sales comments - not public for staff only
Also commenting on user profiles could be used for different purposes.

The UI to allow another kind of commenting is not perfect (field ui needs some love) but this is a common place where we add related properties|fields|behaviours to entity. Probably #1875974: Abstract 'component type' specific code out of EntityDisplay or #1818680: Eliminate hook_field_extra_fields() / Redesign field UI architecture could help but not sure we have enough time to implement because there's still a lot of work in #1346214: [meta] Unified Entity Field API

Proposed ER will not work because comment references to its entity.

So in perfect world we should have a config entity comment form (holds settings: subject, author details, anonymous settings) thats better UX because most of sites have 1 or 2 kinds of comment forms with a set of fields on them. It seems that ER could just point to that comment-form-entity

But after #1497374: Switch from Field-based storage to Entity-based storage things are not changed too much, we just can't reference to the same "comment-form" from different entities and again we spend a week to merge the changes and change storage from field_name to field_id with additional code.

IMHO if the patch was accepted when it was ready and RTBCed in 7-8 months ago me and larowlan could have more time to improve UX and write more tests, also we could get better feedback and do not spend most of time by merging HEAD without any reviews.

Status:Needs review» Postponed
Issue tags:+Needs screenshot

Ok so here's where it stands.
I've spent 12 hours on re-rolling this issue this week since #1497374: Switch from Field-based storage to Entity-based storage went in and changed it so that field names are no longer unique.

If you're one of the 162 people following this issue, and you're a site-builder, UX type or front-ender, now is the time to speak up if you want this functionality or not. Previously on the issue there have been several positive reviews of the changes including the fact that everything is now managed in the one spot. Eg you don't want comments to show, you head to manage display, like you would for any other field.

Now back to the issue at hand.

I'm not prepared to spend any more time on this if there's a chance it is going to be knocked back. I had an undertaking from some of the committers that this issue was a critical blocker for CMI.

If it doesn't go in then

  1. oh well. I mean its annoying but in the process of keeping this back green since November 2012 we've (@andypost and I) been exposed to nearly every core sub-system and as a result can be considered one of the few core generalists. This is a rare commodity and we are certainly the better for it.
  2. We don't have a solution for porting comment settings to CMI. At all. We haven't even thought about it as far as I can tell.
  3. We've significantly delayed the release of Drupal 8. I would estimate that I have spent 100 (part paid but largely personal) hours re-rolling this since November last year. I can't speak for @andypost but anecdotally I'd estimate he too has spent that amount of hours. In addition @xjm and @alexpott have put in close to 20 hours between them reviewing it. This patch was green before any of the following were in core
    • the new routing system
    • Field types as plugins
    • Services in yml files
    • \Drupal

    We've taken the time to keep this patch up to date with the changes.

    Now imagine if this patch had gone in during it's long, cold RTBC period, where it sat at RTBC from Feb 18 (before feature freeze) until May 16 (Yes 3 Months - look back at the comments).

    If it went in during that period. Those 220 hours would have been spent on other criticals and any other follow ups that this issue may have generated. So at most lets say this issue generated 40 hours of follow ups. Thats still 180 hours of time that could have been spent on criticals. Not by new contributors. By some of our best reviewers and two top 20 contributors to Drupal 8. Both @andypost and I have some of the most general knowledge of Drupal core. If you'd have said to any of the initiative leads sometime between February and May "Hey I've got two top 20 Drupal 8 contributors with knowledge of nearly all of core's subsystems who have 90 hours each to give towards your initiative, do you want them?" Do you think they'd have jumped at the opportunity? Of course they would.

If it does get the green-light to go in

  1. Lets do it sooner than later so it gets maximum exposure over what little time we have remaining. With a green-light we can commit to resolving the remaining tasks (see issue summary) in as timely a manner as possible so we can get it in. Getting it in as soon as possible means we get more eyes on it and identify any issues sooner rather than after release. In addition we get the data changes in before beta and ensure we don't have to support HEAD to HEAD updates.
  2. If issues are identified we can commit to making them our priority. I put my hand up to be a co-maintainer of comment (under the assumption this would go in). I'm not about to leave others holding the screaming baby. Putting my hand up to be co-maintainer demonstrates this

Finally, I have to say that I find the tone of the previous comments less than constructive. I respect your opinion and your contributions but I believe there is a nicer way to say it than phrases like completely the wrong approach and UX Travesty. We've spent 12 months on this patch and I think your tone is disrespectful. That said I'm not about to get pissy about it or do the typical tantrum/dummy spit we've come to expect in core development. In fact even if this doesn't go in, I'm still happy to remain in maintainers.txt for comment, even though the main reason I put my hand up was to continue to support this refactor.

I thought long and hard about this response. I typed it and then thought the better of it a couple of times. Then I slept on it. I'm not pissed off about the feedback (just the tone), and if it comes across that I am, then I apologize. I'm pragmatic about it and as with anything in Drupal, discourse and debate are what lead to a better product.

Now thats out of the way, lets get a decision and put this issue to bed either way so we can focus on getting through the other criticals.

Thanks for reading.

You are 100% correct in your criticisms about tone, and I humbly apologize. I will try and review the latest patch in the next few days.

In which I case I need to re-roll after #2074547: Remove ConfigFieldItemInterface::getInstance() which (as seen at comment #449) broke it again.

However, this:

"product display with 3 comment fields:
1) reviews - plain list of comments
2) discussions - threaded
3) support|sales comments - not public for staff only"

and this:

"Proposed ER will not work because comment references to its entity."

Can you explain this a bit more?

Let's pretend we're not doing this in Drupal at all. Let's pretend we are designing a database schema for a custom app in Ruby or whatever.

You'd have a table called "comments". That table would have fields called "entity ID" and "entity type" which would be foreign keys to the "entity" table. Yes? You might also have a "comments_entity_settings" table or whatever that tracks per entity whether things are enabled or not. But you would not stick a "comments" field on your "entity" table, because that's the wrong way about building the relationship.

So then, back in Drupal land, I do not understand why we are not doing this the opposite way here, and having a Comment entity with an Entity Reference field that points to whatever entity/ies it's you want to have comments on. You could still make 3 different types of comment entities: reviews, discussions, and support. Maybe reviews only goes on products, discussions goes on both products and blogs, and support only goes on users. You can configure that at the entity settings level.

This seems a far more natural way to go about it, and I'm still struggling to parse out from either the issue summary or comments in this issue why it's not being done this way, other than "we already spent N months working on it this way." But if "this way" is ultimately not correct, I can't understand why we're forging ahead, regardless of how much effort was put in to this point.

Very happy to hear arguments here on why I am wrong-headed about this. But with what normalization background I have, it just doesn't seem to jive.

As one of those 162 followers that started following around #60 I would like to give a huge +1 for this to get in. I’ve been mostly quietly watching Drupal since the D5 days not having much to say until recently. I maintain several intranet sites for my enterprise and have slowly been converting most of them to Drupal over the last couple years. One of the problems I’ve had with Drupal is the tight coupling of the comments to nodes. Really the way the comment module is right now it should be called Node Comments because they just don’t work with anything else.

One use case we have for this patch is private and public comment streams about form submissions. This is just impossible to do well with D7 because you can only have 1 set of comments attached to a node. Another is attaching comments to non-node entities such as a photo gallery or just a straight file upload like a word document. The closest thing we have is the Reply module. The problem with building this in contrib is you would basically need to recreate the comment module to do it. People tend to not want to do that because, “why waste time doing that when we have a perfectly good comment module in core”. This issue resolves both of those situations.

You can take these comments for what they are worth but coming from my perspective this is a pretty good win for Drupal. Perhaps from an architecture and maintainability perspective it is not and that is ok too.

Moving forward, it sounds like it is time for an executive decision on whether this patch is a go or not. If it is going to go in, let’s make it an actual beta blocker and get it in and start tackling the follow ups. If it is not getting in that’s fine, last time I checked Drupal worked just fine without it, but let’s not push this out any longer.

One last thing, special karma kudos to larowlan and andypost for running the amazing marathon of this patch and I hope you know regardless of how this issue goes your efforts are very much appreciated, at least by me.

In respond to #452. Yes, as a sitebuilder and UX person I want this badly. I do understand concerns some have voiced here, and yes it wont be perfect. However, compared to how comments works now (D7 and before), this is a massive step in the right direction. Then we perfect it for D9 instead.

When I was approached to make my first UX review of this, I was smiling when poking around as I could see all the new possibilities this would open up for.

For me it would be a big disappointment if it's canned. I've already made up use cases for it...

I'm also on standby for performing a new UX review. Just waiting for @larowlan to give me the go ;)

Status:Postponed» Active

Ok so we discussed this (swentel, berdir, webchick and I) on irc.
The plan is
a) We make comment field a non-ui field so it doesn't show up in the 'add new field' area
b) We add an 'edit' option to the 'Comment forms' UI at admin/structure/comments. This has two fields. One is entity type (select) and the other is bundles (checkboxes). You can either add a new comment form or edit an existing one. When you add a new comment form you are asked to select the entity type to which it applies. This fetches (using ajax) the list of bundles for that entity type. You select all those that apply. When you submit this form, the submit handler takes care of creating the new comment fields on the target bundles for you. When you edit an existing comment form you can no longer change the entity type. It is locked. But you can add/remove the bundles. Doing so adds/removes instances of the comment field from the given bundle.

This means we get the best of both worlds. The bulk of the patch remains salvageable but the UX concern around having a 'comments' field available in the list of field types is removed.

I think that distills the conversation down to the salient action points. If I've misrepresented/understood anything, please comment. Thanks.

@larowlan

If I understand the new approach correct, if comments aren't enabled for a bundle, for example a content type, there will be no option from within the edit UI for it to enable the comment fields? I would have to go to the separate comment configuration and "push" the fields from there?

While I do see the point of not having the comments in the add-field list in the Field UI, this is now instead introducing other UX issues. Namely pogo-sticking and the above mentioned push configuration. I'm not fond of either of them.

Status:Active» Needs review
StatusFileSize
new127.37 KB
new420.85 KB
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 ]
new835 bytes

Fixes for #2074547: Remove ConfigFieldItemInterface::getInstance()
Shot of it in action with two fields

StatusFileSize
new220.39 KB

bah chrome screenshot tool busted the screenshot
Try again on PNG
Foo - d83.taco_.png

Status:Needs review» Postponed

FTR, I agree with webchick that piggy backing on Field API for something that is fundamentally configuration is fundamentally wrong in numerous ways. The UI issues it creates are one problem. The other is semantic validity.

Consider: This means that when a node is serialized out via rest.module, it will include the current comment settings. Not comment relationships, the actual comment hidden/open settings. And when writing a node via REST you can/must set that value as well. That's a major WAT that is not going to make any sense to anyone.

Really, what this points to is that we need a parallel system for entity metadata, not just entity data. It may use field plugins for it, but conceptually it has to be a separate data structure even if we sometimes show it in a single admin form. I've been saying that for some time, and this is just another data point for it.

That said, we're certainly not going to be adding such a system in Drupal 8, and, sadly, I cannot think of an alternative way to handle comments. Doing it this way is Wrong(tm), but without another several months of work (that I am not suggesting we do) I don't know what to do instead that wouldn't be even More Wrong(tm).

So if we go this route, be prepared to have to explain this "Holy crap, WTF are you thinking, Drupal you idiot!" situation a few hundred/thousand times. (Because both the UX folks and the REST/web services folks will say exactly that, and they'll be right.)

How is $node->comment handled in REST atm?

@webchick,

I like this thought of what this would look like if we weren't doing this in Drupal...

So let's go to the land of rainbows and unicorns where Drupal doesn't exist...

I think the first thing I would strive for is making the site ACID compliant. I actually already talked about this in #1995944: Remove entity_id and entity_type from the comment table and replace with relationship tables. Basically, instead of having the entity_type and entity_id, there really should be relational tables that build a relation between the target entity and the comment entity. The relation, ought not exist in the comment entity itself (or at least that's how it would look in our land of leprechauns).

Although, the more I think about this, the more I realize that we already have this association type in the Entityreference fields. The relation between a comment and it's target entity can solely exist in those tables.

Then again, a field is no more ACID compliant than the entity itself is. This is outside of this issue, but in my land where lollipops grow on trees, a field would not cross more than one entity. For instance, if I have a text field called
field_subtitle
currently I would end up with a table named
field_data_field_subtitle
which would contain fields like this:

entity_type
bundle
deleted
entity_id
revision_id
language
delta
field_subtitle_value
field_subtitle_format

Of course this is necessary if the same field is being used across entities, but what if we created a table for each entity that it's being used on?
Then we'd end up with a table like this
node_field_subtitle
and
taxonomy_term_field_subtitle
and the structure for these tables would look something like this:
nid
delta
value
format

The reason I bring this up, is because it makes the complex entityreference structure of this:

entity_type
bundle
deleted
entity_id
revision_id
language
delta
field_comment_tid

to this:
nid
tid

(Ideally it should be "cid" and should have a foreign key constraint, but that's beyond the point).
Since Entityreference fields only target a single entity type, they are already well suited for this kind of reference and is nearly identical in nature to my existing issue.

Given that, I suppose I support using Entityreference to reference an entity to it's comments (although, that may not be possible at this stage).

Now... back to reality... I think I'll have to fix the field schema in contrib later like I did the Comment Tree (https://drupal.org/project/comment_tree). But if the relations are already in Entitiyreference fields, all we'll have to do is fix the field schema. :)

Although, it would probably be best to somehow hide the "value" of the Entityreference field from the user (as in, on the node edit page, etc.) so as not to confuse the user and overwhelm the form. Using Entityreference field would expose the relation to the REST API. I do however, like the idea of having more than one comment field on an entity, perhaps it really is an Entityreference field, but the widget is what controls the settings. This way the data is stored in the field like you'd expect, but the actual config (on entity_save) is stored in the CMI. Does that make sense? Field data is entitiyreference, but widget (for that field) is CMI.

Thank you all for your amazing work! I only mean this post in the utmost respect and honor for what all of you do.

peace & love

If $node->comment is not a field now, I think it's just ignored entirely. REST module only works with EntityNG-based entities, and only on first-class properties (ie, Fields and title). "Random extra properties" are explicitly ignored.

@Crell $node->comment is a first-class property. Defined by ..... node module.
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...

So there is no difference to REST in this patch.

Really, what this points to is that we need a parallel system for entity metadata, not just entity data...That said, we're certainly not going to be adding such a system in Drupal 8

We already have such a system: entity types. Can't we make a "Comment Relationship" (or "Comment Status", "Comment Settings", "Comment Binding", or some better name TBD) entity type, following a similar architecture as Flag 3.x? In fact, verifying we can do this pattern in core might help ensure that Flag can be ported cleanly to D8 as well.

Status:Postponed» Needs review

This means that when a node is serialized out via rest.module, it will include the current comment settings. Not comment relationships, the actual comment hidden/open settings. And when writing a node via REST you can/must set that value as well. That's a major WAT that is not going to make any sense to anyone.

How is it any more of a WAT than $node->promote or $node->sticky? That you *can* set those (along with $node->comment) via REST, I don't see as problematic. As far as *must*, in POST we can apply default values, and in PATCH you don't need to set anything you're not wanting to change from its current value. That just leaves PUT, and I don't know what the rules are for applying default values in PUT. But since it's too late to put Flag module into D8 core, and therefore, we're not going to convert $node->promote or $node->sticky to flags, I don't really see making (or per #466, keeping) $node->comment a field as that big a problem, if #467 is deemed too big a change to make to a patch that is otherwise close to ready.

This is interesting and good task. And a lot of work in it. I would be sorry for it if he would not get into the core. Unfortunately for me already too difficult, but I help with the testing with pleasure.

Ok, so before we go any further, lets make sure we've got a consensus about
a) whether this is viable or not - and -
b) if the approach at #458 is the desired approach

Perhaps the better solution, is to have a field that is based off of Entityreference, but has the following differences:

  • The only entity that can be targeted is Comment, so this setting is non existant
  • The widget does not change the field value, just field instance settings
  • The field storage is very similar to Entitityreference, but the "value" is otherwise hidden from the user
  • The field is rendered (display) with a comment create form as well as all of the rendered comments
  • The comments are rendered in hierarchical fashion like they are now

Although it would be nice to be able to access the comments from the target enttity via REST (like this solution would). I'm not sure that is completely necessary. If you loaded a node for instance, you could always just make a second request to the "comment" endpoint to get the comments for the node. It's not as sexy as just accessing them at the node endpoint, but the comments are not unavaible, or even unwritable in it's current state.

AH!

Now I see the problem. If we went with my own solution, there would be no way to set the comment settings (open, closed, etc.) on an individual node like you can now.

So really, what fixes the problem (at least in my mind) is having two fields (FIELD_NAME_form and FIELD_NAME_thread), but they kind of act like one.

When you create a "form" field, it would also create a "thread" field. The only one that shows up on the Manage Fields page is the "form" field (since it's the only one that has a widget). The thread field would be an entity reference field (with no widget), I can't think of what settings the user would need to change on it, so it could be hidden from view on the manage fields page. When a "form" field is created, a "thread" field is created, when the "form" field is deleted, then the "thread" field is deleted.

It would also be nice if the fields were separated on the Display tab. This would allow someone to have different display setting for the form and the thread. For instance, if I wanted the most recent comment to show up on the teaser, but without the form, I could just display the "thread" field and just set 1 in the number to show in the display settings and then hide the "form" field. Or, if I wanted the form to be beneath the comments instead of above them, I could easily move them around.

I think doing it this way would fix a lot of the UX issues, as well as bring some added flexibility on display. Doing it this way would also expose not only the comment settings to REST but also the comments themselves (via the "thread" field, which is just an entity reference field).

Probably way too much work at this point, but if it solves everyone's issues, it might be worth a shot.

peace & love

If this patch doesn't get in, I will be a very sad panda. :-( I've been following it from the beginning, and having comments on any entity and multiple types of comments on a single entity is something that would be super useful. Of course, using something like the Reply module works (and it's what I've done in Drupal 7), the main difference is that EVERYTHING in contrib integrates with comments. When I'm using the Reply module - I can't take advantage of most of it.

While I understand @Crell's point about how Drupal should have an entity metadata system - we're not going to get one for Drupal 8. I think this patch (although, imperfect architecturally) is no worse that what's already in - in fact, I'd say it's a an improvement. ;-)

Just my 2 cents! I'll be waiting anxiously to see what call gets made.

Until we have an 'entity metadata API' which can handle things like comment settings, sticky/promoted etc., then I personally think having a normal field with the settings in Field UI is fine. It's not ideal, it's not worse than comment settings being hacked in with form_alters and hard-coded database schema. It's a lot cleaner than what we have now with a comment property hardcoded into the {node} table even when comment module is disabled. Migrating to another entity isn't going to be a big move if that happens later.

Creating an entirely one-off custom interface, which behind the scenes creates fields, is going to be more confusing to people in the long run. For example I'd want to be able to move comment settings around on the form display, but if it's hidden from the UI that weight is going to be hard-coded somewhere and I can't get to it. Or will it only be hidden in the 'manage fields' tab but visible in the others?

I think it's fine if the "thread" field shows up as well. I guess it would be best if it showed up in "Manage Fields", but it didn't show up in "Manage Form Display" (because, there is no widget). But it should show up on any "Manage Display" (or anywhere else the settings can be changed).

I think we'd need to figure out the best way to display both of the fields without confusing the user.I think the best way to do that is just remove it from the "Manage Form Display" settings (or at least give it some other interaction, like making it greyed out or something) and leave it everywhere else.

I agree with catch.

I also think this is a huge improvement to the crusty old node-centric comments code we currently have.
We can either commit it, or kick it to contrib where it is going to become yet another "essential contrib", which I don't think is in our interest.

The fact that someone has been spending all their time rerolling an RTBC patch since february is already a huge failure of our process.
Let's not make it even bigger.

I lot of folks are asserting that this patch is really important and useful. I think we all agree on that, so need to pile on.

What I have suggested is that this patch has too much likelihood of spawning follow-up issues, especially criticals. In other words, the release is delayed. At some point, you have to make unpleasant decisions like this. It is possible that riskiness is not a topic that can be productively discussed in an issue. I don't know. If so, we can just proceed toward RTBC and let the committers decide.

There's no known critical follow-ups as a result of this issue at the moment.

It does resolve a critical issue though - converting comment settings to CMI. There's no alternative approach for that anywhere.

If we commit this and there's 5 critical bugs as a result next week, we can still roll it back again until those are resolved or reconsider.

On the other hand I can think of recently committed (and less recently committed), self-contained, completely new features with open critical bugs against them where this argument could be applied to.

@moshe weitzman

I agree, there are risks with this patch. It will spawn follow ups and in all likelihood (or maybe likelihook ;)) delay the release of Drupal 8.

However, I think it would be wrong to just see the delay as an unwanted thing and use that as a big reason to dismiss it.

There is also a great deal of opportunities to factor in. The ability to add comments to any entity is not a small change. It is more than a major change really. As a result Drupal 8 will become much better than without it. The use cases for comments on any entity are as many as the different entities itself. Many more actually!

Also, the follow ups will certainly not just be useful for making this work. It is quite safe to assume they will also make many other things work better too.

Lastly, keeping comments coupled to only node entities means we have to live with it until Drupal 9, whenever that is. Would that wait make up for being able to release Drupal 8 without this delay risk?

It should be a field, and for now, I think we should re-use the existing Field UI.

This issue exposes the brokenness of the Field UI, which is why I believe people are concerned about the impact of this patch. At the end of the day, that brokenness shouldn't hold back good progress. I believe the field UI should be re-architected, possibly into a 3-step process: (i) build the data model, (ii) build the form to input that data and (iii) build your different output formats. In a world where the Field UI was easier to use, it would make sense for comment settings to be a field.

So does this mean we're back to the approach outlined in #458 or that it stays as the patch currently stands - ie comments is presented as an option in the field ui 'add new field' area? Note there is still work to do to address the reviews in the helper issue as detailed in the issue summary. The helper issue is #1907960: Helper issue for "Comment field"

I read #474 plus #482 as overruling #458, and instead suggesting the Comment field be a regular field, as far as Field UI is concerned, as per the existing patch.

Everyone is right. Including all arguments and counter-arguments.

One year ago, I strongly encouraged @larowlan to work on this issue. Facing a continued reality of privileged Comment module options in a central core entity concept, which provides no API for contributed modules to achieve the same "out of the box" experience, and facing a vastly improved Entity/Field API and configuration system, it appeared about time to get rid of this special case and replace it with an implementation whose architecture can be easily resembled by contributed modules. @larowlan agreed from the beginning, invested plenty of time, and mostly owned this effort from there on.

The most sensible solution appeared to be KISS: Field API did actually offer an architecture that fully meets the technical requirements. — Working through the Mambo-Jambo of stone-age Comment module has always been the actual, cumbersome, borderline insane part of the task. The ultimate "field" related implementation actually was pretty clear in everyone's head right from the start. But to be clear: The counter-arguments are absolutely valid from multiple perspectives. They were raised in their current form one year ago already.

But since the beginning, those did not present a barrier. In fact, one of the early discussed pathways was to push this change into core, and later re-evaluate its conceptual fit with Field API (-not- its architecture). Even that option appeared to be sensible back then (and today).

Fast-forward to today, there do not appear to be any blocking concerns with the implementation and its architectural design and integration. It may be hard to see it, but what you have is 1) a working architecture/subsystem and 2) a working implementation. Architecturally, things are fine. You only have 3) a design/intention conflict.

You are able to resolve the conflict by duplicating the existing and well-working architecture into a new and separate subsystem. You have the unique benefit of having the architectural solution in the first place.

This means to literally copy /core/modules/field into /core/modules/data (or /banana). As others have noted already, there are some minor differences in APIs, business logic, and also user interface aspects. Instead of trying to bend both to a common denominator, you intentionally give both the freedom to evolve on their own. But ultimately, differences are small, everything is prepared and works already, so besides copying and renaming, there is little to do. Smart folks will notice the identical pieces, but abstracting them further can happily be D9 material. Refactoring this changeset to tie into that API instead of Field API is most likely piece of cake. Risks are kept minimal, since the architecture exists and comes with a swath of tests already.

In short, you literally have all the tools you need. Somewhat coming from an outsider perspective, for some reason, thoughts and options appear to be constrained to a bounding box that factually does not exist.

You can use existing intelligence and foster evolution.

I know this is about core, but I never thought that having to add a field collection this way was weird. Isn't this sort of the same thing? Perhaps what we need is an "add reference to entity"-field, where we could add refs to users, other entities, taxonomy, comments and contribs like field collection?

@odegard

Are you referring to Entity reference (https://drupal.org/project/entityreference)?
If so, the module is going to be included in Drupal 8.

Apologies if I've misunderstood something, but it seems part of the criticism of this patch is that you use the add field-select list to add the comment? Well, field group (contrib) add a new list, why not add one more for reference-type-fields of which comment is one of them? This way we could separate traditional fields like text/list/number etc. from the reference type fields like comments, term ref, user ref etc. Contrib modules that offer some kind of reference can then choose to populate this list and not the FIELD-field list (text/list/etc).

it seems part of the criticism of this patch is that you use the add field-select list to add the comment...why not add one more for reference-type-fields of which comment is one of them?

Entity reference fields (both the ones provided by entity_reference module already in core in D8, and the still semi-custom ones of term refs, files, and images) follow the regular Field model where the data stored by that field is the id of the referenced entity. In that way, it is still regular content data, just like text, date, etc. fields. What's different about this issue is that the data being stored by the field is *not* references to the comments, but just the status of whether commenting is open, closed, or hidden. This status is what you edit when you edit the node (or whatever other entity type you want to enable comments for), and when you view the node, the field formatter "renders" this status as the actual comment thread (if the status isn't "hidden") by querying comment entities that reference back to this node. So the field is not a reference field, it's a status field, and some have argued here that that should be treated as metadata/configuration/whatever, but not as content, and that we shouldn't be using Field UI for adding non-content data. Personally, I'm not yet convinced that Field UI needs to be separated on a "content" / "metadata" distinction. In practice, I've seen many sites mix those two things up anyway, and add checkbox and list fields to their nodes that are more metadata'ish than content'ish, and I'm not aware of any of those site builders having cared about the distinction. Perhaps the distinction matters to content authors working with the editing form, but perhaps that's where #482, #483, contrib solutions like Display Suite, etc., can help site builders create intuitive forms for content authors to use.

Just my 2 cents for this issue.

Could we separate the comment setting from the node type form completely and store them on the comment type?

The normal workflow would be:
1. Add Content Type
2. Go to a generic 'Comment Settings Page'
3. Add a new Comment Type and set it to be attached to the right entity type and bundle.
4. Set the comment settings such as threading etc.

To make it easier for Content Administrators we could then add a "short cut" way of doing it that adds something to the Content Type form.

This is similar to how the menu links system works in Drupal 7.

Since it's a field, wouldn't it make more sense for display settings (threading, etc.) to be on the display settings of the entity (node)? Or I suppose you could just create a new type for ever display settings you want, but that would then prevent you from having different display settings on different entity displays. For instance, what if I want the comments flat on node teaser, but I want them threaded on node full? Your proposal would mean that i would have to have two separate comment threads to accomplish that goal, which wouldn't work at all.

peace & love

StatusFileSize
new422.51 KB
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 ]

Current sandbox to allow easily test with http://simplytest.me
We are testing patches now om helper for helper #2079093-49: Patch testing issue for Comment-Field

I have done some review in #2079093-64: Patch testing issue for Comment-Field. I am adding tags here to get reviews by all the maintainer of subsystems so that we can review an discuss these changes asap. Because this patch is doing a lot of things with subsystems and it will speed up the reviewing process.

> We already have such a system: entity types. Can't we make a "Comment Relationship" (or "Comment Status", "Comment Settings", "Comment Binding", or some better name TBD) entity type, following a similar architecture as Flag 3.x? In fact, verifying we can do this pattern in core might help ensure that Flag can be ported cleanly to D8 as well.

I don't follow all of that comment, but here's some info on Flag 3.x that may be of relevance.

On D7, Flags are exportable configuration. A Flag, say 'bookmarks' says:

- I am a node flag
- I can be applied to these node bundles
- here are some UI strings that I use
- this is the UI style I use

The user creates this flag in the flag admin UI, and then everything else magically appears on nodes, node forms, etc.

(Note: it's important to distinguish the Flag, which is the configuration, from the Flagging. A flagging is a content entity which says 'User Alice flagged node 1 with the 'bookmarks' flag last Tuesday'.)

My work so far on porting this to D8 would have the Flag be a (config entity + plugin) pair, where the different plugins cater for special behaviours for the entity type being flagged: nodes/comments/users. Thus:

- my flag plugin is 'node'
- I can be applied to these node bundles
- here are some UI strings that I use
- this is the UI style I use (which would be a plugin too, but that's by the by)

It's been suggested to me that an alternative way of doing Flag on D8 would be having the Flag configuration be somewhat like a DS field.

So the user would instead look at their entity field or display settings, and have a way of adding a Flag to this node bundle.

I have at the moment no idea how that would work :)

Also, some UX issues this would raise:

- what if the user wants the same flag (ie same UI options, same strings) on several node bundles?
- what if they want it on ALL node bundles, *automatically*?
- does this get added at the manage field or manage display page? It's a thing that goes (mostly) on the display, but display feels a bit weak for it if that's where you're defining the behaviour for something that stores data.

Issue summary:View changes

new field settings screenshot

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Updated issue

Issue summary:View changes

Updated issue summary

Status:Needs work» Needs review

All of the review items from this issue and #1907960: Helper issue for "Comment field" except the changes to CommentUserTest have been rectified, lastest patch is at #2079093: Patch testing issue for Comment-Field

We have 4 fails because of shortcomings in entity-render cache in HEAD.

Keep the reviews coming - lets get this in during Drupalcon!

Issue summary:View changes

Updated issue summary.

Latest patch is passing on #2079093: Patch testing issue for Comment-Field
All reviews from #1907960: Helper issue for "Comment field" addressed (so closed that).
Keep the reviews coming
Updated issue summary with remaining to-dos.

Issue summary:View changes

Updated issue summary.

Issue summary:View changes

Added more follow ups/related issues

Issue summary:View changes

Added last todo issue

Issue summary:View changes

Updated UX review bit

In case #486 was not clear (possibly due to length), I'd like to clarify:

  1. Disregarding the concrete use-case: Architecturally, this concept is badly needed in Drupal.

  2. This concrete sample implementation will be copied + resembled by 1,000+ contributed and custom modules.

  3. All major concerns and objections that have been raised are 100% valid. It would be a huge mistake to ignore them.

  4. Some of the concerns are about fundamental concepts (content vs. configuration-ish meta data).

    The current/previous implementation is only acceptable, because the meta data is, in fact, not part of the actual content entity that is being processed in various ways. Instead, the additional meta data is pulled in and used where needed and where appropriate only.

  5. The proposed change turns the formerly separate meta data into data of the content itself — implicitly participating in all possible content entity interactions, even though the data is not content.

  6. We're seeing this anti-pattern and inappropriate (ab)use of architecture in plenty of contributed modules already.

    A prime example might be the Media module (adding internal meta data fields to all entity types/bundles that it is operating with). That inappropriate usage is the topmost reason for why a proper architectural solution in Drupal core is required.

  7. Ultimately, the conflict stems from the fact that we happen to have an architecture in core that happens to fully resolve the needs of affected modules.

    From a technical standpoint, it delivers a 99% solution. But from a conceptual and business logic perspective, the solution is 100% incompatible and inappropriate to use.

  8. Business logic matters.

  9. The good news: All of the code exists already. Including tests.

  10. Consequently, what you want to do is this:

    $ cp -R core/modules/field core/modules/metadata
    $ find core/modules/metadata -type f -exec sed -i "s/field_/metadata_/g;s/Field/Metadata/g" '{}' ';'

    In words: Literally copy the whole shebang. Apparently, it solves plenty of hard problems for contrib. You have the solution right in your hands, you can provide it.

    You want to diverge from the Field API mainline, because of the very valid concerns that have been raised with regard to differences in business logic. These fields != fields, but yet, == fields.

    You can further consider to simplify the Meta Data API afterwards; e.g., by restricting storage to SQL (debatable; separate discussion), and/or possibly removing any other complexities of Field API that are irrelevant (e.g., multiple values).

  11. Regardless of these major conceptual and architectural concerns, I do support this change. It is required, and it attacks one of the most critical bottlenecks in Drupal's architecture.

    Unless core provides a proper solution, the world will continue to abuse existing APIs in utterly inappropriate ways, further disrupting a clear and common understanding of what exactly "content" constitutes.

    Drupal has to have a solid architectural concept for this data. Core consumers badly need a solution, yesterday.

@sun: It would be great if you could please let the Media module maintainers know more about what you mean by the following:

A prime example might be the Media module (adding internal meta data fields to all entity types/bundles that it is operating with). That inappropriate usage is the topmost reason for why a proper architectural solution in Drupal core is required.

It does not have to be in this issue, in fact I would encourage you to post something in https://drupal.org/project/issues/media. I'd like to know what you mean by this and why I haven't heard from you about this before.

Issue summary:View changes

Updated Api changes

StatusFileSize
new449.85 KB
PASSED: [[SimpleTest]]: [MySQL] 59,215 pass(es).
[ View ]

Latest patch from helper issue (green).
Only final UX review left in remaining tasks.

StatusFileSize
new449.77 KB
FAILED: [[SimpleTest]]: [MySQL] 59,033 pass(es), 2 fail(s), and 1 exception(s).
[ View ]

Final patch

Issue summary:View changes

Commented out manual upgrade path

Edit: See #511

@dixon So the priority of that UX issue will become critical and a bug report?

StatusFileSize
new5.06 KB

UX Follow ups:
#2099421: Add an administrative description for a comment field
Move entity-type from title to its own column
Add hook_help() for the admin/structure/comments page

StatusFileSize
new3.99 KB
new450.54 KB
PASSED: [[SimpleTest]]: [MySQL] 59,272 pass(es).
[ View ]

merged HEAD and added suggested "HumanName" for field ui overview screens

Status:Needs review» Reviewed & tested by the community

So, I've spend the last couple of days at DrupalCon walking through and reviewing this patch in as much detail as I can. See #2079093: Patch testing issue for Comment-Field for detailed reviews.

The general take-aways are:

The time has come (assuming patch comes back green) ;)

@Bojhan: Yes, the UX follow-up should be marked critical when this gets in.

Green! UX does not breaks things!

Title:Convert comment settings into a field to make them work with CMI and non-node entitiesChange notice: Convert comment settings into a field to make them work with CMI and non-node entities
Priority:Critical» Major
Status:Reviewed & tested by the community» Active
Issue tags:-Avoid commit conflicts

Committed 37949fe and pushed to 8.x. Thanks!

HOLY SHIIIIIIT! AWESOME!

Thanks larowland, andypost!!!

Title:Change notice: Convert comment settings into a field to make them work with CMI and non-node entitiesConvert comment settings into a field to make them work with CMI and non-node entities
Priority:Major» Critical
Status:Active» Reviewed & tested by the community

Please see suggested commit message in the issue summary, to not forget people that worked in helper issues.

Title:Convert comment settings into a field to make them work with CMI and non-node entitiesChange notice: Convert comment settings into a field to make them work with CMI and non-node entities
Priority:Critical» Major
Status:Reviewed & tested by the community» Active

Cross post :)

Thank you very much for all the work! You are great! :)

I got this.

The following updates returned messages
comment module
Update #8006
    Failed: Drupal\Core\Database\DatabaseExceptionWrapper: SQLSTATE[42S02]: Base table or view not found: 1146 Table 'drupal_d8.node_type' doesn't exist: SELECT * FROM {node_type}; Array ( ) in Drupal\Core\Database\Connection->query() (line 568 of /Users/clemens/Sites/drupal/d8/www/core/lib/Drupal/Core/Database/Connection.php).

Woohoo! Congrats to everyone who worked on this!

Title:Change notice: Convert comment settings into a field to make them work with CMI and non-node entities[HEAD BROKEN] Change notice: Convert comment settings into a field to make them work with CMI and non-node entities
Priority:Major» Critical

I haven't been able to track down exactly *what* is causing the failures, but from a timing perspective, it appears that the patch committed before this one tested successfully. I retested before posting, so these errors are reproducible.

Found in the apache error logs, with many duplicates removed (may or may not be related to this specific break, but occured during this test):

Uncaught PHP Exception Symfony\\Component\\HttpKernel\\Exception\\HttpException: "Internal Server Error" at /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/lib/Drupal/system/Controller/FormAjaxController.php line 49
PHP Fatal error:  Call to undefined method Symfony\\Component\\HttpKernel\\Event\\FilterResponseEvent::getTargetUrl() in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/system/tests/modules/session_test/lib/Drupal/session_test/EventSubscriber/SessionTestSubscriber.php on line 52
PHP Fatal error:  Cannot inherit previously-inherited or override constant FIELD_LOAD_CURRENT from interface Drupal\\user\\UserStorageControllerInterface in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/user/lib/Drupal/user/UserStorageController.php on line 25
Uncaught PHP Exception Symfony\\Component\\Routing\\Exception\\RouteNotFoundException: "Route "front" does not exist." at /var/lib/drupaltestbot/sites/default/files/checkout/core/lib/Drupal/Core/Routing/RouteProvider.php line 127
PHP Fatal error:  Call to a member function hasPath() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/views_ui/lib/Drupal/views_ui/ViewListController.php on line 274

Notes:
- I saw the FormAjaxController errors while troubleshooting https://qa.drupal.org/pifr/test/636563 at 5:30 am, so they are probably a red herring in this particular failure.
- The FIELD_LOAD_CURRENT fatal appears all over the apache error log.

qa.d.o review log Error results:

Fatal error: Call to a member function id() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/comment/lib/Drupal/comment/Tests/Views/CommentTestBase.php on line 49
FATAL Drupal\comment\Tests\Views\ArgumentUserUIDTest: test runner returned a non-zero error code (255).
Fatal error: Call to a member function id() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/comment/lib/Drupal/comment/Tests/Views/CommentTestBase.php on line 49
FATAL Drupal\comment\Tests\Views\CommentRowTest: test runner returned a non-zero error code (255).
Fatal error: Call to a member function id() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/comment/lib/Drupal/comment/Tests/Views/CommentTestBase.php on line 49
FATAL Drupal\comment\Tests\Views\FilterUserUIDTest: test runner returned a non-zero error code (255).
Fatal error: Call to a member function id() on a non-object in /var/lib/drupaltestbot/sites/default/files/checkout/core/modules/comment/lib/Drupal/comment/Tests/Views/CommentTestBase.php on line 49
FATAL Drupal\comment\Tests\Views\RowRssTest: test runner returned a non-zero error code (255).

Failed Test suites:

ExpandDrupal\comment\Tests\Views\ArgumentUserUIDTest 5 3 1
ExpandDrupal\comment\Tests\Views\CommentRowTest 5 3 1
ExpandDrupal\comment\Tests\Views\FilterUserUIDTest 5 3 1
ExpandDrupal\comment\Tests\Views\RowRssTest 5 3 1
ExpandDrupal\contact\Tests\Views\ContactLinkTest 41 6 1
ExpandDrupal\content_translation\Tests\Views\TranslationLinkTest 17 3 2
ExpandDrupal\field\Tests\Views\FieldUITest 14 15 3
ExpandDrupal\history\Tests\Views\HistoryTimestampTest 9 2 1
ExpandDrupal\node\Tests\Views\BulkFormTest 34 5 1
ExpandDrupal\node\Tests\Views\StatusExtraTest 57 5 1
ExpandDrupal\standard\Tests\StandardTest 30 5 3
ExpandDrupal\statistics\Tests\Views\IntegrationTest 13 2 1
ExpandDrupal\system\Tests\System\AccessDeniedTest 58 4 1
ExpandDrupal\system\Tests\System\PageNotFoundTest 16 1 0
ExpandDrupal\user\Tests\Views\ArgumentDefaultTest 6 2 1
ExpandDrupal\user\Tests\Views\BulkFormTest 30 4 2
ExpandDrupal\views\Tests\Handler\FilterDateTest 16 6 1
ExpandDrupal\views\Tests\Plugin\DisplayFeedTest 41 2 1
CollapsedDrupal\views\Tests\Plugin\PagerTest 71 35 1

Typical failures:

simplexml_import_dom(): Invalid Nodetype to import
simplexml_import_dom(Object)
Drupal\simpletest\WebTestBase->parse()
Drupal\simpletest\WebTestBase->drupalPostForm('user', Array, 'Log in')
Drupal\simpletest\WebTestBase->drupalLogin(Object)
Drupal\comment\Tests\Views\CommentTestBase->setUp()
Drupal\simpletest\TestBase->run()
simpletest_script_run_one_test('801', 'Drupal\comment\Tests\Views\ArgumentUserUIDTest')

To clarify, is the update broken from Head to Head or from Drupal 7 to Head?

(are we at the stage where HEAD to HEAD updates are guaranteed too or is that after beta?)

@clemens.tolboom This error is because there in no more node_type table in D8.
@clemens.tolboom and @nbz HEAD to HEAD update is not supported yet it will be supported after beta.

@nbz: It's not an upgrade break ... it means a fresh pull of HEAD currently will not pass testing.

DO NOT ROLL THIS BACK. I am ssh'd into the bot, reproduced the fail, will fix in minutes.

Status:Active» Needs review
StatusFileSize
new617 bytes
PASSED: [[SimpleTest]]: [MySQL] 59,054 pass(es).
[ View ]

Fix suggested by berdir, verified on bot! http://privatepaste.com/b576f959e0

Status:Needs review» Reviewed & tested by the community

Title:[HEAD BROKEN] Change notice: Convert comment settings into a field to make them work with CMI and non-node entitiesChange notice: Convert comment settings into a field to make them work with CMI and non-node entities
Status:Reviewed & tested by the community» Active

Thanks, committed! Back to need change notices!

Priority:Critical» Major

And resetting Priority.

Priority:Major» Critical
Status:Active» Needs work

We still need a change notice

Priority:Critical» Major
Status:Needs work» Active

Resetting to #514

Status:Active» Needs review

Draft change notice: https://drupal.org/node/2100015

Title:Change notice: Convert comment settings into a field to make them work with CMI and non-node entitiesConvert comment settings into a field to make them work with CMI and non-node entities
Priority:Major» Critical
Status:Needs review» Fixed

Looks good to me, very detailed :)

F. I. X. E. D. :)

F. I. X. E. D. :-)

Finally, Intergalactic Xylophone-Enhanced Drupal? Yes, I agree. :-)

Excellent change notice - one of the easiest to understand and most detailed I've seen. Once again, congrats to everyone who helped get this in!

Sorry for the noise .. bash2head is done :p

The patch is awesome :)

Hi everyone, I'm following this issue from when the title was more simple "Decouple comment.module from node". It was a time when I lost myself on your work on this issue, but I think that this is one of the most improvements that drupal 8 will have. I only can put my hat off and congratulate all of you for this very hard and complex work. :)) Thanks a lot!

Issue tags:-sprint

Removing tag.

Yay! Great job everybody ;)

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

Issue summary:View changes

Updated issue summary with suggested commit message

This introduced an interesting new bug when using the "multiple comment fields on an entity" functionality: #2155387: Multiple comment fields per entity are supported, but break horribly when they need pagers.

Pages