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

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

// 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
$mode = variable_get('comment_default_mode_' . $node->type, COMMENT_MODE_THREADED);
D8
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
  if ($node->comment == COMMENT_NODE_OPEN) {
    // Do something.
  }
D8
  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
$comment_count = $node->comment_count;
D8
$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.

CommentFileSizeAuthor
#526 731724_526.patch617 byteschx
#510 drupal8.comment-module.731724-510.patch450.54 KBandypost
#510 interdiff.txt3.99 KBandypost
#509 interdiff.txt5.06 KBlarowlan
#505 731724-comment-field-505.patch449.77 KBandypost
#503 comment-mcfield.patch449.85 KBlarowlan
#493 drupal8.comment-module.731724-493.patch422.51 KBandypost
#461 Foo - d83.taco_.png220.39 KBlarowlan
#460 interdiff.txt835 byteslarowlan
#460 comment-field.reroll.459.patch420.85 KBlarowlan
#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
#418 decouple-comment-node-731724.418.patch389.13 KBandypost
#414 sandbox-merge-210.patch389.67 KBandypost
#406 decouple-comment-node-731724.406.patch392.13 KBandypost
#404 decouple-comment-node-731724.404.patch381.98 KBandypost
#398 decouple-comment-node-731724.398.patch378.03 KBlarowlan
#392 decouple-comment-node-731724.helper.169.patch377.93 KBlarowlan
#392 xhprof-output.zip17.87 KBlarowlan
#375 731724-comment-decouple-375.patch374.95 KBandypost
#372 cleanup_default_value.patch6.74 KBandypost
#372 rename-nested-interdiff.txt30.29 KBandypost
#372 731724-comment-decouple-372.patch382.28 KBandypost
#370 731724-comment-decouple-371.patch338.42 KBrodrigoaguilera
#362 interdiff-362.txt3.73 KBandypost
#362 731724-comment-decouple-362.patch365.79 KBandypost
#353 sandbox-70pre.txt3.59 KBlarowlan
#353 sandbox-70pre.patch374.75 KBlarowlan
#337 0001-Field-storage-column-changed-from-comment-to-status.patch36.42 KBandypost
#337 0002-Fixed-all-tests-after-conversion-except-FileFieldWid.patch19.78 KBandypost
#337 0003-Fix-access-to-files-and-clean-up-remains-of-ER-trans.patch9.41 KBandypost
#337 731724-comment-decouple-337.patch362.88 KBandypost
#335 731724-interdiff-335.txt34.52 KBandypost
#335 731724-comment-decouple-335.patch367.56 KBandypost
#326 731724-comment-decouple-326.patch370.04 KBdixon_
#326 731724-comment-decouple-326.interdiff.txt7.28 KBdixon_
#319 731724-comment-decouple-319.patch369.08 KBandypost
#311 731724-comment-decouple-311.patch363.73 KBdixon_
#308 731724-comment_field-307.patch365.01 KBandypost
#308 731724-interdiff-307.txt1.25 KBandypost
#305 731724-comment_field-305.patch364.34 KBandypost
#305 731724-interdiff-305.txt15.71 KBandypost
#303 731724-interdiff-303.txt41.26 KBandypost
#303 731724-comment_field-303.patch359.59 KBandypost
#297 731724-interdiff-295.txt1.44 KBandypost
#297 decouple-comment-node-731724.295.patch359.48 KBandypost
#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
#287 731724-interdiff-287.txt2.55 KBandypost
#287 decouple-comment-node-731724.287.patch359.85 KBandypost
#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
#280 decouple-comment-node-731724.280.interdiff.txt1.72 KBlarowlan
#280 decouple-comment-node-731724.280.patch1.26 MBlarowlan
#278 731724-interdiff-278.txt23.85 KBandypost
#278 decouple-comment-node-731724.278.patch358.74 KBandypost
#275 decouple-comment-node-731724.275.patch369.54 KBandypost
#273 decouple-comment-node-731724.273.patch353.91 KBandypost
#271 decouple-comment-node-731724.271.interdiff.txt170.81 KBlarowlan
#271 decouple-comment-node-731724.271.patch355.25 KBlarowlan
#263 efq-comment-ui-test-do-not-test.patch1.94 KBxjm
#259 decouple-comment-node-731724.259.patch346.6 KBlarowlan
#256 decouple-comment-node-731724.256.interdiff.txt529 byteslarowlan
#256 decouple-comment-node-731724.256.patch346.6 KBlarowlan
#253 decouple-comment-node-731724.253.interdiff.txt7.34 KBlarowlan
#253 decouple-comment-node-731724.253.patch346.66 KBlarowlan
#251 decouple-comment-node-731724.251.interdiff.txt593 byteslarowlan
#251 decouple-comment-node-731724.251.patch340.63 KBlarowlan
#249 decouple-comment-node-731724.249.interdiff.txt1.04 KBlarowlan
#249 decouple-comment-node-731724.249.patch340.62 KBlarowlan
#247 decouple-comment-node-731724.247.interdiff.txt588 byteslarowlan
#247 decouple-comment-node-731724.247.patch340.13 KBlarowlan
#245 decouple-comment-node-731724-245.patch340.24 KBlarowlan
#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
#228 decouple-comment-node-731724.228.interdiff.txt12.25 KBlarowlan
#228 decouple-comment-node-731724.228.patch340.21 KBlarowlan
#226 decouple-comment-node-731724.226.interdiff.txt7.54 KBlarowlan
#226 decouple-comment-node-731724.226.patch332.84 KBlarowlan
#222 731724-220-222.interdiff.txt763 bytespenyaskito
#222 731724-222.patch337.2 KBpenyaskito
#220 731724-220.patch337.2 KBpenyaskito
#216 731724-216.patch346.6 KBfubhy
#214 731724-214.patch346.34 KBfubhy
#212 731724-212.patch347.83 KBfubhy
#207 decouple-comment-node-731724.207.patch337.45 KBdagmar
#204 decouple-comment-node-731724.204.patch347.92 KBdagmar
#202 decouple-comment-node-731724.202.patch338.65 KBdagmar
#197 decouple-comment-node-731724.197.patch337.27 KBdagmar
#197 interdiff.txt12.19 KBdagmar
#192 decouple-comment-node-731724.192.interdiff.txt11.74 KBlarowlan
#192 decouple-comment-node-731724.192.patch338.66 KBlarowlan
#187 decouple-comment-node-731724.187.patch336.34 KBdagmar
#187 interdiff.txt701 bytesdagmar
#185 decouple-comment-node-731724.185.patch310.96 KBdagmar
#185 interdiff.txt701 bytesdagmar
#183 decouple-comment-node-731724.183.interdiff.txt2.25 KBlarowlan
#183 decouple-comment-node-731724.183.patch337.97 KBlarowlan
#174 decouple-comment-node-731724.174.interdiff.txt1.25 KBlarowlan
#174 decouple-comment-node-731724.174.patch337.91 KBlarowlan
#171 decouple-comment-node-731724.171.interdiff.txt2.97 KBlarowlan
#171 decouple-comment-node-731724.171.patch337.98 KBlarowlan
#167 decouple-comment-node-731724.167.patch337.62 KBlarowlan
#165 decouple-comment-node-731724.164.patch337.58 KBlarowlan
#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
#147 decouple-comment-node-731724.147.interdiff.txt58.28 KBlarowlan
#147 decouple-comment-node-731724.147.patch335.87 KBlarowlan
#137 decouple-comment-node-731724.137.patch328.92 KBlarowlan
#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
#133 decouple-comment-node-731724.133.patch326.37 KBlarowlan
#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
#128 decouple-comment-node-731724.128.interdiff.txt4.54 KBlarowlan
#128 decouple-comment-node-731724.128.patch322.92 KBlarowlan
#126 decouple-comment-node-731724.126.interdiff.txt622 byteslarowlan
#126 decouple-comment-node-731724.126.patch322.48 KBlarowlan
#124 decouple-comment-node-731724.124.interdiff.txt11.27 KBlarowlan
#124 decouple-comment-node-731724.124.patch322.78 KBlarowlan
#122 decouple-comment-node-731724.122.interdiff.txt18.33 KBlarowlan
#122 decouple-comment-node-731724.122.patch318.66 KBlarowlan
#120 decouple-comment-node-737124.120.patch318.32 KBlarowlan
#117 decouple-comment-node-737124.117.interdiff.txt119.48 KBlarowlan
#117 decouple-comment-node-737124.117.patch318.23 KBlarowlan
#115 decouple-comment-node-731724.115.interdiff.txt59.31 KBlarowlan
#115 decouple-comment-node-731724.115.patch283.64 KBlarowlan
#112 decouple-comment-node-731724.112.patch250.07 KBlarowlan
#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
#53 comment.731724-10.patch67.07 KBthehong
#50 comment.731724-6.patch51.52 KBthehong
#47 comment.3.patch12.92 KBthehong
#46 comment.2.patch9.04 KBthehong
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

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

marcingy’s picture

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

Bumping this is d8 material.

dargno’s picture

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

sun’s picture

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.

Rene Bakx’s picture

Title: Add entity_type column to comment table » Decouple 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?

rlmumford’s picture

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.

sun’s picture

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.
rlmumford’s picture

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

narayanis’s picture

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.

NaheemSays’s picture

yes, there is the nodecomments module which does that.

rlmumford’s picture

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.

joachim’s picture

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!

narayanis’s picture

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

rlmumford’s picture

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.

narayanis’s picture

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.

sun’s picture

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.

NaheemSays’s picture

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?

sun’s picture

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.

rlmumford’s picture

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.

joachim’s picture

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.

tstoeckler’s picture

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.

tstoeckler’s picture

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

joachim’s picture

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.

moshe weitzman’s picture

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

tstoeckler’s picture

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

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

sun’s picture

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.

tstoeckler’s picture

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.

dozymoe’s picture

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.

joachim’s picture

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

thehong’s picture

Status: Active » Needs work
FileSize
9.04 KB

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.

thehong’s picture

FileSize
12.92 KB

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_*
thehong’s picture

FileSize
51.52 KB

working on comment_form().

thehong’s picture

Assigned: tstoeckler » thehong
FileSize
67.07 KB

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

thehong’s picture

FileSize
85.78 KB

Basic comment, links worked.

tstoeckler’s picture

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.

thehong’s picture

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

webmasterkai’s picture

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.

tstoeckler’s picture

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

thehong’s picture

pounard’s picture

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.

dargno’s picture

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

amateescu’s picture

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

pounard’s picture

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.

moshe weitzman’s picture

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

pounard’s picture

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.

rogical’s picture

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

Is this possible to back port to D7?

bojanz’s picture

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

Anonymous’s picture

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

pounard’s picture

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

bojanz’s picture

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

vinoth.3v’s picture

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

Fidelix’s picture

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.

Anonymous’s picture

BTW: Any help with finishing Reply module is welcome.

Summit’s picture

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

eidoscom’s picture

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.

larowlan’s picture

Assigned: thehong » larowlan

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

andypost’s picture

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

larowlan’s picture

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

larowlan’s picture

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

andypost’s picture

dixon_’s picture

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.

larowlan’s picture

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

jhodgdon’s picture

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.

larowlan’s picture

Issue tags: -Avoid commit conflicts

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

sun’s picture

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

fubhy’s picture

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!

Anonymous’s picture

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.

larowlan’s picture

@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
larowlan’s picture

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.

semei’s picture

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.

larowlan’s picture

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

fubhy’s picture

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!

larowlan’s picture

@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

moshe weitzman’s picture

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!

fubhy’s picture

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.

larowlan’s picture

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.

sun’s picture

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.

fubhy’s picture

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

larowlan’s picture

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.

tim.plunkett’s picture

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

larowlan’s picture

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

larowlan’s picture

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

larowlan’s picture

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
larowlan’s picture

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.

larowlan’s picture

All comment module tests now passing

3270a56 All comment module tests now pass
larowlan’s picture

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

larowlan’s picture

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.

larowlan’s picture

Status: Active » Needs review
FileSize
283.64 KB
59.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

larowlan’s picture

Status: Needs review » Needs work
FileSize
318.23 KB
119.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

larowlan’s picture

Status: Needs work » Needs review
FileSize
318.32 KB

Re-roll

larowlan’s picture

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.

larowlan’s picture

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.

larowlan’s picture

larowlan’s picture

larowlan’s picture

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

larowlan’s picture

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

larowlan’s picture

larowlan’s picture

nevergone’s picture

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

Berdir’s picture

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

Berdir’s picture

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.

larowlan’s picture

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.

larowlan’s picture

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

julien’s picture

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.

larowlan’s picture

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

larowlan’s picture

Reroll for changes from @berdir and @chx on irc

Xano’s picture

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

larowlan’s picture

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

larowlan’s picture

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

falcon03’s picture

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

larowlan’s picture

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

larowlan’s picture

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

nevergone’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
22.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! :)

chx’s picture

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.

Berdir’s picture

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.

nevergone’s picture

FileSize
33.65 KB

Small curiosity: comment in comment! :)

falcon03’s picture

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

Berdir’s picture

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

Berdir’s picture

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

larowlan’s picture

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.

larowlan’s picture

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

larowlan’s picture

hmm lost attachment

larowlan’s picture

chasing HEAD

fubhy’s picture

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

rodrigoaguilera’s picture

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
larowlan’s picture

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.

larowlan’s picture

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

larowlan’s picture

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

larowlan’s picture

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

Lars Toomre’s picture

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

larowlan’s picture

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

Lars Toomre’s picture

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

larowlan’s picture

Thanks, will address on next re-roll.

larowlan’s picture

Berdir’s picture

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.

Xano’s picture

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.

larowlan’s picture

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

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

dagmar’s picture

dagmar’s picture

I forgot to add the new files.

dagmar’s picture

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!

xjm’s picture

Lars Toomre’s picture

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.

larowlan’s picture

Assigned: larowlan » Unassigned
Status: Needs work » Needs review
FileSize
338.66 KB
11.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.

chx’s picture

Status: Needs review » Needs work

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

dagmar’s picture

Assigned: Unassigned » dagmar

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

larowlan’s picture

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

dagmar’s picture

Assigned: dagmar » Unassigned
Status: Needs work » Needs review
FileSize
12.19 KB
337.27 KB

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

larowlan’s picture

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

Fabianx’s picture

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

andypost’s picture

dawehner’s picture

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

dagmar’s picture

Status: Needs work » Needs review
FileSize
338.65 KB

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

dagmar’s picture

Views issues are caused by bad named classes.

Renamed all the Ncs* classes into StatistcsLastComment*

dawehner’s picture

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

dagmar’s picture

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.

fubhy’s picture

FileSize
347.83 KB

Re-roll.

fubhy’s picture

FileSize
346.34 KB
fubhy’s picture

FileSize
346.6 KB
penyaskito’s picture

FileSize
337.2 KB

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

penyaskito’s picture

Status: Needs review » Needs work
FileSize
337.2 KB
763 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.

larowlan’s picture

Assigned: Unassigned » larowlan
Status: Needs work » Needs review
FileSize
332.84 KB
7.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.

larowlan’s picture

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

nevergone’s picture

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

mgifford’s picture

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.

larowlan’s picture

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.

realityloop’s picture

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

larowlan’s picture

Re-rolled

nevergone’s picture

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.

mbrett5062’s picture

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

mbrett5062’s picture

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.

mbrett5062’s picture

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.

tim.plunkett’s picture

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

mbrett5062’s picture

FileSize
17.1 KB
44.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.

larowlan’s picture

@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

larowlan’s picture

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

larowlan’s picture

Issue summary: View changes

Updated issue summary with api changes

larowlan’s picture

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.

larowlan’s picture

Fixes for new entity display changes

larowlan’s picture

Wrong bundle in display settings.

larowlan’s picture

Should fix remaining fails from entity translation and views changes.

andypost’s picture

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

larowlan’s picture

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

larowlan’s picture

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

kim.pepper’s picture

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?
kim.pepper’s picture

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.

larowlan’s picture

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

xjm’s picture

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

larowlan’s picture

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

xjm’s picture

Existing test attached for your convenience. :)

YesCT’s picture

larowlan’s picture

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

larowlan’s picture

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

larowlan’s picture

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

larowlan’s picture

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

andypost’s picture

Merged HEAD + fix some views plugins

andypost’s picture

Another try to merge tests

larowlan’s picture

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

andypost’s picture

Status: Needs work » Needs review
FileSize
358.74 KB
23.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

larowlan’s picture

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

larowlan’s picture

here's the -M25% one

tsvenson’s picture

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.

tsvenson’s picture

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.

tsvenson’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
359.85 KB
2.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

tsvenson’s picture

Status: Needs review » Needs work

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

tsvenson’s picture

Issue summary: View changes

Added sandbox link and updated testing instructions

tsvenson’s picture

Status: Needs work » Needs review

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

andypost’s picture

Merged 8.x and added fixes from #1903138: Move global comment permissions to comment-type 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

tsvenson’s picture

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?

andypost’s picture

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

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

tsvenson’s picture

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?

tsvenson’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
359.48 KB
1.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.

larowlan’s picture

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

swentel’s picture

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.

andypost’s picture

andypost’s picture

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"

andypost’s picture

Status: Needs work » Needs review
FileSize
359.59 KB
41.26 KB

Let's see results of merge

andypost’s picture

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

larowlan’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
1.25 KB
365.01 KB

Display still needed to convert view-modes

larowlan’s picture

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

dixon_’s picture

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.

dixon_’s picture

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

Anonymous’s picture

Issue summary: View changes

Added links to sandbox

dixon_’s picture

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.

andypost’s picture

@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.
larowlan’s picture

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

nevergone’s picture

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

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

andypost’s picture

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

larowlan’s picture

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

dixon_’s picture

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

Berdir’s picture

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 \

dixon_’s picture

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.

Berdir’s picture

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

Berdir’s picture

Issue summary: View changes

Added links to helper issue

dixon_’s picture

Issue summary: View changes

Updated issue summary to be more expressive.

dixon_’s picture

Issue summary: View changes

Updated issue summary.

dixon_’s picture

Issue summary: View changes

Corrected html.

yched’s picture

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 ?

yched’s picture

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 ?

larowlan’s picture

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.

andypost’s picture

Status: Needs work » Needs review
FileSize
367.56 KB
34.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.

andypost’s picture

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

larowlan’s picture

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 field settings that relate to rendering to formatter options
- 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 field settings that relate to rendering to formatter options
- 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 comment-type 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?

chx’s picture

Status: Needs review » Reviewed & tested by the community
yched’s picture

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.

amateescu’s picture

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:

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

andypost’s picture

Status: Reviewed & tested by the community » Needs work
bojanz’s picture

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.

andypost’s picture

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

larowlan’s picture

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

yched’s picture

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

andypost’s picture

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

tsvenson’s picture

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

tsvenson’s picture

Issue summary: View changes

Updated issue summary.

dixon_’s picture

Issue summary: View changes

Added follow-ups

dixon_’s picture

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

larowlan’s picture

FileSize
374.75 KB
3.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.

webchick’s picture

Title: Decouple comment.module from node » Decouple 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?

larowlan’s picture

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.

Crell’s picture

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

larowlan’s picture

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

larowlan’s picture

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.

Berdir’s picture

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.

andypost’s picture

Title: Decouple comment.module from node by turning comments into a field » Decouple comment.module from node by turning comment settings into a field
Status: Needs work » Needs review
FileSize
365.79 KB
3.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.

tsvenson’s picture

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.

podarok’s picture

Status: Needs work » Needs review

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

podarok’s picture

Issue summary: View changes

Updated issue summary. Added link to upgrade path issue

Berdir’s picture

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?

Crell’s picture

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.

larowlan’s picture

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.

rodrigoaguilera’s picture

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

andypost’s picture

Status: Needs review » Needs work
FileSize
382.28 KB
30.29 KB
6.74 KB

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

andypost’s picture

Status: Needs work » Needs review
FileSize
374.95 KB
mgifford’s picture

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).
tsvenson’s picture

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

andypost’s picture

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 
fubhy’s picture

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.

fago’s picture

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.

fago’s picture

Issue summary: View changes

Updated issue summary.

dixon_’s picture

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.

larowlan’s picture

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

Berdir’s picture

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

larowlan’s picture

larowlan’s picture

andypost’s picture

Currently sandbox needs some work to merge current HEAD and see #1907960-154: Helper issue for "Comment field"

larowlan’s picture

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
andypost’s picture

Status: Needs review » Reviewed & tested by the community

I think all addressed and patch ready to fly

tsvenson’s picture

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

sinasalek’s picture

Wow, that's great. super patch :) Thanks for all your hard works guys.

larowlan’s picture

Issue tags: +Avoid commit conflicts

*optimistic*

larowlan’s picture

Back to RTBC, re-roll against head (unless bot disagrees).

yannickoo’s picture

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?

tim.plunkett’s picture

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

yannickoo’s picture

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.

davidwbarratt’s picture

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!

andypost’s picture

Status: Needs work » Needs review
FileSize
381.98 KB

So here's a fixed patch with all suggestions - details see #1907960-170: Helper issue for "Comment field" till 175

andypost’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests, -Needs architectural review
FileSize
392.13 KB

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

nevergone’s picture

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
larowlan’s picture

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.

larowlan’s picture

Issue summary: View changes

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

andypost’s picture

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

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

andypost’s picture

An once again

effulgentsia’s picture

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?

Dries’s picture

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

tsvenson’s picture

@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 comment-type level follow up issue also makes it. Without moving those permissions to field level the usability will be severely limited.

mgifford’s picture

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.

larowlan’s picture

Status: Needs work » Needs review
FileSize
390.07 KB

Latest patch from helper issue.

andypost’s picture

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

xjm’s picture

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.

xjm’s picture

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.

xjm’s picture

Issue summary: View changes

types

effulgentsia’s picture

Title: Decouple comment.module from node by turning comment settings into a field » Convert 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.

alexpott’s picture

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

davidwbarratt’s picture

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!

Berdir’s picture

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.

Berdir’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Issue summary: View changes

updated remaining tasks

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

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

larowlan’s picture

Issue tags: +API change, +CMI

Added to the to-dos' in the issue summary

larowlan’s picture

Issue summary: View changes

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

tsvenson’s picture

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

Bojhan’s picture

Issue tags: -Needs usability review

nothing to review

xjm’s picture

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

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.

xjm’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Issue tags: +beta blocker

As discussed with @alexpott and @xjm in IRC

webchick’s picture

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.

webchick’s picture

Issue tags: +beta blocker

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

larowlan’s picture

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.

webchick’s picture

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

moshe weitzman’s picture

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.

larowlan’s picture

bugger, just as I got motivated again

nevergone’s picture

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

nevergone’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Added link to latest patch in the issue summary

larowlan’s picture

Issue summary: View changes

updated summary

andypost’s picture

Issue summary: View changes

added screenshots

catch’s picture

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?

webchick’s picture

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

webchick’s picture

Issue tags: +D8 upgrade path

Grrr.

andypost’s picture

Issue tags: -Needs screenshots

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.

larowlan’s picture

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

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.

webchick’s picture

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.

larowlan’s picture

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

webchick’s picture

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.

haydeniv’s picture

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.

tsvenson’s picture

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

larowlan’s picture

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.

tsvenson’s picture

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

larowlan’s picture

Status: Active » Needs review
FileSize
127.37 KB
420.85 KB
835 bytes

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

larowlan’s picture

FileSize
220.39 KB

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

Crell’s picture

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

larowlan’s picture

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

davidwbarratt’s picture

@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

Crell’s picture

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.

larowlan’s picture

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

effulgentsia’s picture

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.

effulgentsia’s picture

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.

nevergone’s picture

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.

larowlan’s picture

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

davidwbarratt’s picture

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.

davidwbarratt’s picture

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

dsnopek’s picture

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.

catch’s picture

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?

davidwbarratt’s picture

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.

bojanz’s picture

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.

moshe weitzman’s picture

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.

catch’s picture

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.

tsvenson’s picture

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

tsvenson’s picture

Dries’s picture

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.

webchick’s picture

larowlan’s picture

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"

effulgentsia’s picture

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.

sun’s picture

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.

odegard’s picture

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?

davidwbarratt’s picture

@odegard

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

odegard’s picture

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

effulgentsia’s picture

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.

rlmumford’s picture

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.

davidwbarratt’s picture

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

andypost’s picture

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

podarok’s picture

jibran’s picture

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.

joachim’s picture

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

joachim’s picture

Issue summary: View changes

new field settings screenshot

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Issue summary: View changes

Updated issue

larowlan’s picture

Issue summary: View changes

Updated issue summary

larowlan’s picture

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!

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

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.

larowlan’s picture

Issue summary: View changes

Updated issue summary.

larowlan’s picture

Issue summary: View changes

Added more follow ups/related issues

larowlan’s picture

Issue summary: View changes

Added last todo issue

larowlan’s picture

Issue summary: View changes

Updated UX review bit

sun’s picture

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.

Dave Reid’s picture

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

Dave Reid’s picture

Issue summary: View changes

Updated Api changes

larowlan’s picture

FileSize
449.85 KB

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

larowlan’s picture

andypost’s picture

FileSize
449.77 KB

Final patch

dixon_’s picture

Issue summary: View changes

Commented out manual upgrade path

dixon_’s picture

Edit: See #511

Bojhan’s picture

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

larowlan’s picture

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

andypost’s picture

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

dixon_’s picture

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

dixon_’s picture

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

andypost’s picture

Issue tags: +Avoid commit conflicts

Green! UX does not breaks things!

alexpott’s picture

Title: Convert comment settings into a field to make them work with CMI and non-node entities » Change 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!

fubhy’s picture

HOLY SHIIIIIIT! AWESOME!

Thanks larowland, andypost!!!

dixon_’s picture

Title: Change notice: Convert comment settings into a field to make them work with CMI and non-node entities » Convert 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.

dixon_’s picture

Title: Convert comment settings into a field to make them work with CMI and non-node entities » Change 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 :)

nevergone’s picture

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

clemens.tolboom’s picture

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

dsnopek’s picture

Woohoo! Congrats to everyone who worked on this!

jthorson’s picture

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')
NaheemSays’s picture

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

jibran’s picture

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

jthorson’s picture

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

chx’s picture

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

chx’s picture

Status: Active » Needs review
FileSize
617 bytes

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

Berdir’s picture

Status: Needs review » Reviewed & tested by the community
Gábor Hojtsy’s picture

Title: [HEAD BROKEN] Change notice: Convert comment settings into a field to make them work with CMI and non-node entities » Change 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!

jthorson’s picture

Priority: Critical » Major

And resetting Priority.

andypost’s picture

Priority: Major » Critical
Status: Active » Needs work

We still need a change notice

chx’s picture

Priority: Critical » Major
Status: Needs work » Active

Resetting to #514

larowlan’s picture

Status: Active » Needs review

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

Berdir’s picture

Title: Change notice: Convert comment settings into a field to make them work with CMI and non-node entities » Convert 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. :)

dsnopek’s picture

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!

clemens.tolboom’s picture

Sorry for the noise .. bash2head is done :p

The patch is awesome :)

eidoscom’s picture

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!

tstoeckler’s picture

Berdir’s picture

Issue tags: -sprint

Removing tag.

klonos’s picture

Yay! Great job everybody ;)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary with suggested commit message

yched’s picture

Wim Leers’s picture

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.

mohamed.marrouchi’s picture

Status: Closed (fixed) » Needs review

526: 731724_526.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 526: 731724_526.patch, failed testing.

Berdir’s picture

Status: Needs work » Closed (fixed)

Uhm.

nevergone’s picture

Assigned: » Unassigned

Please, don't touch this issue.

sol0matrix80’s picture

Any chance on this getting a D7 Port sometime in the future. Not all project are able to upgrade to D8

nevergone’s picture

sol0matrix80: not possible

sol0matrix80’s picture

So none D8 sites should use reply or figure out a way to attach comments to other entity besides nodes ?

Status: Closed (fixed) » Needs review

molnitza queued 526: 731724_526.patch for re-testing.

molnitza’s picture

Status: Needs review » Closed (fixed)

Sorry

molnitza’s picture

Sorry

molnitza’s picture

Sorry

molnitza’s picture

Sorry