Create a comment-type config entity and use that as comment bundles, require selection in field settings form

Makes sense, will tackle tomorrow unless someone beats me to it.

While not directly blocked, it might be easier to tackle this after the related issue gets committed (which will hopefully be *soooooon*), as that adds some helpful constants.

Not sure about the issue status of this one, it is essentially part of a critical beta blocker, but that is more about defining the ruleset, enforcing it in the API is less important I think. As this is an API change on it's own, might still be a beta blocker. @xjm?

Tagging as a beta target. I don't think it's beta-blocking on its own since it's only going to affect the comment module API and functionality, not the data model or the Entity API itself. (I do think it's worth a mention in the release notes if we don't sort it by then; inventing a new tag to that end.)

Once #1709960: declare a maximum length for entity and bundle machine names lands, as far as I can tell, HEAD will be extremely restrictive for comment bundles, capping them at EntityTypeInterface::BUNDLE_MAX_LENGTH... right? So if you try to make a comment field with a 16-character name on an entity type with a 16-character ID, it will blow up in your face, because that plus the comment prefix and the magic underscores is over 32. Would we have to override the entire entity constructor for comments?

It would be really great to put some examples in the summary of how the comment bundle is composed and what a few config object names with it would look like (a hypothetical field, instance, etc.). @Berdir has given me examples at least twice but I can't find a comment with them anywhere. :) The ones in my active config in my D8 installation are:

field.field.comment.comment_body.yml
field.instance.comment.node__comment.comment_body.yml
entity.form_display.comment.node__comment.default.yml
entity.view_display.comment.node__comment.default.yml
rdf.mapping.comment.node__comment.yml

...but sometimes the word "comment" is the module name here, and I think other times it's the name of the shipped default comment field on nodes. Right?

The problem is that if we'd do the switch to use a setting of the field as bundle, it would completely break all comments unless we provide a possibly non-trivial upgrade path (ah, maybe not if we'd default them to use the current bundle as their setting).

Once #1709960: declare a maximum length for entity and bundle machine names lands, as far as I can tell, HEAD will be extremely restrictive for comment bundles, capping them at EntityTypeInterface::BUNDLE_MAX_LENGTH... right?

No. HEAD can only validate known bundle entities, comment bundle's are a virtual construct, there's no way to do this. Even if we'd say that field_instances are the bundles of comments, that's a) not correct because we don't account for the . => __ replacement and it's limiting because right now, bundle_of is a 1:1 relationship. Nothing else could use field instances as bundles then.

The only thing we could do is validate them once they are defined in hook_entity_bundle_info(). But that would duplicate the preSave validation added in the parent issue, which is preferrable as we can validate that *before* we create the bundle, validating after won't make it go away.

The config bundle is {commented_entity_type}__{fieldname}.

One thing, as commented in the other issue is that we could make an exception to a certain degree because we *know* comment bundles only happen on comment, which only uses 7 of the allowed 32 characters, so it could give the 25 additional ones to the bundle length. That will however pose a problem when the bundle is stored somewhere on it's own and that relies on the new 32-constant. So not sure that's a good idea.

What about this?

Includes the constant

And probably should check the type

That adds validation for what we have, but the problem is that entity_types themself can be up to 32 characters long, which makes them un-commentable with the current implementation.

There's an example in #2220757: Limit length of Config::$id to something <= 166 characters about adding fields to commerce_order_line_item or an entity of similar length, the exact example there is no longer relevant, but that entity type is already 24 characters, which leaves 32-24-2=6 characters for the field name, and since the field_ prefix is 6 characters, it is impossible to add fields to that entity type without changing/removing the default prefix.

This also turned up in https://drupal.org/node/2116363, where we did run into a recursive loop because comment.module needed the field map to define the bundles but the new field map method there needs the bundles.

I think it would simplify the handling there if we could keep track of comment bundles in a simpler way, like a simple config file or a config entity and it's the only way we could support comment fields as base fields.

New title?

WIP patch, progressing well, lots to move though obviously
This has to be major and beta blocker b/c database changes.

In real the bundle for comments is a field instance entity reference + label & admin description

At the same time having a real config entity nice idea, so +1 here.
Actually this entity could carry "entity_type" and "field_name" inside, allowing us to get rid of this fields in comment table.
So maybe file a follow-up to change schema as well, also performance could be affected by this change

Only critical issues are beta blockers, and only core maintainers should add the beta blocker tag. Have you discussed this issue with a core maintainer? How critical is this change?

Edit: didn't see at first that this started out as the comment bundle name length issue and I just tried to add it as a reference here. :P

If it's not critical, it can be either or both of "beta target" (we're not going to block the beta on it, but we should really have it done) or "beta deadline" (if it's not done by beta, it needs to wait for 8.1 or 9.0).

For the record, it does sound like a beta blocker to me, and a more sensible architecture. :) But let's get a maintainer to look at this.

More work in progress
This allows us to clean up a lot of cruft fwiw.

To quote @Berdir

but the problem is that entity_types themself can be up to 32 characters long, which makes them un-commentable with the current implementation.

If we're going to have the ability to add a comments to any entity type then we need to be able to add comments to them. This is both critical and beta blocker.

Thanks @alexpott!

Another pass, getting some passes locally now.

Should we check with testbot then? :)

Another API change, FieldItemInterface::settingsForm needs to have $form passed by reference. Could be a separate issue t.b.h.

missed an rdf mapping

Let's see how this goes

Green: bring on the reviews!

I know you mean architecture review but here are some minor issues.

1. +++ b/core/modules/comment/lib/Drupal/comment/CommentTypeFormController.php
   @@ -0,0 +1,115 @@
   +    // @todo Inject this.
   +    foreach (\Drupal::entityManager()->getDefinitions() as $entity_type) {
   ...
   +    $edit_link = \Drupal::linkGenerator()->generateFromUrl($this->t('Edit'), $this->entity->urlInfo());
   

   Let's do it then.

2. +++ b/core/modules/comment/lib/Drupal/comment/CommentTypeFormController.php
   @@ -0,0 +1,115 @@
   +      $language_configuration = language_get_default_configuration('comment', $comment_type->id());
   

   This is not @deprecated but I think we can also use injection here.

3. +++ b/core/modules/comment/lib/Drupal/comment/CommentTypeInterface.php
   @@ -0,0 +1,23 @@
   +  public function getDescription();
   ...
   +  public function setDescription($description);
   ...
   +  public function getTargetEntityTypeId();
   

   doc blocks missing.

4. +++ b/core/modules/comment/lib/Drupal/comment/CommentTypeListBuilder.php
   @@ -0,0 +1,59 @@
   +    $row['type'] = \Drupal::linkGenerator()->generateFromUrl($entity->label(), $entity->urlInfo());
   

   I think we can use $entity->urlInfo()->toString() here

5. +++ b/core/modules/comment/lib/Drupal/comment/Entity/CommentType.php
   @@ -0,0 +1,112 @@
   + *   controllers = {
   

   Why don't we have an access controller fo this?

6. +++ b/core/modules/comment/lib/Drupal/comment/Entity/CommentType.php
   @@ -0,0 +1,112 @@
   + *   links = {
   + *     "delete-form" = "comment.type_delete",
   + *     "edit-form" = "comment.type_edit"
   + *   }
   

   add-form missing.

7. +++ b/core/modules/comment/lib/Drupal/comment/Entity/CommentType.php
   @@ -0,0 +1,112 @@
   +    elseif ($this->getOriginalId() != $this->id) {
   +      entity_invoke_bundle_hook('rename', 'comment', $this->getOriginalId(), $this->id);
   

   We can introduce $this->id() method.

8. +++ b/core/modules/comment/lib/Drupal/comment/Entity/CommentType.php
   @@ -0,0 +1,112 @@
   +  public function getDescription() {
   ...
   +  public function setDescription($description) {
   ...
   +  public function getTargetEntityTypeId() {
   

   Doc blocks missing.

9. +++ b/core/profiles/standard/config/install/rdf.mapping.comment.comment.yml
   @@ -1,6 +1,6 @@
   +id: comment.eomment
   

   typo

#29.4, no this is correct. toString is for urls only, doesn't help with links.

Ok then we have to override the constructor so that we can inject \Drupal::linkGenerator()

Note to self: We need some local actions/tasks for the comment-type pages/forms

29.1, 29.3. 29.6-29.9, 32 fixed
29.2 its not a simple wrapper, so that would mean duplication, prefer to leave until it is deprecated.
29.4 ignored as per 30
29.5 Added new permission instead, its a simple config entity, so kept it simple.

Missed perm in tests

Fixes 31
Ready for reviews again..

To the fourteen followers here - any chance of a review? This is one of the criticals blocking a beta

Patch is pretty solid, new functionality covered with tests.
Some points in my review are questionable because I still not fully understand the relation "comment type" => field => comment that causes to entity_load() the type config entity.

the new schema changes needs adjust a size of fields and indexes

the api change with &$form looks reasonable just not clear is this affects a serialization of the forms.
probably better to use static methods and get_class() to attach additional validation & submit

1. +++ b/core/modules/comment/comment.install
   @@ -67,12 +67,19 @@ function comment_schema() {
   -      'field_id' => array(
   +      'field_name' => array(
   

   I think there should be a follow-up to move this field to config entity, probably with entity_type

2. +++ b/core/modules/comment/comment.install
   @@ -67,12 +67,19 @@ function comment_schema() {
   -        'default' => 'node.comment',
   +        'default' => 'comment',
   
   @@ -196,12 +203,12 @@ function comment_schema() {
   -        'default' => 'node__comment',
   +        'default' => 'comment',
   

   not sure it make sense to provide default value

3. +++ b/core/modules/comment/comment.install
   @@ -152,7 +159,7 @@ function comment_schema() {
   -        array('field_id', 32),
   +        array('comment_type', 32),
   
   @@ -162,7 +169,7 @@ function comment_schema() {
   -        array('field_id', 32),
   +        array('comment_type', 32),
   

   comment_type is not needed 32 limit as it is already.

   field_id should be adjusted with current limit (probably 32)

4. +++ b/core/modules/comment/comment.install
   @@ -236,7 +243,7 @@ function comment_schema() {
   -    'primary key' => array('entity_id', array('entity_type', 32), array('field_id', 32)),
   +    'primary key' => array('entity_id', array('entity_type', 32), array('field_name', 32)),
   

   entity_type is 32 new

5. +++ b/core/modules/comment/comment.module
   @@ -263,6 +247,10 @@ function comment_permission() {
   +    'administer comment types' => array(
   

   great!

6. +++ b/core/modules/comment/comment.module
   @@ -1480,3 +1431,16 @@ function comment_file_download_access($field, EntityInterface $entity, FileInter
   +function comment_type_load($id) {
   

   any reason to introduce API change

7. +++ b/core/modules/comment/comment.services.yml
   @@ -12,9 +12,3 @@ services:
   -  comment.route_enhancer:
   

   awesome!

8. +++ b/core/modules/comment/config/schema/comment.schema.yml
   @@ -42,3 +42,29 @@ action.configuration.comment_unpublish_by_keyword_action:
   +comment.type.*:
   ...
   +    targetEntityTypeId:
   

   Suppose in follow-up we add field_names[] to allow apply this for a set of fields

9. +++ b/core/modules/comment/lib/Drupal/comment/CommentInterface.php
   @@ -67,24 +67,15 @@ public function getCommentedEntityId();
   -  public function getFieldId();
   

   Suppose better to add getTypeiD() like TermInterface does

10. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
    @@ -119,7 +119,24 @@ public function getAllFields() {
    +    if ($comment_type = $comment_type_storage->load($comment_type_id)) {
    +      if ($comment_type->targetEntityTypeId !== $entity_type) {
    +        throw new \InvalidArgumentException($this->t('The given comment type id %id can only be used with the %entity_type entity type', array(
    ...
    +    else {
    +      $comment_type_storage->create(array(
    ...
    +      ))->save();
    

    this needs inline comment about that "type" created silently

11. +++ b/core/modules/comment/lib/Drupal/comment/CommentManagerInterface.php
    @@ -53,31 +53,18 @@ public function getAllFields();
    +   * @param string $comment_type_id
    +   *   ID of comment type to use.
    

    Defaults to "comment"

12. +++ b/core/modules/comment/lib/Drupal/comment/CommentTypeFormController.php
    @@ -0,0 +1,143 @@
    +class CommentTypeFormController extends EntityForm implements ContainerInjectionInterface {
    

    CommentTypeForm - no more formControllers now after #2238077: Rename EntityFormController to EntityForm

13. +++ b/core/modules/comment/lib/Drupal/comment/CommentTypeListBuilder.php
    @@ -0,0 +1,98 @@
    +    return $this->t('Comment types');
    

    Suppose better make it in routing.yml

14. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
    @@ -505,8 +493,10 @@ public function getChangedTime() {
       public static function preCreate(EntityStorageInterface $storage, array &$values) {
    ...
    +    if (empty($values['comment_type']) && !empty($values['field_name']) && !empty($values['entity_type'])) {
    ...
    +      $field = entity_load('field_config', $values['entity_type'] . '.' . $values['field_name']);
    +      $values['comment_type'] = $field->getSetting('comment_type');
    

    it's not clear why comment type does not know its fields

15. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php
    @@ -30,7 +31,7 @@ class CommentItem extends FieldItemBase implements CommentItemInterface {
       public static function defaultSettings() {
         return array(
    -      'description' => '',
    +      'comment_type' => 'new',
    

    is this needed?
    hook_schema() and other places defines this as "comment"

16. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php
    @@ -194,41 +193,100 @@ public function isEmpty() {
    -  public static function processSettingsElement($element) {
    ...
    -    if (\Drupal::moduleHandler()->moduleExists('content_translation')) {
    ...
    -      $element += content_translation_enable_widget('comment', $element['#bundle'], $comment_form, $comment_form_state);
    

    is this really not needed now?

17. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php
    @@ -194,41 +193,100 @@ public function isEmpty() {
    +    $form['#submit'][] = array($this, 'createNewCommentType');
    +    $form['#validate'][] = array($this, 'validateNewCommentType');
    

    Nice DX achievement!

18. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php
    @@ -194,41 +193,100 @@ public function isEmpty() {
    +      // @todo Inject this once typed-data supports ContainerInjectionInterface.
    +      $comment_type = entity_create('comment_type', $settings['new_comment_type']);
    

    needs follow-up

19. +++ b/core/modules/field_ui/lib/Drupal/field_ui/Form/FieldEditForm.php
    @@ -166,6 +166,8 @@ public function buildForm(array $form, array &$form_state, FieldInstanceConfigIn
    +    $form['#submit'][] = array($this, 'submitForm');
    +    $form['#validate'][] = array($this, 'validateForm');
    

    what for?

Some points in my review are questionable because I still not fully understand the relation "comment type" => field => comment that causes to entity_load() the type config entity.

A comment-type contains the references to the targeted entity type only.
The field contains a reference to the comment-type.
Several fields on a given entity-type can use the same comment-type.
Eg this is allowed
Comment-type 'debate' - references node.
Node type 'debate' with field 'for' => uses 'public' comment type.
Node type 'debate' with field 'against' => uses 'public' comment type.
Node type 'bar' with field 'Comments' => uses 'public' comment type.
But this is not
Comment-type 'comments' - references node.
Custom block type 'comments thread' with field 'comments' => uses 'comments' comment-type.
Because the comment-type is only for nodes.
This is because of the entity_id field on Comment, the definition needs to set the target type based on bundle - so one comment-type (bundle) can only reference one target type.

the new schema changes needs adjust a size of fields and indexes

Will address as per your detailed review

the api change with &$form looks reasonable just not clear is this affects a serialization of the forms.
probably better to use static methods and get_class() to attach additional validation & submit

Typed-data doesn't support container-injection so there should be no container-services etc that can be injected.

1) The relationship goes the other way, several fields can reference one comment-type. See above for reasoning. Also this allowed for the smallest patch size, as much of the comment code is tied to field-names.
2) Will remove
3) Will fix
4) Not sure what you mean
5) :)
6) Its an exists callback for a machine name field only
7) :)
8) Again, relationship goes the other way field -> comment type
9) Not sure what you mean
10) Will do
11) Will do
12) Will do
13) Will do
14) Again, relationship goes the other way
15) This defaults to the form showing the 'add new comment-type' nested form
16) Yeah, we move it to the comment-type form instead - another major win from this patch
17) :)
18) #2053415: Allow typed data plugins to receive injected dependencies
19) They don't fire if #submit or #validate isn't null.

So that leaves 2,3, 10-13 to fix.
Not sure on 4 and 9 - can you get back to me on those so I can address in one pass?

1. +++ b/core/modules/comment/comment.module
   @@ -555,10 +543,13 @@ function comment_node_view_alter(&$build, EntityInterface $node, EntityViewDispl
    function comment_add(EntityInterface $entity, $field_name = 'comment', $pid = NULL) {
   +  /** @var \Drupal\field\FieldConfigInterface $field */
   +  $field = entity_load('field_config', $entity->getEntityTypeId() . '.' . $field_name);
   

   #2116363: Unified repository of field definitions (cache + API) will add a Fieldconfig::loadByName($entity_type_id, $field_name) :) that issue will also conflict quite a bit with this and is almost ready...

   One question that came up there is if this will also allow to define an entity type with a comment base field and then manage it properly, that became obvious in that issue that this is currently impossible.

2. +++ b/core/modules/comment/comment.module
   @@ -1480,3 +1431,16 @@ function comment_file_download_access($field, EntityInterface $entity, FileInter
   +
   +/**
   + * Loads a comment type.
   + *
   + * @param int $id
   + *   The ID of the comment type to load.
   + *
   + * @return \Drupal\comment\CommentTypeInterface
   + *   A CommentTypeInterface object or NULL if the requested $id does not exist.
   + */
   +function comment_type_load($id) {
   +  return entity_load('comment_type', $id);
   +}
   

   We're not doing this anymore... See #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities, reviews there would be great, want to get that in.

3. +++ b/core/modules/comment/comment.views.inc
   @@ -332,9 +332,26 @@ function comment_views_data() {
   +
   +  $data['comment']['comment_type'] = array(
   +    'title' => t('Comment type'),
   +    'help' => t('The comment type from which the comment originated.'),
   

   That help doesn't seem to make much sense?

4. +++ b/core/modules/comment/lib/Drupal/comment/CommentStatistics.php
   @@ -156,7 +156,7 @@ public function getRankingInfo() {
              'alias' => 'ces',
              // Default to comment field as this is the most common use case for
              // nodes.
   -          'on' => "ces.entity_id = i.sid AND ces.entity_type = 'node' AND ces.field_id = 'node__comment'",
   +          'on' => "ces.entity_id = i.sid AND ces.entity_type = 'node' AND ces.field_name = 'comment'",
   

   Hardcoding that here seems... weird?

5. +++ b/core/modules/comment/lib/Drupal/comment/Entity/CommentType.php
   @@ -0,0 +1,123 @@
   +class CommentType extends ConfigEntityBase implements CommentTypeInterface {
   

   Do we need to care about config dependencies somehow here? Do we, can we, make the fields depend on this?

6. +++ b/core/modules/comment/lib/Drupal/comment/Entity/CommentType.php
   @@ -0,0 +1,123 @@
   +    elseif ($this->getOriginalId() != $this->id()) {
   +      entity_invoke_bundle_hook('rename', 'comment', $this->getOriginalId(), $this->id());
   

   We should really have a base class for all those bundle config entity types that automatically invoke those methods :)

And some comments on the above two comments:

2. Yeah, schema-level default values will go away anyway in #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities.
3./4.) entity_type and bundle lenght is now limited to 32 characters, so you don't need to limit the length on all those indexes.
9. As I understand it, he's suggesting to add a comment-specific method that returns the bundle, similar to getVocabularyId() on terms, as that often makes more sense when reading the code.

postponed on #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities for now, but will re-roll in the meantime

@larowan about 4/9 exactly what @berdir said:
4) no more need in 32 in index and key
9) special method to get bundle name (comment type)

@larowlan, this is a beta blocker, but #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities is just a major task, so we probably shouldn't block this issue on that one. Setting back to NR for now. (That said, #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities is a personal favorite and I'd love to see it go in too.) :) Maybe we could provide a -do-not-test.patch that shows what it would be rolled against that change?

The changes/overlaps with the load issue are minor, it will not conflict with that I think.

The one that does overlap a lot is #2116363: Unified repository of field definitions (cache + API), which is a critical beta blocker as well, so postponing on that would be OK.

Two related small issues that I created in the meantime are #2261401: Automatically provide bundle info/list based on bundle_of annotation. and #2261369: Introduce a common config entity base class for all bundle config entity types, which will save a few lines of code but worth waiting for...

Makes sense; postponing on #2116363: Unified repository of field definitions (cache + API).

Actually setting postponed.

Unpostponed!

Correct status.

36: comment-type-2228763.10.patch queued for re-testing.

tackling reroll and fixes

Fixed 38.2-4, 38.9-13.
Fixed 40.1 via re-roll
40.2 still not in HEAD
Fixed 40.3
40.4 its how it is in HEAD now, an same functionality as D7
40.6 you did and its in, so re-rolled
40.5 attempted but had no luck - couldn't access protected dependencies property or protected addDependency method on the field config with type comment.

40.4: True, it's an existing issue but you can't compare it with D7 because it wasn't configurable there. We don't need to fix it here, but we should open an issue for it so that we can expose it per field or something.

@Berdir and @larowlan re 40.5

You get bundle entity dependency for free with field instances!

dependencies:
  entity:
    - comment.type.comment
    - field.field.comment.comment_body

field.instance.comment.comment.comment_body after installing the standard profile has the correct dependencies AFAICS.

dependencies:
  module:
    - comment
    - text

field.instance.comment.comment_body after installing the standard profile has the correct dependencies AFAICS.

dependencies: {  }

comment.type.comment after installing the standard profile has the correct dependencies AFAICS. N.b. the dependency on the comment module is implicit.

Also...

+++ b/core/profiles/standard/config/install/rdf.mapping.comment.comment.yml
@@ -27,3 +27,5 @@ fieldMappings:
diff --git a/core/vendor/behat/mink-goutte-driver b/core/vendor/behat/mink-goutte-driver

diff --git a/core/vendor/behat/mink-goutte-driver b/core/vendor/behat/mink-goutte-driver
new file mode 160000

new file mode 160000
index 0000000..b114b9b

index 0000000..b114b9b
--- /dev/null

--- /dev/null
+++ b/core/vendor/behat/mink-goutte-driver

+++ b/core/vendor/behat/mink-goutte-driver
+++ b/core/vendor/behat/mink-goutte-driver
@@ -0,0 +1 @@

@@ -0,0 +1 @@
+Subproject commit b114b9b9706fa880ede66081322f5a86bdf020d8

Having fun with behat? :)

Talked with @Berdir on IRC

17:30 berdir
alexpott: https://drupal.org/node/2228763 is not about the default dependency
17:30 Druplicon
https://drupal.org/node/2228763 => Create a comment-type config entity and use that as comment bundles, require selection in field settings form [
#2228763]
=> 56 comments, 4 IRC mentions
17:30 berdir
alexpott: you create a comment bundle. then you create a comment field and select which comment bundle this should use
17:30 alexpott
berdir: ok - so what dependency is missing?
17:30 berdir
it's that setting
17:31 alexpott
berdir: ah so the FieldConfig needs to depend on the comment type too
17:31 berdir
yes
17:33 alexpott
berdir: lol so this is the same problem as translation sync... dumping a setting into FieldConfig is the problem
17:34 alexpott
berdir: or we need to add a way for FieldTypes to add dynamic dependencies
17:35 berdir
alexpott: no, it is not, because this is a setting of the field type plugin, not *someone else*
17:35 berdir
alexpott: this is only needed for comment fields, the ones you attach to e.g. nodes so that you can comment on them
17:35 berdir
not fields that you attach to a comment bundle, there it's just the bundle and handled
17:36 alexpott
berdir: so we need to add a way for FieldTypes to add dynamic dependencies
17:36 berdir
I think so yes
17:36 berdir
and they currently don't use ConfigurablePluginInterface
17:37 berdir
CommentItem is the class/plugin that would want to do this
17:39 alexpott
berdir: well at least this seems solvable
17:40 berdir
sure, using the same pattern as we have for ConfigurablePluginInterface should work fine :) just yet another special case :)
17:40 berdir
but it's not possible right now :)
17:40 alexpott
berdir: why not?
17:41 berdir
because the API doesn't exist? :)
17:41 berdir
not possible right now = without adding a new API for field items/types
17:42 alexpott
berdir: ok - so a new issue or do it in the comment issue?

So we in the examples above we need field.instance.comment.comment_body to depend on comment.type.comment because the field is linked in its settings to that particular bundle.

+++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
+++ b/core/lib/Drupal/Core/Field/FieldItemInterface.php
@@ -250,7 +250,7 @@ public static function defaultInstanceSettings();

@@ -250,7 +250,7 @@ public static function defaultInstanceSettings();
    * @return
    *   The form definition for the field settings.
    */
-  public function settingsForm(array $form, array &$form_state, $has_data);
+  public function settingsForm(array &$form, array &$form_state, $has_data);
 
   /**
    * Returns a form for the instance-level settings.

Why is this function signature being changed?

1. +++ b/core/modules/comment/comment.install
   @@ -67,12 +67,18 @@ function comment_schema() {
          'uid' => array(
            'type' => 'int',
   @@ -151,8 +157,8 @@ function comment_schema() {
   
   @@ -151,8 +157,8 @@ function comment_schema() {
          'comment_status_pid' => array('pid', 'status'),
          'comment_num_new' => array(
            'entity_id',
   -        array('entity_type', 32),
   -        array('field_id', 32),
   +        'entity_type',
   +        'comment_type',
            'status',
            'created',
            'cid',
   @@ -161,8 +167,8 @@ function comment_schema() {
   
   @@ -161,8 +167,8 @@ function comment_schema() {
          'comment_uid' => array('uid'),
          'comment_entity_langcode' => array(
            'entity_id',
   -        array('entity_type', 32),
   -        array('field_id', 32),
   +        'entity_type',
   +        'comment_type',
            'langcode',
   

   The 32 should be a constant.

2. +++ b/core/modules/comment/comment.install
   @@ -196,12 +202,12 @@ function comment_schema() {
          'cid' => array(
            'type' => 'int',
   @@ -236,7 +242,7 @@ function comment_schema() {
   
   @@ -236,7 +242,7 @@ function comment_schema() {
            'description' => 'The total number of comments on this entity.',
          ),
        ),
   -    'primary key' => array('entity_id', array('entity_type', 32), array('field_id', 32)),
   +    'primary key' => array('entity_id', 'entity_type', array('field_name', 32)),
        'indexes' => array(
          'last_comment_timestamp' => array('last_comment_timestamp'),
          'comment_count' => array('comment_count'),
   diff --git a/core/modules/comment/comment.local_actions.yml b/core/modules/comment/comment.local_actions.yml
   
   diff --git a/core/modules/comment/comment.local_actions.yml b/core/modules/comment/comment.local_actions.yml
   new file mode 100644
   

   This 32 should be a constant.

57 will file an issue and postpone this, have the patch in my head
58 so field types can add submit and validate callbacks
59 this is in head, can we open a new issue?

Interdiff towards fixing fails (just one in migrate to go, which is because of the change to field info)
Patch coming after re-roll

Ok, I can't get MigrateDrupal6Test to pass, so chasing some help, seems related to #2116363: Unified repository of field definitions (cache + API)

On this list core/modules/migrate_drupal/lib/Drupal/migrate_drupal/Tests/d6/MigrateCommentTest.php:89

$field_definitions = $this->entityManager->getFieldDefinitions($this->storage->getEntityTypeId(), $bundle);

We're not getting the 'comment_body' field.
Clearing field definitions doesn't help.
The bundle is definitely correct (comment).
So something not right.
Any help would be appreciated.
Re-roll after #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities

Added #2271419: Allow field types, widgets, formatters to specify config dependencies for 57

Another re-roll, things move fast

more fixes

More fixes

The fail is because line 89 of \Drupal\migrate\Plugin\migrate\destination\EntityBaseContent isn't returning the comment-body field.
The bundle looks to be correct.

$field_definitions = $this->entityManager->getFieldDefinitions($this->storage->getEntityTypeId(), $bundle);

Had a look at the test, the setUp() is only executed for the single-run mode. MigrateDrupal6Test.php runs the test methods without calling the setUp() methods as it expects that the migrations alone create the complete environment.

Which makes sense, because that's what a real migration run will need to do.

Somehow, we need to "migrate" the default comment type out of thin air... :)

That's hardly a problem, just find a handy plugin that provides a single row (variable does that just make sure you provide a variable surely existing in D6) and then use the constants feature.

Thanks chx, did so using site_name variable
Also thanks to @benjy for advising that I need to add the migration to the test too

Wrong method, doh

So the issue remains that sub-tests of MigrateDrupal6Test don't get their setUp methods run.
Not sure if that is by design.
If it is, then I'm not sure how the comment_body field is built in HEAD.
But wherever that code is, I suspect it uses the wrong bundle

@@ -73,34 +73,6 @@ class CommentType extends ConfigEntityBase implements CommentTypeInterface {
   /**
    * {@inheritdoc}
    */
-  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
-    parent::postSave($storage, $update);
-
-    if (!$update && !$this->isSyncing()) {
-      entity_invoke_bundle_hook('create', 'comment', $this->id());
-      if (!$this->isSyncing()) {
-        \Drupal::service('comment.manager')->addBodyField($this->id());
-      }
-/

Found it!

Fingers crossed

I am unhappy with

+      // Set default bundle.
+      $row->setDestinationProperty('comment_type', 'comment');

this makes it impossible to migrate into another bundle if someone wants. Other destination plugins do not call row->setDestinationProperty at all. And they should not, that's not their role. So, is this really necessary? I saw you are setting comment_type to a constant comment anyways in d6_comment.yml . If it is necessary we need to talk about migrate architecture...

Edit: the source is the mine, the process is the factory, the destination is just the department store. It is hardly the role of the store to fix the broken wares even if it's like the good ole' soviet padlock which could be opened with a finger however it needed a key to close it. http://youtu.be/LGq2TUQ01zk?t=2m15s (dubbed in Hungarian but you can see it).

> So the issue remains that sub-tests of MigrateDrupal6Test don't get their setUp methods run.
> Not sure if that is by design.

It is, that's the point of the whole thing: single tests call D8 APIs in their setUp to make a test which tests a given migration (and allows easier debug). MigrateDrupal6Test is about the integration of it all.

I guess the site_name variable will always exist (cos it's on the installer form), good call.

Edit: menu_expanded, menu_masks, css_js_query_string and clean_url are other good choices. I would include a few of them just to be sure. Variable always returns a single row so it's not like adding more variables will break you.

The set destination property is only called on the stub. Is that OK?

Fixed #82 and #83
Some more reviews please? Lets get this in and knock off another beta-blocker, in the process unlocking some more comment module issues.

1. +++ b/core/modules/comment/comment.install
   @@ -67,12 +67,18 @@ function comment_schema() {
            'type' => 'varchar',
            'not null' => TRUE,
   -        'default' => 'node.comment',
   +        'default' => 'comment',
            'length' => 255,
   

   This shouldn't have a default, #2183231: Make ContentEntityDatabaseStorage generate static database schemas for content entities will do away with them, and while that will remove the schema, removing it now might catch possible troubles early.

2. +++ b/core/modules/comment/comment.module
   @@ -100,26 +101,10 @@ function comment_help($route_name, Request $request) {
   +  foreach (entity_load_multiple('comment_type') as $comment_type) {
   +    $bundles['comment'][$comment_type->id] = array(
   +      'label' => $comment_type->label,
   +    );
      }
      return $bundles;
   

   You should be able to remove this completely now.

3. +++ b/core/modules/comment/comment.module
   @@ -142,23 +127,21 @@ function comment_uri(CommentInterface $comment) {
   +  foreach (entity_load_multiple('comment_type') as $comment_type) {
   

   Use CommentType::loadMultiple()?

4. +++ b/core/modules/comment/comment.module
   @@ -572,10 +540,13 @@ function comment_node_view_alter(array &$build, EntityInterface $node, EntityVie
   +  /** @var \Drupal\field\FieldConfigInterface $field */
   +  $field = Fieldconfig::loadByName($entity->getEntityTypeId(), $field_name);
   

   @var should no longer be needed.

5. +++ b/core/modules/comment/lib/Drupal/comment/CommentForm.php
   @@ -215,7 +215,7 @@ public function form(array $form, array &$form_state) {
    
        // Add internal comment properties.
        $original = $comment->getUntranslated();
   -    foreach (array('cid', 'pid', 'entity_id', 'entity_type', 'field_id', 'uid', 'langcode') as $key) {
   +    foreach (array('cid', 'pid', 'entity_id', 'entity_type', 'comment_type', 'field_name', 'uid', 'langcode') as $key) {
          $key_name = key($comment->$key->getFieldDefinition()->getPropertyDefinitions());
          $form[$key] = array('#type' => 'value', '#value' => $original->$key->{$key_name});
        }
   

   This whole thing is useless, it was needed when we built entity objects from scratch on form submission, we no longer do that. We already removed it from nodes.

6. +++ b/core/modules/comment/lib/Drupal/comment/CommentManager.php
   @@ -191,49 +210,38 @@ public function addBodyField($entity_type, $field_name) {
              'weight' => 0,
            ))
            ->save();
   -    }
   -  }
    
   -  /**
   -   * {@inheritdoc}
   -   */
   -  public function getFieldUIPageTitle($commented_entity_type, $field_name) {
   -    $field_info = $this->getFields($commented_entity_type);
   -    $sample_bundle = reset($field_info[$field_name]['bundles']);
   -    $sample_definition = $this->entityManager->getFieldDefinitions($commented_entity_type, $sample_bundle)[$field_name];
   -    return String::checkPlain($sample_definition->getLabel());
   +      $this->entityManager->clearCachedFieldDefinitions();
   +    }
   

   A bit hard to follow the context here, but why is this necessary now and wasn't before?

7. +++ b/core/modules/comment/lib/Drupal/comment/CommentTypeInterface.php
   @@ -0,0 +1,43 @@
   +   *
   +   * @return $this
   +   *
   

   Nitpick: unecessary space?

8. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -306,10 +299,14 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    if ($entity_type->id() == 'comment') {
   +      if ($comment_type = entity_load('comment_type', $bundle)) {
   

   I remember there was a problem with this, but what's the reason you do an entity type check here? This is only called for your class, the only difference would be if this class is used for multiple entity types.

   CommentType::load() :)

9. +++ b/core/modules/comment/lib/Drupal/comment/Entity/Comment.php
   @@ -505,8 +495,10 @@ public function getChangedTime() {
   +      /** @var \Drupal\field\FieldConfigInterface $field */
   +      $field = entity_load('field_config', $values['entity_type'] . '.' . $values['field_name']);
   

   FieldConfig::loadByName() and remove the @var...

10. +++ b/core/modules/comment/lib/Drupal/comment/Entity/CommentType.php
    @@ -0,0 +1,105 @@
    +  public $targetEntityTypeId;
    

    I have no idea why we use camel case sometimes and sometimes not, but I do prefer not using camel case on config entities.

11. +++ b/core/modules/comment/lib/Drupal/comment/Entity/CommentType.php
    @@ -0,0 +1,105 @@
    +    if (!$update && !$this->isSyncing()) {
    +      \Drupal::service('comment.manager')->addBodyField($this->id());
    +    }
    

    We've been discussing if this behavior is really correct and if we shouldn't do this in the UI only. For example, this makes it impossible to provide a default comment type that does not have this field (sync works, but not install). But we should probably keep it consistent and change it everywhere together.

12. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php
    @@ -30,7 +31,7 @@ class CommentItem extends FieldItemBase implements CommentItemInterface {
         return array(
    -      'description' => '',
    +      'comment_type' => 'new',
         ) + parent::defaultSettings();
    

    can't we use an empty string or something that's not a valid config entity type ID for this, would be very easy to mess with this if you create a comment type named "new" ? ;)

13. +++ b/core/modules/comment/lib/Drupal/comment/Plugin/Field/FieldType/CommentItem.php
    @@ -194,41 +191,100 @@ public function isEmpty() {
    +    // @todo Inject storage controller once typed-data supports container
    +    //   injection.
    +    $comment_types = entity_load_multiple('comment_type');
    

    storage controller => entity storage or something, we eventually want to avoid the term "controller", and until then, CommentType::loadMultiple() :)

Fixes #86
With #86.5 made it consistent with node, which meant adding language selector field to get language tests to pass.

@larowlan asked me to look at the last interdiff. The language selector looks sane :) I should do some manual testing when I have time to see it works as good as any other entity language selector.

87: comment-type-2228763-2.ebe82dd.patch queued for re-testing.

Re-roll for field_info_instance()

Rerolled for PSR-4.

Creating a new comment type on the field settings form results in an error.

Clicking the "enable language support" link on the field settings form results in an SQL error, but this is an unrelated error. I verified that a text field settings form will lead to the same error. I logged this separately.

#2275905: Clicking the enable language support link for a field settings form results in an SQL Insert error

aha, I think this is a Drupal States issue and something I'm quite suited to fix :)

Alright, fixed that. The select option was missing a value, so the States code was not firing against it.

I didn't update the states code, another one where we need front end testing.
The states code must still ref 'new', needs to be ''
I'm away till Monday so if someone else is able to reroll with that fix?

See 86.12

Good point about the possible config entity name clash larowlan. I hadn't thought that an empty string would work so I didn't even bother trying, but it turns out it does work. So here's the reroll with the empty string.

Thank you @jessebeach! I think we're close now...

I reviewed language + language selector + translation support on this one and it does work well. I found #2276387: Translate routes should properly inherit admin path use from edit route on the way (and some other bugs that I need to see if I can reproduce), but those are not related to the patch. The only actual issue I found with the patch is the default visible bundle name: "comment" is totally not in line with all the rest of the config entity labels which are capitalized. The lowercase option looks odd when selecting a comment type. Users are very likely naming their type uppercase and the "Add new comment type" is also uppercase. Should be an easy fix.

BTW I also found out that the right translation of the comment was not showing when switching language but that is *very* likely has nothing to do with this patch as comment translation is a previously available feature.

Fixes case and merges against #2259243: Database schema of content entities is automatically generated based on entity type and field definitions
Not sure how successful that will be, fingers crossed.

Edit: sorry, commented on the wrong comment beta blocker. ;)

How about this

109: comment-type-2228763-2.57ac5bb.patch queued for re-testing.

field name is no longer calculated
getting some uber-screwy fails locally, but I think they're from my drive, which is on its last legs

Exciting, this is getting close! Can we get:

1. A confirmation that the API changes listed in the summary are the only public API changes
2. A quick search of existing change records for the methods being changed (e.g. is the original comment-as-field change record affected?) and then an update of those change records to add this issue to their list of issue references, so we can update them if/when this goes in.

Updated #2100015: Comment settings are now a field. Comments allowed on any entity type. to reference this.
Added full list of API changes thus:

• FieldItemInterface::settingsForm is passed $form by reference
• \Drupal\comment\CommentFieldNameItem removed
• \Drupal\comment\CommentFieldNameValue removed
• \Drupal\comment\CommentInterface::getFieldId() removed
• \Drupal\comment\CommentInterface::setFieldId() removed
• \Drupal\comment\CommentInterface::setFieldName added
• \Drupal\comment\CommentInterface::getTypeId() added
• \Drupal\comment\Controller\AdminController::overviewBundles() removed
• \Drupal\comment\CommentManagerInterface::getFieldUIPageTitle() removed
• \Drupal\comment\CommentManagerInterface::addDefaultField now accepts 5th optional argument, comment type ID
• \Drupal\comment\CommentManagerInterface::addBodyField arguments changed from entity type and field name, to comment type id
• \Drupal\comment\Routing\CommentBundleEnhancer and associated test removed

More reviews please. This is one of the fifteen beta blockers, let's get it in.

So, this is minor, but it would be nice if the naming was consistent. I see e.g.

+   * @param string $comment_type_id
+   *   (optional) ID of comment type to use. Defaults to 'comment'.
...
+   * @param string $comment_type
+   *   The comment bundle.

So, the same thing is called the "comment type ID", "comment type", and "comment bundle"? I guess this kind of mismash is all over core though.

This line in CommentStatistics seems a little funny, since it's hard-coding the default field name only?

+          'on' => "ces.entity_id = i.sid AND ces.entity_type = 'node' AND ces.field_name = 'comment'",

Did some manual testing, looks like the "Create new" selector when creating a new field is still broken, doesn't display anything and then fails validation.

It does work when disabling javascript. Everything else worked fine when I tested it (didn't test advanced stuff like search and views integration, though), leaving the tag for the #states stuff for now, can be removed once that works IMHO. We already know that search integration only works for the default comment type, that was the case before, I'm OK with a follow-up there, see below.

1. +++ b/core/modules/comment/comment.install
   @@ -49,12 +49,12 @@ function comment_schema() {
          ),
   -      'field_id' => array(
   +      'field_name' => array(
            'type' => 'varchar',
            'not null' => TRUE,
   -        'default' => 'node__comment',
   +        'default' => '',
            'length' => 255,
   -        'description' => 'The field_id of the field that was used to add this comment.',
   +        'description' => 'The field_name of the field that was used to add this comment.',
          ),
   

   Missed this before but field names are limited to 32 characters, which means that you can limit it here as well and further simplify the primary key below.

2. +++ b/core/modules/comment/src/CommentManager.php
   @@ -191,32 +211,29 @@ public function addBodyField($entity_type, $field_name) {
   +    $field_instances = $this->entityManager
   +      ->getFieldDefinitions('comment', $comment_type_id);
   +    if (empty($field_instances['comment_body'])) {
   

   You should use FieldInstanceConfig::loadByName() here, because you're really looking for configurable fields explicitly, not any field definition. Should also be a bit easier.

3. +++ b/core/modules/comment/src/CommentStatistics.php
   @@ -157,7 +157,7 @@ public function getRankingInfo() {
              // Default to comment field as this is the most common use case for
              // nodes.
   -          'on' => "ces.entity_id = i.sid AND ces.entity_type = 'node' AND ces.field_id = 'node__comment'",
   +          'on' => "ces.entity_id = i.sid AND ces.entity_type = 'node' AND ces.field_name = 'comment'",
            ),
   

   As discussed before, this is not a problem of this patch as it was hardcoded before but we should create a follow-up issue for this.

4. +++ b/core/modules/comment/src/CommentStorage.php
   @@ -149,7 +148,7 @@ public function getSchema() {
            'entity_id',
            array('entity_type', 32),
   -        array('field_id', 32),
   +        array('comment_type', EntityTypeInterface::BUNDLE_MAX_LENGTH),
            'status',
            'created',
   

   I think we lost the improvements here that we made with the entity_type length and bundle length should now be correct automatically if it's defined as an entity reference to the config entity type.

5. +++ b/core/modules/comment/src/CommentTypeListBuilder.php
   @@ -0,0 +1,91 @@
   +    $row['type'] = $this->linkGenerator->generateFromUrl($entity->label(), $entity->urlInfo());
   

   Do we really want to make this a link? It points to the edit form, that we already explicitly hide below edit fields operations.

   Node type labels are not linked either, I think it's common that if those are linked, they lead to a view page?

6. +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -285,20 +285,15 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
   +    $fields['comment_type'] = FieldDefinition::create('string')
   +      ->setLabel(t('Comment Type'))
   +      ->setDescription(t('The comment type.'))
   +      ->setSetting('max_length', EntityTypeInterface::BUNDLE_MAX_LENGTH);
   

   Ah, you're not doing this yet, I think this should be entity_reference, with a target_type setting, then the max_length will be correct automatically.

7. +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -285,20 +285,15 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
        $fields['field_name'] = FieldDefinition::create('string')
          ->setLabel(t('Comment field name'))
          ->setDescription(t('The field name through which this comment was added.'))
   -      ->setComputed(TRUE);
   -
   -    $item_definition = $fields['field_name']->getItemDefinition();
   -    $item_definition->setClass('\Drupal\comment\CommentFieldNameItem');
   -    $fields['field_name']->setItemDefinition($item_definition);
   +      ->setSetting('max_length', EntityTypeInterface::BUNDLE_MAX_LENGTH);
   

   While it's technically the same value, I'm not sure if it's correct to use that constant here too.

8. +++ b/core/modules/comment/src/Entity/Comment.php
   @@ -541,4 +532,14 @@ public function setOwner(UserInterface $account) {
   +   * Get the comment type id for this comment.
   +   *
   +   * @return string
   +   *   The id of the comment type.
   +   */
   

   id should be ID

9. +++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php
   @@ -194,41 +192,99 @@ public function isEmpty() {
   +      '#states' => array(
   +        'visible' => array(
   +          ':input[name="field[settings][comment_type]"]' => array('value' => 'new'),
   +        ),
   +      ),
   

   #states to make the fields below required would be nice, not that important.

10. +++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php
    @@ -194,41 +192,99 @@ public function isEmpty() {
    +      // @todo Inject this once typed-data supports ContainerInjectionInterface.
    +      $comment_type = entity_create('comment_type', $settings['new_comment_type']);
    

    You can use CommentType::create() now for this.

11. +++ b/core/modules/comment/src/Tests/CommentTypeTest.php
    @@ -0,0 +1,202 @@
    +    $node = entity_create('node', array(
    +      'type' => 'page',
    +      'title' => 'foo',
    +    ));
    +    $node->save();
    +
    +    // Add a new comment of this type.
    +    $comment = entity_create('comment', array(
    

    This could also use the new ::create() and other new code too.

12. +++ b/core/modules/comment/src/Tests/CommentTypeTest.php
    @@ -0,0 +1,202 @@
    +  protected function createCommentType($label) {
    

    Could we add this to CommentTestBase, then other tests can use it too

13. +++ b/core/modules/text/src/Plugin/Field/FieldType/TextItem.php
    index 0000000..ceef688
    --- /dev/null
    
    --- /dev/null
    +++ b/core/profiles/standard/config/install/comment.type.comment.yml
    
    +++ b/core/profiles/standard/config/install/comment.type.comment.yml
    +++ b/core/profiles/standard/config/install/comment.type.comment.yml
    @@ -0,0 +1,9 @@
    
    @@ -0,0 +1,9 @@
    +id: comment
    +label: Comment
    +description: 'Default comment field'
    +target_entity_type_id: node
    +status: true
    +langcode: en
    +dependencies:
    +  entity:
    +    - comment.type.comment
    

    this depends on itself? Not sure what it has to depend on, probably only the node and comment modules?

    I'm not sure if "comment" makes sense as a default type, would "default" make more sense? Would be ugly to change now I guess, so don't do it just based on my opinion. It's different to before (node_comment) because then it was a combination of entity type and field name.

    We still have some minor bugs if bundle == entity_type, for example the form ID is inconsistent.

    Description says default comment field, field => type?

@pwolanin: Yeah, we're consistently inconsistent with bundle/type/type ID, but we should try to unify to type ID as much as possible if we talk about the ID.

Yup, 'max_length' setting on entity_ref fields is obsolete and has no effect.

See #2278051: Remove uses of non-existing 'max_length' setting on EntityRef fields - let's not add ne ones here :-)

So the states issue is that we're using '' as the key for 'new'.
We need to use something that doesn't evaluate to FALSE but is also an invalid config entity key, so how about '-1'?

Not sure if I'll have time to re-roll this today, but if someone else does, essentially we want the fixes required from #118 and then using the interdiff from #88, reverse the change from 'new' => '' and instead make it '-1'

Just a thought, I don't think it matters much what exactly we use, but what about something like ':new'? And preferably put that in a constant (CommentTypeInterface::NEW_PLACEHOLDER for example?).

I'm going to do this reroll at DrupalCon Austin

Fixes #118

Left the default bundle as 'comment'

Sorry for reviewing this so late. Sadly in Drupal 8 its not uncommon that we don't review far reaching UX changes anymore. By doing it this late, it ends up as followup (and never gets fixed) or requires stripping out stuff people worked on.

The only thing that I found truly weird and shouldn't be in this patch is the inline adding of new comment types. I don't understand why we should be doing that for this UI, we don't do it for any of the other fields. From my point of view, that should be removed before RTBC.

In general the code looks solid. I've done om some manual testing and below is my feedback (most of which can be dealt with in follow-ups I believe).

1. +++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php
   @@ -194,41 +193,99 @@ public function isEmpty() {
   +        'visible' => array(
   +          ':input[name="field[settings][comment_type]"]' => array('value' => 'new'),
   +        ),
   

   This needs to use the new CommentTypeInterface::NEW_PLACEHOLDER constant. The #states doesn't work on the comment field settings page because of this...

   OR

   ...we completely skip the whole "Create new" workflow from this form and just rely on adding comment types from the main comment types screens.

2. Follow-up: We probably enable per comment type permissions, similar to how content type permissions work.

3. Follow-up: We need to be able to override the various comment templates (comment.html.twig, comment-wrapper.html.twig, more?) per comment type

4. 

   Follow-up: At the moment the comment form heading always says "Add new comment" on the form. If you have a specific comment type like "Greeting" you probably want it to say "Write greeting" instead. I see two ways this can be solved:

   1. Provide theme suggestions to per comment type so one can hard-code the titles (we probably should do this anyhow as per the previous point).
   2. Provide settings on the comment type config entity, similar to "Submission form settings" for content types where you can configure 1) the list heading (defaults to "Comments") and 2) the form heading (defaults to "Add new comment").

Edit: Let's try that again

126 I respectfully disagree. Having to create the comment type first is a far worse UX.

127.1 yep
127.2-4 we have issues for these, none of them are beta blockers.

Follow-ups created. They need more details, but at least they're there.

Edit: Sorry, cross-post. I missed we had those.

We already have these as follow ups from when comment-field first went in.
Please remove the duplicates.

126 - what if we meet half way - a link to add a new comment-type that loads in a dialog, then you don't loose where you are if you need to create one?

The existing follow ups are
#1903138: Move global comment permissions to comment-type level
#1920044: Move comment field settings that relate to rendering to formatter options
#1962846: Use field instance name for header of comment list, drop comment-wrapper template
#2031883: Markup for: comment.html.twig

These are tracked in our meta existing comment-module path to beta meta - #2191565: [META] Comment/Forum/History path to beta

Closed duplicates

Added related

Adding tag as per @bojhan request.

Usability question to ask - can we remove the 'create new comment-type' inline form, and replace it with an 'add new comment type' link in the description of the select field, that loads the add new comment-type form in a modal, then refreshes the page with the new value.

Usability question to ask - can we remove the 'create new comment-type' inline form, and replace it with an 'add new comment type' link in the description of the select field, that loads the add new comment-type form in a modal, then refreshes the page with the new value.

It's of course up to @Bojhan to make a call on this, but IMO considering...

1. this issue is a beta blocker
2. the rest of the code is very close
3. that "create new" pattern is not used anywhere else in core right now

... I'd say we should keep it simple and just put a link in the description text to the comment type administration screen as @Bojhan suggested.

Any further UX improvements (e.g. modal pop-up) can easily be introduced later without breaking any BC.

I dont think it makes much sense to expose this in a modal. I am not sure a compromise helps our end-users. If you look at the bigger picture you will see that this patch introduces a UI concept that is not used by any of the other fields (inline adding of "types"). This will mean an inconsistency, which makes it harder for users to have an expectation about what they can do in the Field UI and where with the Drupal UI. I understand that there is a disconnect between the two UI's, but it doesn't help to have two places to create comment types.

If you want to introduce a concept like this, I think its best done in a follow-up or separate issue not in the beta blocker. Because it might work for other fields, or it might not - we don't know. We don't typically do interactions like this in the Drupal UI. I hope this helps to put some background to why I am suggesting this. I am sadly going to be traveling for a bit, back in a few days.

I wanted to suggest that initially as well, but the problem is that you are in the middle of the create-a-field process and clicking on that link will abort that and leave your field in an incomplete and broken state, and you then either have to delete and re-create it or remember to go back to the field settings and then the instance settings.

Also, you probably tested this with a comment field on nodes with the standard profile, which have a default comment type, so it's not really a problem there, but if you for example try to add a comment field to terms, there is no default and you can not continue without creating one first.

I don't think just a link is enough, if you're strongly against the "Create new", then what about automatically creating a default comment type if there is none and you get to that settings form?

Similar ux issue here #1433796: Link to images styles from image field display settings

Perhaps we could do some AB testing here, although berdir's comment about broken site state shouldn't be ignored

but the problem is that you are in the middle of the create-a-field process and clicking on that link will abort that and leave your field in an incomplete and broken state

Very true. Note that once #2277941: Allow injecting an arbitrary FieldConfig when building a FieldInstanceConfig is in, though, Field UI could be modified to *not* create a FieldConfig until the moment you actually save the "field settings" form (or even the "instance settings form").

That the field config may be in an inconsistent state is an existing problem. If your network goes out or your browser crashes or your computer runs out of power, etc. A views-like solution where everything is temporary until actually saved sounds good, but I think this would solve an existing problem and therefore could be deferred. My 2 cents.

OK, so I think we've settled on just remove the inline stuff
Will do

Removes the inline stuff

Fixes the new comments-not-allowed-on-entities-with-string-ids test

Fixes other UX issues at #126

Missed a use during re-roll

This looks really solid now. The only nit-pick I have before I believe this one is RTBC is:

+++ b/core/modules/comment/src/Plugin/Field/FieldType/CommentItem.php
@@ -194,39 +193,27 @@ public function isEmpty() {
+    // @todo Inject entity storage once typed-data supports container injection.
+    $comment_types = CommentType::loadMultiple();

We should point to that issue - #2053415: Allow typed data plugins to receive injected dependencies

added url suggested by 152

Thanks @martin107!

I believe this one is ready now.

Good to see this RTBC, a few minor things though:

1. +++ b/core/modules/comment/src/CommentTypeForm.php
   @@ -0,0 +1,141 @@
   + * Base form controller for category edit forms.
   + */
   +class CommentTypeForm extends EntityForm implements ContainerInjectionInterface {
   +
   

   This already implements ContainerInjectionInterface via EntityForm - doesn't need to specify that again.

2. +++ b/core/modules/comment/src/Form/CommentTypeDeleteForm.php
   @@ -0,0 +1,138 @@
   +    drupal_set_message(t('comment type %label has been deleted.', array('%label' => $this->entity->label())));
   +    watchdog('comment', 'comment type %label has been deleted.', array('%label' => $this->entity->label()), WATCHDOG_NOTICE);
   +  }
   

   Should use Drupal::logger() now instead of watchdog or inject the logger service.

3. +++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_comment_type.yml
   @@ -0,0 +1,23 @@
   +  variables:
   +  # We just need a single one of these, include a few to be sure.
   +    - site_name
   +    - menu_expanded
   +    - menu_masks
   +    - css_js_query_string
   +    - clean_url
   +  constants:
   

   Is this a copy and paste error?

4. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCommentTypeTest.php
   @@ -0,0 +1,58 @@
   + * Tests the Drupal 6 to Drupal 6 comment type migration.
   

   Drupal 6 to Drupal 8.

5. +++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCommentTypeTest.php
   @@ -0,0 +1,58 @@
   +
   +  /**
   +   * Tests the Drupal 6 to Drupal 6 comment type migration.
   +   */
   

   Drupal 6 to Drupal 8.

#155.1 fixed

#155.2 fixed
I went the dependency injection route.
It strikes me that the logger should be optional in the constructors as this would be more flexible in the future, other can comment
BUT I erred on the side of caution and implemented the functionally equivalent form of what was there as this was RTBC

#155.3 I am leaving this for others to comment

#155.4 fixed

#155.5 fixed

Looks good to me.

(As a side note that logger freaked me out for translatability. Opened #2285063: Support for Drupal 8 logger API. Not related to this issue per say...)

Noticed that we never were testing pressing the button on the comment type delete form. Also noticed a few minor things to tidy up.

+++ b/core/modules/comment/src/CommentManager.php
@@ -103,9 +106,27 @@ public function getAllFields() {
+        throw new \InvalidArgumentException($this->t('The given comment type id %id can only be used with the %entity_type entity type', array(
+          '%id' => $comment_type_id,
+          '%entity_type' => $entity_type,

Exceptions shouldn't use t(). Not ever.

+++ b/core/modules/migrate_drupal/config/install/migrate.migration.d6_comment_type.yml
@@ -3,7 +3,9 @@ label: Drupal 6 comment type
+  # Ensure that we get at least one row to process by using variables that exist
+  # on every Drupal 6 site. We just need a single one of these, include a few to
+  # be sure.

Grammar nitpick: The second sentence either needs a conjunction, or to become two sentences.

Just doing a deeper review.

CommentManager was even already using String. :)

This looks great - just some more tidy up of unused uses, spelling and using a constant rather than hard coding.

Assigning back to catch.

Committed/pushed to 8.x, thanks!

yeah!
/me unpostpones other issues

Change notice is here https://drupal.org/node/2285633

Please review so we can publish.

Search API D8 Branch broke due to this. We're trying to dig where the problem could be but the change record wasn't as useful as I hoped it would be. We'll review it and make improvements as soon as we have it working again

Actually, there is a change record, and it is published. :) It's a little confusing, but it's also not a 7-to-8 BC break. So long as the main change record for the original comment change is up-to-date for this issue, I think we can mark this fixed and improve the change record as needed.

Created a followed on this to unify URLs of "xx-type" handling (content-types and comment-types). See:
#2501691: Change content-types, comment-types, and block-types URLs

The followup for @dixon in #127/4 was not opened if I see it right. Opened #2546116: Allow a sitebuilder to change the Comment form heading. Please close if duplicate of something.