Create a persistent comment body field storage

Do we have a similar issue for block_content bodies ?

I think there's an existing issue to move comment field for forum to cmi, we should close as duplicate of this, leave it with me

The body field needed creating in the test since it is no longer automatically created when the comment type is created. Patch attached should fix migrate fails.

+++ b/core/modules/comment/src/CommentTypeForm.php
@@ -156,6 +167,7 @@ public function save(array $form, FormStateInterface $form_state) {
+      $this->commentManager->addBodyField($comment_type->id());

+++ b/core/modules/comment/src/Entity/CommentType.php
@@ -92,14 +92,4 @@ public function getTargetEntityTypeId() {
-  /**
-   * {@inheritdoc}
-   */
-  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
-    parent::postSave($storage, $update);
-    if (!$update && !$this->isSyncing()) {
-      \Drupal::service('comment.manager')->addBodyField($this->id());
-    }
-  }

So this moves the logic from the Entity to the submit handler. What happens if you (for example) create a comment-type using the API - or via REST. Are we sure we want to go this way? Logic in form submissions handlers is something we've been moving away from.

Also given how much cruft is added to work around #2321385: Creation of node body field in postSave() incompatible with default config and overrides - I think it would be worth postponing this one on that?

Just read #2321385: Creation of node body field in postSave() incompatible with default config and overrides and realise my comment above is contra to that - i.e. creating fields in post-save is bad. But I'm not sure creating them via the form is that hot either. How about a check-box in the form 'Add body field' that if checked adds the field? Still comes down to logic in the form - but makes it clear that its associated with a given form, rather than a particular API operation.

Of course feel free to say 'shut up larowlan and just rtbc the damn patch' :)

I also don't like the pattern of doing stuff like this in form submit handlers, it caused an issue with the book module as well #2363647: Cannot programatically update books. Migrate is particularly good at finding bugs that are hidden by form submit handlers.

The checkbox is a nice idea, conceptually I think that does feel better.

MigrateDrupal6Test will still fail because it runs all the migrations without their setup methods. Lets add a CommentType destination and add the body field there.

Unless bot disagrees, I'm happy with this - committers - please put #2321385: Creation of node body field in postSave() incompatible with default config and overrides in before this (also rtbc) and then this can be re-rolled.

@alexpott++

In regards to #22, I think it's clearer if the form handlers don't make assumptions about what you need.

For example, if I'm using the UI to create comment types and I always get a body field and then I create one from a REST call, i'd be Googling, "Why doesn't body field get created from REST call". However, if it was a separate action, like the checkbox larowlan mentioned, or you had to manually add the field, I think that would then register with me that it was two actions and i'd need to handle the body field creation myself.

Not trying to hold this issue up, just wanted to share my thoughts on this. The last patch should still fail until we add a CommentType destination that creates the body/default field.

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCommentTest.php
@@ -28,11 +28,7 @@ protected function setUp() {
     $this->container->get('entity.manager')->getStorage('comment_type')->create(array(
       'id' => 'comment_no_subject',
       'label' => 'comment_no_subject',

How come we don't also replace the second instance with an addDefaultField/addBodyField call?

+++ b/core/modules/migrate_drupal/src/Tests/d6/MigrateCommentTest.php
@@ -28,11 +28,7 @@ protected function setUp() {
+    \Drupal::service('comment.manager')->addDefaultField('node', 'story');

I don't quite understand how adding this to setUp fixed MigrateDrupal6Test, is it possible something else also changed that isn't in the interdiff for #21? My patch in #15 had failures only in MigrateDrupal6Test which does not call setUp for tests. Your interdiff shows a change only in setUp but then the tests pass?

+++ b/core/modules/migrate/src/Plugin/migrate/destination/EntityCommentType.php
@@ -0,0 +1,28 @@
+    \Drupal::service('comment.manager')->addBodyField(reset($entity_ids));

Can we inject this like we do for the UrlAlias destination? Otherwise the rest looks much more like what I was expecting. RTBC for the migrate stuff.

Great cleanup, only minor remarks :

1. +++ b/core/modules/comment/src/Tests/CommentPreviewTest.php
   @@ -28,6 +28,48 @@ class CommentPreviewTest extends CommentTestBase {
   +  protected function setUp() {
   

   Minor, but the line spacing groups code blocks in a way that obscures what is being done (and code comments do not really help) :

   [default form mode]
   
   [other form modes]
   [default view mode]
   [other view modes]
   

   & Same in the duplicated code in StandardProfileTest, StandardTest, UserPictureTest.

   Also, maybe I'm missing something, but those all have a @todo "Remove after #2321385: Creation of node body field in postSave() incompatible with default config and overrides", but not clear how that issue there about node body will let us remove that code about comments here. Should we commit that other issue first to see clearer ?

2. +++ b/core/modules/comment/src/Tests/CommentTypeTest.php
   @@ -109,14 +109,14 @@ public function testCommentTypeEditing() {
        $this->assertUrl(\Drupal::url('field_ui.overview_comment', array('comment_type' => 'comment'), array('absolute' => TRUE)), [], 'Original machine name was used in URL.');
   -
   +    $this->assertTrue($this->cssSelect('tr#comment-body'), 'Body field exists.');
        // Remove the body field.
   

   Can we preserve the line spacing that was there ?

3. +++ b/core/modules/hal/src/Tests/NormalizerTestBase.php
   @@ -64,19 +64,22 @@ protected function setUp() {
   -          if (in_array('node', $class::$modules, TRUE)) {
   -            $this->installEntitySchema('node');
   +          $entity_schema_to_install = array_intersect(array('node', 'comment'), $class::$modules);
   +          if (!empty($entity_schema_to_install)) {
   +            foreach($entity_schema_to_install as $module) {
   +              $this->installEntitySchema($module);
   +            }
   

   OK, it was expected that this code would become more complex once we deal with comment bodies (custom blocks to come), but ouch...

   Side note:
   - missing space after foreach
   - could be simplified to:

   foreach (array_intersect(...)) {
     $this->installEntitySchema($module);
   }
   

   (and same in EntityUnitTestBase)

Also, maybe I'm missing something, but those all have a @todo "Remove after #2321385: Creation of node body field in postSave() incompatible with default config and overrides", but not clear how that issue there about node body will let us remove that code about comments here. Should we commit that other issue first to see clearer ?

That issue just got committed, so looks like we can take care of the @todo now ?

Side note about #30.1 : this code was copy/pasted from CommentManager::addDefaultField(), which could use some cleanup itself. Patch posted in #2376581: Cleanup CommentManager::addDefaultField().

Thanks @alexpott.

Code remarks in #30.3 still apply though - at least the missing space for code standards :-)

Thanks !

Fixes a critical so fine per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?.

Committed/pushed to 8.0.x, thanks!