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
Update issue summaryConvert CommentUserTest to use entity_test_render instead of user entityUpdate This spreadsheet with comments from comment 348, comment 349 and comment 351, comment 359, comment 360, comment 362 of the review/testing issue.Finish the unresolved items from the same spreadsheetFinal pass over the code to ensure that any @todos are encapsulated in follow up issuesFix ajax callback for new indicator in a multiple comment-field world.- Another UX review (have send message via contact form to @tsvenson 26-9)
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)Update API changes listRe-review test coverageRevisit history module integrationRevisit access control on comment-admin screenRevisit 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
Admin settings
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
- #1901110: Improve the UX for comment bundle pages and comment field settings
- #1920044: Move comment field settings that relate to rendering to formatter options
- #1920046: Remove comment_entity_view and introduce second formatter for links
- #1029708: History table for any entity
- #1919834: Field instance got no default value when created in field UI
- #1903138: Move global comment permissions to comment-type level
- #1805932: Resolve menu UI issues on 'Comment fields' tab of bundle field ui pages
- #2097123: Deprecate comment_num_new() in favour of method on CommentManager
- #1938062: Convert the recent_comments block to a view
- #2031903: Support per comment field comment count tokens
- #2098011: Support dynamic route parameters to enhance comment admin paths with entity type and field name from comment bundle
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.
Comment | File | Size | Author |
---|---|---|---|
#526 | 731724_526.patch | 617 bytes | chx |
#510 | drupal8.comment-module.731724-510.patch | 450.54 KB | andypost |
#510 | interdiff.txt | 3.99 KB | andypost |
#509 | interdiff.txt | 5.06 KB | larowlan |
#505 | 731724-comment-field-505.patch | 449.77 KB | andypost |
Comments
Comment #2
joachim CreditAttribution: joachim commentedExcellent 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...
Comment #3
marcingy CreditAttribution: marcingy commentedBumping this is d8 material.
Comment #5
dargno CreditAttribution: dargno commentedMore 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 :)
Comment #10
sunMight make sense to wait for #375397: Make Node module optional to get in first, but then again, there's no hard-dependency on that.
Comment #16
Rene BakxI'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?
Comment #17
rlmumfordSo personally I think comment should probably stay in core, but at any rate that would need to be discussed by the whole community. If comments were to stay in core then you probably couldn't rely on Relation to do the linking, but then I'm not sure you would need to.
Comment #18
sunComment 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
{comment_node}
,{comment_user}
, etc.Comment #20
rlmumfordSo 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())
Comment #22
narayanis CreditAttribution: narayanis commentedI 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.
Comment #23
NaheemSays CreditAttribution: NaheemSays commentedyes, there is the nodecomments module which does that.
Comment #24
rlmumfordWith 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.
Comment #25
joachim CreditAttribution: joachim commentedI 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!
Comment #26
narayanis CreditAttribution: narayanis commentedYes, @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
Comment #27
rlmumfordI 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.
Comment #28
narayanis CreditAttribution: narayanis commentedThe 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.
Comment #29
sunAgain, 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.
Comment #30
NaheemSays CreditAttribution: NaheemSays commentedBack 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?
Comment #31
sunFor 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.
Comment #32
rlmumfordHaving 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.
Comment #33
joachim CreditAttribution: joachim commentedYou 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.
Comment #34
tstoecklerI'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.
Comment #35
tstoecklerWhen 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! :)
Comment #36
joachim CreditAttribution: joachim commentedSince 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.
Comment #38
moshe weitzman CreditAttribution: moshe weitzman commentedAnd here is a link that reply project which does seem like a much modernized comment.module.
Comment #39
tstoeckler@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:
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.
Comment #40
sunFWIW, 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.
Comment #41
tstoecklerThat'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.
Comment #43
dozymoe CreditAttribution: dozymoe commentedI 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.
Comment #45
joachim CreditAttribution: joachim commented> 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...
Comment #46
thehong CreditAttribution: thehong commentedThis 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.
Comment #47
thehong CreditAttribution: thehong commentedContinue #46, patch in #49:
Comment #50
thehong CreditAttribution: thehong commentedworking on comment_form().
Comment #53
thehong CreditAttribution: thehong commented- Better install functions, schema alter.
- Various fixes.
Comment #54
thehong CreditAttribution: thehong commentedBasic comment, links worked.
Comment #55
tstoecklerDude, 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.
Comment #56
thehong CreditAttribution: thehong commentedCan you submit your patch or link me to your sandbox?
Comment #57
webmasterkai CreditAttribution: webmasterkai commentedtstoeckler, 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.
Comment #58
tstoecklerWell 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...
Comment #59
thehong CreditAttribution: thehong commentedMy sandbox: https://bitbucket.org/andytruong/drupal/src/a93d8bb74d6f
Comment #61
pounardFully 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.
Comment #62
dargno CreditAttribution: dargno commentedI 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
Comment #63
amateescu CreditAttribution: amateescu commentedI'd also like comments to remain an entity. What we really need is Entity reference or Relation in core (and Entity tree, ofc) :)
Comment #64
pounardComments 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.
Comment #65
moshe weitzman CreditAttribution: moshe weitzman commentedRight - thats what reply module does (as mentioned in #38 . It is a good model.
Comment #66
pounardYes, 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.
Comment #67
rogical CreditAttribution: rogical commentedReally need this in D7, as more and more data structures become entity.
Is this possible to back port to D7?
Comment #68
bojanz CreditAttribution: bojanz commented#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?
Comment #69
Anonymous (not verified) CreditAttribution: Anonymous commented@bojanz: "though the code itself is not exactly nice" - explain.
Comment #70
pounard@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.
Comment #71
bojanz CreditAttribution: bojanz commented@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.
Comment #72
vinoth.3v CreditAttribution: vinoth.3v commentedI think creating a new comment field is easier than rewriting the core comment module. :)
Comment #73
Fidelix CreditAttribution: Fidelix commentedvinoth.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.
Comment #74
Anonymous (not verified) CreditAttribution: Anonymous commentedBTW: Any help with finishing Reply module is welcome.
Comment #75
Summit CreditAttribution: Summit commentedHi,
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
Comment #76
eidoscomHi 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.
Comment #78
larowlanTransferring in from #1790764: [meta] Convert comment module to field_api which was marked as duplicate of this issue.
Comment #79
andypostThere was a mention about module http://drupal.org/project/reply
It looks pretty similar approach in http://drupal.org/sandbox/larowlan/1790736
Comment #80
larowlanCommitted first pass at upgrade path to my sandbox for this issue:
http://drupalcode.org/sandbox/larowlan/1790736.git/commit/6f541e7
Comment #81
larowlanCommitted 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
Comment #82
andypostThere's big checnges happen in #1778178: Convert comments to the new Entity Field API
Comment #83
dixon_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.
Comment #85
larowlanMore sandbox commits
Ready to start testing and porting tests
Comment #86
jhodgdonUm. 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.
Comment #88
larowlanMore sandbox commits
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
Comment #89
sun@larowlan: Wowza! Can you use git diffup to keep us updated? :)
Comment #90
fubhy CreditAttribution: fubhy commentedI 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!
Comment #91
Anonymous (not verified) CreditAttribution: Anonymous commentedHow 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.
Comment #92
larowlan@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:
Comment #93
larowlanShould 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.
Comment #94
semei CreditAttribution: semei commentedIs 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.
Comment #95
larowlanThis is purely D8, for D7 you'll need reply or similar.
Comment #96
fubhy CreditAttribution: fubhy commentedOkay, 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:
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!
Comment #97
larowlan@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
Comment #98
moshe weitzman CreditAttribution: moshe weitzman commentedRegarding 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!
Comment #99
fubhy CreditAttribution: fubhy commentedThanks 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.
Comment #100
larowlanMarked #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.
Comment #101
sunFriendly 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.
Comment #102
fubhy CreditAttribution: fubhy commentedYep, the mentioned suggestions would definitely be follow-up issues. I think we just wanted to point them out. :)
Comment #103
larowlanMore progress in sandbox
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.
Comment #104
tim.plunkettThis issue inherently fixes #1426062: Cannot uninstall comment when node is disabled., but that might land before this. Just a heads up.
Comment #105
larowlanThanks TIm, will follow that one.
Latest commits
Starting to see more green in test results :)
Comment #106
larowlantim-e has come on board to help with the formatters, thanks Tim.
Comment #107
larowlanMore sandbox commits
No more fatals for Comment module test suite
Slowly getting there!
Comment #108
larowlanMore sandbox commits
Down to 61 fails in Comment suite, forum and tracker tbd.
Comment #109
larowlanAll comment module tests now passing
Comment #110
larowlanAll 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
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
Comment #112
larowlanLet'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.
Comment #115
larowlanExpecting failures, just want to see how many
Still to do:
*Upgrade path
*Finish CommentUserTest
Comment #117
larowlanLet's see what the bot says now, expecting about 12-16 failing test classes.
Work from the weekend:
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
Comment #120
larowlanRe-roll
Comment #122
larowlanWork from last night and this morning:
@dawehner has provided patches for views issues, will apply and post those tonight.
Comment #124
larowlanOnce 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.
Comment #126
larowlangroan, long day
Comment #128
larowlanmeh
Comment #131
larowlanShould be down to just exceptions now.
Upgrade path is tested in LanguageUpgradePath as it tests for comments.
Comment #133
larowlanupdates bundle for forum tests to match removal of field_attach_bundle_rename from upgrade path
Comment #135
larowlanfingers crossed...
Comment #137
larowlanthis time?
Comment #140
nevergone CreditAttribution: nevergone commentedAfter 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).
Comment #141
BerdirWas just skimming through to look what you're doing with the preprocess functions, see #1810710: Remove copying of node properties wholesale into theme variables
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"?
Comment #142
BerdirLooked 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() :)
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 :)
Old comment :)
Careful with making a huge patch even bigger with unecessary doc fixes.
I assume this is just a temporary measure until with have generic entity access support, which is close to being commited?
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?
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?
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 :)
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.
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.
Same here, comment_reply_access() sounds much better to me.
As said above. This also uses a different tag than the one above.
You can also use isNull().
Ah, here is the @todo for that...
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.
Comment #143
larowlanThanks 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.
Comment #144
larowlanSome responses to questions above
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 :)
Yep, will add a @todo
This is very similar to what is already there, but will rewrite to be a queued task.
No worries, I had that in earlier patches that were using node_save (which I realise is a no-no in update hooks)
I believe so
Comment #145
julien CreditAttribution: julien commentedVery 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.
Comment #146
larowlan@julien - see #1735118: Convert Field API to CMI - CMI will manage the field - win!
Comment #147
larowlanReroll for changes from @berdir and @chx on irc
Comment #148
XanoDo 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)
Docblocks should start with a third person verb, in this case "Adds new...". This goes for more (update) functions in the patch.
Comment #149
larowlanI'm not sure we have a generic entity access solution for queries, we do for single entities (nearly)
Comment #151
larowlanRerolled to fix failing tests from comment translation ui, included Xano's changes to update hook comments.
Comment #152
falcon03 CreditAttribution: falcon03 commented@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.
Comment #153
larowlanHi @falcon03 - this will need a re-install - there are changes made in the install profile that cannot survive a HEAD to HEAD update.
Comment #154
larowlanAnd 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
Comment #155
nevergone CreditAttribution: nevergone commentedI'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! :)
Comment #156
chx CreditAttribution: chx commentedRe + // @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.
Comment #157
BerdirI 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.
Comment #158
nevergone CreditAttribution: nevergone commentedSmall curiosity: comment in comment! :)
Comment #159
falcon03 CreditAttribution: falcon03 commented@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....
Comment #160
BerdirStill 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.
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.
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?
Should be @param \Drupal\... . Probably wrong in other places too.
Comment #161
BerdirCreated http://groups.drupal.org/node/267143 to discuss that.
Comment #162
larowlanYes had thought of that too, I think it's a way forward
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
@chx asked for this so links in emails etc would keep working
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.
Comment #164
larowlanchasing 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
Comment #165
larowlanhmm lost attachment
Comment #167
larowlanchasing HEAD
Comment #168
fubhy CreditAttribution: fubhy commentedYou 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())
Comment #169
rodrigoaguileraI applied the patch and created comments for taxonomy, user and node, here are my notes
Comment #170
larowlanDoes 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.
Comment #171
larowlanHave extended comment_form_field_ui_field_edit_form to hide the required checkbox (and force cardinality to one).
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'll open a new ticket for this, once I confirm it's unrelated
This limits the number shown on a page, not the number entered.
fixed, was handling comment count incorrectly
Comment #172
larowlanjust to confirm that
is a pre-existing bug in HEAD
will file new issue with tests
Comment #174
larowlanWhoops, didn't mean to alter cardinality on all fields, just comment ones.
Comment #175
Lars Toomre CreditAttribution: Lars Toomre commentedAttached 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.
Should be something like 'Pagecallback: Displays...'. This docblock is missing a @return directive.
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.
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).
Needs to be a one-line summary less than 80 characters.
Should be a blank line after '<?php'.
Comment #176
larowlanThanks Lars, will include those in next re-roll - some questions though:
Ok, will try and find a reference to that in the standards.
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
Comment #177
Lars Toomre CreditAttribution: Lars Toomre commentedSorry 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).
Comment #178
larowlanThanks, will address on next re-roll.
Comment #179
larowlan@rodrigoaguilera #1845372: Deleting a field from a non-node entity bundle results in an Entity Field Query Exception is for the new issue
Comment #180
BerdirNot if they map to a column in the table of the entity, as they do in this case, then entity_type is correct.
Comment #182
XanoWhere 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.
Comment #183
larowlanRe-roll to include Lars Toomre's comments, left it as ->entity_title for consistency.
Chasing HEAD where history.module has been committed, win!
Comment #185
dagmarComment #187
dagmarI forgot to add the new files.
Comment #189
dagmarCan we implement this using a hook? Maybe hook_query_TAG_alter.
$node->comment_count?
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?
Could you explain why this is required?
Why this test is removed?
I think this is similar to issues with convert url paths into fields.
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!
Comment #190
xjmFollowup: #1847540: [META] Clean up comment module tests and decouple from node
Comment #191
Lars Toomre CreditAttribution: Lars Toomre commentedHere are some observations from reading through first portion of this patch.
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'.
I would format this like 'comment_entity_langcode' just below.
Shouldn't this value be set by a Config file?
I think this function needs @param and @return directives.
This needs to be split into type hint and then description line.
This needs a @return directive.
Can you add type hints to rest of this docblock? Odd to see them only on the the changed @params.
To 'comment on' the given enitity?
Looks like @param directives for $entity and $field_name are missing.
Again if adding or changing a @param directive, please add type hints to whole docblock.
Should state what the default value is.
Missing @param and @return directives.
Please add type hint to $pid. Must this be an integer or can it be a string like uuid?
Look like missing @throws directive in docblock for this function.
Comment #192
larowlanFixed
Can these go into follow-ups?
As a pseudo upgrade path for sites with existing tokens of that format
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
Correct, carry over from test debugging.
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
fixed
added a @todo to remove once #1029708: History table for any entity lands
fixed
fixed
fixed
fixed
fixed
fixed
fixed
fixed
fixed
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.
Comment #194
chx CreditAttribution: chx commentedNote: Drupal\system\Tests\Transliteration\TransliterationTest 2 fails is unrelated.
Comment #195
dagmarI'm going to provide a patch to fix the failing tests.
Comment #196
larowlan@dagmar, all references to COMMENT_ENTITY_HIDDEN need to become COMMENT_HIDDEN thx
Comment #197
dagmarRerolled. Thanks @larolwan. I get locally only one fail related to the TranslationUI.
Comment #198
larowlan@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.
Comment #199
Fabianx CreditAttribution: Fabianx commentedTestbot reported for #197:
FAILED: [[SimpleTest]]: [MySQL] 48,688 pass(es), 2 fail(s), and 2 exception(s).
=> needs work
Comment #200
andypostSuppose Translation UI depends on #1848904-13: Bundles cannot be specified in Entity Translation tests
Comment #201
dawehnerDagmar, for sure is one of the few people which don't need help with views,
but hey @dagmar if you need help ... ping.
Comment #202
dagmarI 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
Comment #204
dagmarViews issues are caused by bad named classes.
Renamed all the Ncs* classes into StatistcsLastComment*
Comment #205
dawehner@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?
Comment #207
dagmarI 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.
Comment #212
fubhy CreditAttribution: fubhy commentedRe-roll.
Comment #214
fubhy CreditAttribution: fubhy commentedComment #216
fubhy CreditAttribution: fubhy commentedComment #220
penyaskitoRerolling patch, applies well but I'm confused because its size is lower than #216.
Comment #222
penyaskitoBecause 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.
Comment #226
larowlanPatch 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.
Comment #228
larowlanHmm, git identified the CommentUserTest as a rename of CommentTestBase, attached fixes it. Probably why the patch size dropped.
Comment #229
nevergone CreditAttribution: nevergone commentedGreen, I like this! :) But I no longer dare to RTBC… :S
Comment #230
mgiffordI 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.
Comment #231
larowlanJust so you know, I've an agreement with @fago that #1778178: Convert comments to the new Entity Field API will go in before this.
Comment #232
realitylooppossibly worth looking at for inclusion http://drupal.org/project/better_comments_admin
Comment #236
larowlanRe-rolled
Comment #237
nevergone CreditAttribution: nevergone commentedTest #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.
Comment #238
mbrett5062 CreditAttribution: mbrett5062 commentedThis change already committed in http://drupal.org/files/drupal-1821612-10.patch
Comment #239
mbrett5062 CreditAttribution: mbrett5062 commentedHave 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.
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.
And the widget is now gone from field configuration.
Also see this picture.
Comment #240
mbrett5062 CreditAttribution: mbrett5062 commentedAlso, when editing comment bundles the breadcrumb and title of the overlay should be updated, see content types. Pictures below.
I know what I am editing above. However :
What am I editing here, I forgot.
Comment #241
tim.plunkett#240 sounds like a great follow-up feature request.
Comment #242
mbrett5062 CreditAttribution: mbrett5062 commentedOK, the 'reply' functionality seems broken. I will try to explain, but not easy.
First off, with reference to the following image.
(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).
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.
Comment #243
larowlan@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
Comment #245
larowlanRe-rolled against latest HEAD, updated summary with link to the sandbox and testing instructions + follow up as per #240
Comment #245.0
larowlanUpdated issue summary with api changes
Comment #247
larowlanviews_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.
Comment #249
larowlanFixes for new entity display changes
Comment #251
larowlanWrong bundle in display settings.
Comment #253
larowlanShould fix remaining fails from entity translation and views changes.
Comment #254
andypostTryed 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
Comment #255
larowlanGreen 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...
Comment #256
larowlanMissed one call to views_fetch_data.
Latest code is in sandbox too.
Comment #257
kim.pepperTested this on latest sandbox in branch comment-fieldapi4.
* Installed D8 using standard profile
Testing Article:
Testing User Comments:
Comment #258
kim.pepperThe following errors are appearing in apache logs:
Not sure if it's related.
Comment #259
larowlanRe-roll after blocks as plugins went in.
Merged cleanly (woot!) but expecting fails.
Comment #261
xjm#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?
Comment #262
larowlan@xjm, I didn't get an answer regarding including that test here - happy to do so. Will add on next re-roll.
Comment #263
xjmExisting test attached for your convenience. :)
Comment #265
YesCT CreditAttribution: YesCT commentedif it helps, #1778178: Convert comments to the new Entity Field API is in.
Comment #268
larowlanthanks @YesCT. I'm half way through the epic re-roll. screenshot here: http://imgur.com/h1fuX
Comment #269
larowlanprogress update, have re-rolled after EntityNG but getting some local failures, latest code is in sandbox, will get to it over the weekend
Comment #270
larowlanDidn't get to this at the weekend, working on #1871772: Convert custom blocks to content entities instead
Comment #271
larowlanLatest 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
Comment #273
andypostMerged HEAD + fix some views plugins
Comment #275
andypostAnother try to merge tests
Comment #277
larowlanThanks @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
Comment #278
andypostFixed 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
Comment #280
larowlanThis should fix some of the fails, the disable/enable of comment module can't be done with existing fields, so dropped that.
Comment #282
larowlanhere's the -M25% one
Comment #284
tsvenson CreditAttribution: tsvenson commentedBeing 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.
"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.
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.
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.
The help text isn't actually showing up on the comment edit form any more:
Not really sure how this happened...
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.
Comment #285
tsvenson CreditAttribution: tsvenson commentedFound one more thing:
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.
Comment #286
tsvenson CreditAttribution: tsvenson commentedSeems I found a problem too:
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.
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.
Comment #287
andypostHere'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
Comment #288
tsvenson CreditAttribution: tsvenson commented@andypost Just happy to help out. Following the sandbox issue now to help keep me updated on the progress.
Comment #288.0
tsvenson CreditAttribution: tsvenson commentedAdded sandbox link and updated testing instructions
Comment #290
tsvenson CreditAttribution: tsvenson commentedFollow 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.
Comment #291
andypostMerged 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 wrongComment #293
tsvenson CreditAttribution: tsvenson commented@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?
Comment #294
andypostSuppose we need usability review for #284 and others, "Comment bundles" now "Comment fields"
Comment #295
tsvenson CreditAttribution: tsvenson commented@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?
Comment #296
tsvenson CreditAttribution: tsvenson commentedOne 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_".
This also gets reflected in the table names:
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.
Comment #297
andypost@tsvenson Yes other UI changes will go in follow-up patches because all of them require form_alter's
Last broken test fixed.
Comment #298
larowlanHi @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
Comment #300
swentel CreditAttribution: swentel commentedThis is pretty amazing already. Some first remarks, will do more in depth review and testing this weekend.
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.
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.
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.
This can be removed, display settings are not stored anymore in the instance, happens also in the _comment_body_field_create().
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.
Comment #301
andypostPatch needs re-roll and merge after commited #1801304: Add Entity reference field
Comment #302
andypostDoes 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"
Comment #303
andypostLet's see results of merge
Comment #305
andypostAnother round of fixes:
- broken ER tests
- removed display definition
- fixes in upgrade path
Comment #307
larowlanNote I removed all of the 'System Message' comments to keep this under 300 and avoid the pager. There were 33 of these.
Comment #308
andypostDisplay still needed to convert view-modes
Comment #309
larowlanWe should be able to use entity_get_display() here eg
Repeating for each view mode.
Comment #310
dixon_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:
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.Let's create a follow-up issue for this since generic Entity Access API has landed.
Let's create a follow-up issue about this.
Comment #311
dixon_Here's a simple re-roll to chase HEAD.
Comment #311.0
(not verified) CreditAttribution: commentedAdded links to sandbox
Comment #313
dixon_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.
Comment #314
andypost@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)
toisset($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.
Comment #315
larowlan@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.
Comment #317
nevergone CreditAttribution: nevergone commented#311: 731724-comment-decouple-311.patch queued for re-testing.
Re-roll and go-go-go in Drupal 8! :)
Comment #319
andypostNow 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
Comment #322
larowlanI think the TBD's listed at #319 qualify as follow ups, but I'm biased!
Comment #323
dixon_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 :)
Comment #324
BerdirSkimmed through ~50% the patch, some feedback below.
This function doesn't seem to do much other than the field type check, the underscore/hyphen comments are no longer true it seems...
@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.
Shouldn't inline the namespace.
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.
Unecessary change?
Another inline namespace
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.
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.
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.
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.
Another namespace thing.
Looks like you actually add a use but just for "Entity", not the interface? Is that actually needed?
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.
Missing leading \
Comment #326
dixon_Here's a patch that fixes all Berdir's comments.
Not much, but let's fix/remove that when we are converting over to the new routing system for the menu items, no?
Don't think so, but I added it since it makes sense. I also clarified the comments around that as well.
Yes, I consider this to be a pure conversion. So let's deal with that deeper issue in a follow-up.
Yes, let's do that in a follow-up since it doesn't have anything to do with this issue really.
I converted this to the proper entity access API.
Comment #327
BerdirHm.
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.
Comment #327.0
BerdirAdded links to helper issue
Comment #327.1
dixon_Updated issue summary to be more expressive.
Comment #327.2
dixon_Updated issue summary.
Comment #327.3
dixon_Corrected html.
Comment #329
yched CreditAttribution: yched commented$formatter_settings is never actually used ? Don't you want to just inline it within $display_options_default ?
Comment #333
yched CreditAttribution: yched commentedAwesome work. I mostly looked at the Field API related stuff so far, here are my remarks :
Looks like this was done and the @todo can be removed ?
- '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' ?
- if the form has no 'advanced' vertical tabs, we don't check user_access('administer comments') ?
- class comment-*node*-settings-form ?
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().
- 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 :-)
'behaviors' & FIELD_BEHAVIOR_* no longer exist in widget_info entries, and yup, the @todo should be done :-)
Minor, but why a separate $columns variable ? Why not just directly
return array('columns' => array(...)
?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 ?
This is not needed, fields not present in the EntityDisplay are considered hidden.
Wow. Does that if() really needs to be that convoluted ?
Comment #334
larowlanUntil 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.
Comment #335
andypost@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.
Comment #337
andypostShould 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
Comment #338
larowlanSo 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?
Comment #339
chx CreditAttribution: chx commentedAgreed (NSFW link).
Comment #341
yched CreditAttribution: yched commentedUnanswered from #333 :
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 :
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 ?
The phpdoc was updated, but the
@see field_info_fields()
mention is still stale. Probably just needs to go away.Comment #343
amateescu CreditAttribution: amateescu commentedIf that's the reason, you guys can just copy what entity_reference does for this exact situation:
Also, I assume that in the future this field will extend entity reference, right? But no need to worry about it for now :)
Comment #344
andypostCommited #1901290: Replace the mini pager with a lite pager (which does not run a count query) breaks tests http://qa.drupal.org/pifr/test/448498
Comment #345
bojanz CreditAttribution: bojanz commentedIt'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.
Comment #346
andypostTo 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.
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.
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 commentsComment #347
larowlanJust to note that @catch has confirmed this is a task and can be resolved post feature freeze.
Awesomesauce!
Comment #348
yched CreditAttribution: yched commentedOh 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.
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 ?Comment #350
andypost@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
Comment #351
tsvenson CreditAttribution: tsvenson commentedThere are some more use cases in the #1903138: Move global comment permissions to comment-type level follow up issue as well.
Comment #351.0
tsvenson CreditAttribution: tsvenson commentedUpdated issue summary.
Comment #351.1
dixon_Added follow-ups
Comment #352
dixon_I've added all the follow-ups to the issue summary.
Comment #353
larowlanAdding 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.
Comment #355
webchickUgh. 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?
Comment #357
larowlanConversation from irc
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.
Comment #358
Crell CreditAttribution: Crell commentedI'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. :-(
Comment #359
larowlanSome 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
Comment #360
larowlanJust 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.
Comment #361
BerdirI 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.
Comment #362
andypostI 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.
Comment #363
tsvenson CreditAttribution: tsvenson commented@webchick:
Bluntly, I would say that applying that logic means that in core the wrong decision was made to make Field UI fields of:
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.
Comment #365
podarok#1907960-74: Helper issue for "Comment field" shows no performance regression
Comment #365.0
podarokUpdated issue summary. Added link to upgrade path issue
Comment #367
BerdirThis 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?
Comment #368
Crell CreditAttribution: Crell commentedThe 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.
Comment #369
larowlanJust 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).
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.
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.
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.
Comment #370
rodrigoaguileraJust 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
Comment #372
andypost@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"
Comment #375
andypostTrying same patch as #1907960-88: Helper issue for "Comment field"
Comment #376
mgiffordLooked 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
Comment #377
tsvenson CreditAttribution: tsvenson commented@crell:
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.
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:
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.
Comment #378
andypostI'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
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 #379
fubhy CreditAttribution: fubhy commented'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.
Comment #380
fagoI'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.
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.
Comment #380.0
fagoUpdated issue summary.
Comment #381
dixon_@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.
Comment #383
larowlan@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?
Comment #384
BerdirShouldn't this be $type instead of the hardcoded 'node' for value?
Comment #385
larowlannew patch at #1907960-95: Helper issue for "Comment field"
Comment #386
larowlanRelated #1950632: Create a FieldDefinitionInterface and use it for formatters and widgets
Comment #391
andypostCurrently sandbox needs some work to merge current HEAD and see #1907960-154: Helper issue for "Comment field"
Comment #392
larowlanOk 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:
After:
Comment #393
andypostI think all addressed and patch ready to fly
Comment #394
tsvenson CreditAttribution: tsvenson commented@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.
Comment #395
sinasalek CreditAttribution: sinasalek commentedWow, that's great. super patch :) Thanks for all your hard works guys.
Comment #396
larowlan*optimistic*
Comment #398
larowlanBack to RTBC, re-roll against head (unless bot disagrees).
Comment #399
yannickooJust 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?
Comment #400
tim.plunkettWell. 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().
Comment #401
yannickooOkay, seems like the patch creates the patch
/admin/structure/comments
.Comment #403
davidwbarratt CreditAttribution: davidwbarratt commentedThanks 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!
Comment #404
andypostSo here's a fixed patch with all suggestions - details see #1907960-170: Helper issue for "Comment field" till 175
Comment #406
andypostNew 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
Comment #407
nevergone CreditAttribution: nevergone commentedContents:
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.
Users:
Comment mode is open, but comment form is not visible. If edit and save user, comment form is visible and working.
Comment #408
larowlanYou 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.
Comment #408.0
larowlanHopefully made the goal of this patch a little bit clearer.
Comment #414
andypostAfter a set of merge/re-rolls #1907960-211: Helper issue for "Comment field" is green again
Comment #418
andypostAn once again
Comment #421
effulgentsia CreditAttribution: effulgentsia commented#418 failed bot, so setting to needs work. Looks like the helper issue has a recent green patch. Is it ready for posting here?
Comment #422
Dries CreditAttribution: Dries commentedI'm excited about this patch but worried about the usability impact.
Comment #423
tsvenson CreditAttribution: tsvenson commented@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.
Comment #424
mgiffordThere'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.
Comment #425
larowlanLatest patch from helper issue.
Comment #426
andypostAccessibility 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
Comment #427
xjm@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.
Comment #428
xjmI 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.
Comment #428.0
xjmtypes
Comment #429
effulgentsia CreditAttribution: effulgentsia commentedTagging and retitling accordingly.
Comment #430
alexpottNote that calls to theme have been removed from the comment module - see #2008980: Replace theme() with drupal_render() in comment module
Comment #431
davidwbarratt CreditAttribution: davidwbarratt commentedI 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!
Comment #432
BerdirThat 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.
Comment #432.0
BerdirUpdated issue summary.
Comment #432.1
larowlanupdated remaining tasks
Comment #432.2
larowlanUpdated issue summary.
Comment #433
larowlanHi 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
Comment #434
larowlanAdded to the to-dos' in the issue summary
Comment #434.0
larowlanFixed spelling of my name and getting ready for the UX review when time comes...
Comment #435
tsvenson CreditAttribution: tsvenson commented@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.
Comment #436
Bojhan CreditAttribution: Bojhan commentednothing to review
Comment #437
xjmComment #438
xjmHmm, 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.
Comment #438.0
xjmUpdated issue summary.
Comment #439
larowlanAs discussed with @alexpott and @xjm in IRC
Comment #440
webchickCould 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.
Comment #441
webchickWell that wasn't intentional. Tag monster--;
Comment #442
larowlanI 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.
Comment #443
webchickI 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
Comment #444
moshe weitzman CreditAttribution: moshe weitzman commentedThe 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.
Comment #445
larowlanbugger, just as I got motivated again
Comment #446
nevergone CreditAttribution: nevergone commentedI would test again. Where can I find usable patch?
Comment #446.0
nevergone CreditAttribution: nevergone commentedUpdated issue summary.
Comment #447
larowlanAdded link to latest patch in the issue summary
Comment #447.0
larowlanupdated summary
Comment #447.1
andypostadded screenshots
Comment #448
catchThis 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?
Comment #449
webchickSure.
Let's start by examining the list of field types available to you in the drop-down with this patch:
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.
Comment #450
webchickGrrr.
Comment #451
andypostscreens 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.
Comment #452
larowlanOk 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
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
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.
Comment #453
webchickYou 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.
Comment #454
larowlanIn which I case I need to re-roll after #2074547: Remove ConfigFieldItemInterface::getInstance() which (as seen at comment #449) broke it again.
Comment #455
webchickHowever, 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.
Comment #456
haydeniv CreditAttribution: haydeniv commentedAs 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.
Comment #457
tsvenson CreditAttribution: tsvenson commentedIn 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 ;)
Comment #458
larowlanOk 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.
Comment #459
tsvenson CreditAttribution: tsvenson commented@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.
Comment #460
larowlanFixes for #2074547: Remove ConfigFieldItemInterface::getInstance()
Shot of it in action with two fields
Comment #461
larowlanbah chrome screenshot tool busted the screenshot
Try again on PNG
Comment #462
Crell CreditAttribution: Crell commentedFTR, 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.)
Comment #463
larowlanHow is $node->comment handled in REST atm?
Comment #464
davidwbarratt CreditAttribution: davidwbarratt commented@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:
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:
The reason I bring this up, is because it makes the complex entityreference structure of this:
to this:
(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
Comment #465
Crell CreditAttribution: Crell commentedIf $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.
Comment #466
larowlan@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.
Comment #467
effulgentsia CreditAttribution: effulgentsia commentedWe 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.
Comment #468
effulgentsia CreditAttribution: effulgentsia commentedHow 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.
Comment #469
nevergone CreditAttribution: nevergone commentedThis 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.
Comment #470
larowlanOk, 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
Comment #471
davidwbarratt CreditAttribution: davidwbarratt commentedPerhaps the better solution, is to have a field that is based off of Entityreference, but has the following differences:
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.
Comment #472
davidwbarratt CreditAttribution: davidwbarratt commentedAH!
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
Comment #473
dsnopekIf 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.
Comment #474
catchUntil 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?
Comment #475
davidwbarratt CreditAttribution: davidwbarratt commentedI 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.
Comment #476
bojanz CreditAttribution: bojanz commentedI 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.
Comment #477
moshe weitzman CreditAttribution: moshe weitzman commentedI 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.
Comment #478
catchThere'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.
Comment #479
tsvenson CreditAttribution: tsvenson commented@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?
Comment #480
tsvenson CreditAttribution: tsvenson commented#460: comment-field.reroll.459.patch queued for re-testing.
Comment #482
Dries CreditAttribution: Dries commentedIt 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.
Comment #483
webchickCross-referencing #1770720: [META] Gradual changes to Field UI.
Comment #484
larowlanSo 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"
Comment #485
effulgentsia CreditAttribution: effulgentsia commentedI 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.
Comment #486
sunEveryone 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.
Comment #487
odegard CreditAttribution: odegard commentedI 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?
Comment #488
davidwbarratt CreditAttribution: davidwbarratt commented@odegard
Are you referring to Entity reference (https://drupal.org/project/entityreference)?
If so, the module is going to be included in Drupal 8.
Comment #489
odegard CreditAttribution: odegard commentedApologies 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).
Comment #490
effulgentsia CreditAttribution: effulgentsia commentedEntity 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.
Comment #491
rlmumfordJust 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.
Comment #492
davidwbarratt CreditAttribution: davidwbarratt commentedSince 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
Comment #493
andypostCurrent 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
Comment #495
podarok#493: drupal8.comment-module.731724-493.patch queued for re-testing.
Comment #497
jibranI 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.
Comment #498
joachim CreditAttribution: joachim commented> 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.
Comment #498.0
joachim CreditAttribution: joachim commentednew field settings screenshot
Comment #498.1
larowlanUpdated issue summary.
Comment #498.2
larowlanUpdated issue summary.
Comment #498.3
larowlanUpdated issue
Comment #498.4
larowlanUpdated issue summary
Comment #499
larowlanAll 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!
Comment #499.0
larowlanUpdated issue summary.
Comment #500
larowlanLatest 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.
Comment #500.0
larowlanUpdated issue summary.
Comment #500.1
larowlanAdded more follow ups/related issues
Comment #500.2
larowlanAdded last todo issue
Comment #500.3
larowlanUpdated UX review bit
Comment #501
sunIn case #486 was not clear (possibly due to length), I'd like to clarify:
Disregarding the concrete use-case: Architecturally, this concept is badly needed in Drupal.
This concrete sample implementation will be copied + resembled by 1,000+ contributed and custom modules.
All major concerns and objections that have been raised are 100% valid. It would be a huge mistake to ignore them.
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.
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.
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.
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.
Business logic matters.
The good news: All of the code exists already. Including tests.
Consequently, what you want to do is this:
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).
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.
Comment #502
Dave Reid@sun: It would be great if you could please let the Media module maintainers know more about what you mean by the following:
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.
Comment #502.0
Dave ReidUpdated Api changes
Comment #503
larowlanLatest patch from helper issue (green).
Only final UX review left in remaining tasks.
Comment #504
larowlanAdded #2098191: Add a meta-data API
Comment #505
andypostFinal patch
Comment #505.0
dixon_Commented out manual upgrade path
Comment #506
dixon_Edit: See #511
Comment #507
Bojhan CreditAttribution: Bojhan commented@dixon So the priority of that UX issue will become critical and a bug report?
Comment #509
larowlanUX Follow ups:
#2099421: Add an administrative description for a comment field
Move entity-type from title to its own columnAdd hook_help() for the admin/structure/comments pageComment #510
andypostmerged HEAD and added suggested "HumanName" for field ui overview screens
Comment #511
dixon_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) ;)
Comment #512
dixon_@Bojhan: Yes, the UX follow-up should be marked critical when this gets in.
Comment #513
andypostGreen! UX does not breaks things!
Comment #514
alexpottCommitted 37949fe and pushed to 8.x. Thanks!
Comment #515
fubhy CreditAttribution: fubhy commentedHOLY SHIIIIIIT! AWESOME!
Thanks larowland, andypost!!!
Comment #516
dixon_Please see suggested commit message in the issue summary, to not forget people that worked in helper issues.
Comment #517
dixon_Cross post :)
Comment #518
nevergone CreditAttribution: nevergone commentedThank you very much for all the work! You are great! :)
Comment #519
clemens.tolboomI got this.
Comment #520
dsnopekWoohoo! Congrats to everyone who worked on this!
Comment #521
jthorson CreditAttribution: jthorson commentedI 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):
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:
Failed Test suites:
Typical failures:
Comment #522
NaheemSays CreditAttribution: NaheemSays commentedTo 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?)
Comment #523
jibran@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.
Comment #524
jthorson CreditAttribution: jthorson commented@nbz: It's not an upgrade break ... it means a fresh pull of HEAD currently will not pass testing.
Comment #525
chx CreditAttribution: chx commentedDO NOT ROLL THIS BACK. I am ssh'd into the bot, reproduced the fail, will fix in minutes.
Comment #526
chx CreditAttribution: chx commentedFix suggested by berdir, verified on bot! http://privatepaste.com/b576f959e0
Comment #527
BerdirYeah, we figured that out in #2095399: Merge DatabaseStorageController and DatabaseStorageControllerNG, super weird PHP bug, see https://bugs.php.net/bug.php?id=49472
Comment #528
Gábor HojtsyThanks, committed! Back to need change notices!
Comment #529
jthorson CreditAttribution: jthorson commentedAnd resetting Priority.
Comment #530
andypostWe still need a change notice
Comment #531
chx CreditAttribution: chx commentedResetting to #514
Comment #532
larowlanDraft change notice: https://drupal.org/node/2100015
Comment #533
BerdirLooks good to me, very detailed :)
F. I. X. E. D. :)
Comment #534
dsnopekFinally, 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!
Comment #535
clemens.tolboomSorry for the noise .. bash2head is done :p
The patch is awesome :)
Comment #536
eidoscomHi 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!
Comment #537
tstoecklerAwesome work guys!
Quick follow-up: #2101709: Remove the "bundle_prefix" concept from the entity system
Comment #538
BerdirRemoving tag.
Comment #539
klonosYay! Great job everybody ;)
Comment #540.0
(not verified) CreditAttribution: commentedUpdated issue summary with suggested commit message
Comment #541
yched CreditAttribution: yched commentedFYI: #2143589: _comment_get_comment_fields() is weird, costly and unused ?
Comment #542
Wim LeersThis 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.
Comment #544
mohamed.marrouchi CreditAttribution: mohamed.marrouchi commented526: 731724_526.patch queued for re-testing.
Comment #546
BerdirUhm.
Comment #548
nevergone CreditAttribution: nevergone commentedPlease, don't touch this issue.
Comment #549
sol0matrix80 CreditAttribution: sol0matrix80 commentedAny chance on this getting a D7 Port sometime in the future. Not all project are able to upgrade to D8
Comment #550
nevergone CreditAttribution: nevergone commentedsol0matrix80: not possible
Comment #551
sol0matrix80 CreditAttribution: sol0matrix80 commentedSo none D8 sites should use reply or figure out a way to attach comments to other entity besides nodes ?
Comment #553
molnitza CreditAttribution: molnitza commentedSorry
Comment #554
molnitza CreditAttribution: molnitza commentedSorry
Comment #555
molnitza CreditAttribution: molnitza commentedSorry
Comment #556
molnitza CreditAttribution: molnitza commentedSorry