We can now persist field storages. We should not create comment body field storage in code since that means it is possible to deploy a field storage different from the type assumed in CommentManager::addBodyField(). It also means that contrib or custom modules can rely on the field storage being present. See #1426804: Allow field storages to be persisted when they have no fields.

Files: 
CommentFileSizeAuthor
#35 2374087.35.patch27.85 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,154 pass(es). View
#35 33-35-interdiff.txt1.91 KBalexpott
#33 2374087.33.patch28.1 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,170 pass(es). View
#33 28-33-interdiff.txt9.7 KBalexpott
#28 2374087.28.patch35.5 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,126 pass(es). View
#28 13-28-interdiff.txt2.34 KBalexpott
#21 2321385-new-body-storage.21.patch43.38 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,118 pass(es). View
#21 13-21-interdiff.txt1.23 KBalexpott
#15 interdiff.txt681 bytesbenjy
#15 2374087-15.patch33.82 KBbenjy
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,119 pass(es), 2 fail(s), and 2 exception(s). View
#13 2374087.13.patch33.16 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,167 pass(es), 4 fail(s), and 4 exception(s). View
#13 10-13-interdiff.txt7.6 KBalexpott
#10 2374087.10.patch25.56 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,171 pass(es), 28 fail(s), and 4 exception(s). View
#10 8-10-interdiff.txt1006 bytesalexpott
#8 2374087.8.patch25.63 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View
#8 5-8-interdiff.txt14.47 KBalexpott
#5 3-5-interdiff.txt1.25 KBalexpott
#5 2374087.5.patch11.15 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,164 pass(es). View
#3 2374087.3.patch9.9 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,202 pass(es), 1 fail(s), and 0 exception(s). View
#3 1-3-interdiff.txt7.62 KBalexpott
#1 2374087.1.patch3.05 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,209 pass(es), 6 fail(s), and 5 exception(s). View

Comments

alexpott’s picture

Status: Active » Needs review
FileSize
3.05 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 80,209 pass(es), 6 fail(s), and 5 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1: 2374087.1.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.62 KB
9.9 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,202 pass(es), 1 fail(s), and 0 exception(s). View

Fixing the tests.

Status: Needs review » Needs work

The last submitted patch, 3: 2374087.3.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
11.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,164 pass(es). View
1.25 KB

Final test fix

yched’s picture

Do we have a similar issue for block_content bodies ?

alexpott’s picture

alexpott’s picture

FileSize
14.47 KB
25.63 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] Drupal installation failed. View

Let's also remove the automatic creation of body fields when you create a type and remove the addition of comment fields to forum and article. CMI all the way. I think we need #2321385: Creation of node body field in postSave() incompatible with default config and overrides to land first so that everything can be in CMI

Status: Needs review » Needs work

The last submitted patch, 8: 2374087.8.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
1006 bytes
25.56 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,171 pass(es), 28 fail(s), and 4 exception(s). View

Weird we had a file called rdf.mapping.comment.node__comment.yml but the ID was comment.comment which should result in files with the name rdf.mapping.comment.comment.yml - I think this is a hang up of pre-comment type days.

Status: Needs review » Needs work

The last submitted patch, 10: 2374087.10.patch, failed testing.

larowlan’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.6 KB
33.16 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,167 pass(es), 4 fail(s), and 4 exception(s). View

Can't fix the migrate tests - the comment_body field is being added and nothing should have changed :(

This patch will get considerably smaller once #2321385: Creation of node body field in postSave() incompatible with default config and overrides lands.

Status: Needs review » Needs work

The last submitted patch, 13: 2374087.13.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
33.82 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,119 pass(es), 2 fail(s), and 2 exception(s). View
681 bytes

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.

larowlan’s picture

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

larowlan’s picture

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.

larowlan’s picture

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

benjy’s picture

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.

benjy’s picture

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.

alexpott’s picture

FileSize
1.23 KB
43.38 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,118 pass(es). View

Actually \Drupal::service('comment.manager')->addDefaultField('node', 'story'); should take care of this.

alexpott’s picture

Calling an API function like addDefaultField() or addBodyField from a form submit function is fine. Putting API logic in a submit function is not. Putting other config entity type creation logic inside another config entity is also not good. imo this is nothing like #2363647: Cannot programatically update books

larowlan’s picture

Status: Needs review » Reviewed & tested by the community

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

benjy’s picture

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.

The last submitted patch, 15: 2374087-15.patch, failed testing.

benjy’s picture

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

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

I went a bit mad #21 is mostly #2321385: Creation of node body field in postSave() incompatible with default config and overrides - so my changes do fix MigrateCommentTest but don't fix MigrateDrupal6Test (as @benjy expected - and so did I)

alexpott’s picture

Status: Needs work » Needs review
FileSize
2.34 KB
35.5 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,126 pass(es). View

Here's a fix for all the migrate test fails.

benjy’s picture

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

yched’s picture

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)

yched’s picture

Status: Needs review » Needs work

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 ?

yched’s picture

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

alexpott’s picture

Status: Needs work » Needs review
FileSize
9.7 KB
28.1 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,170 pass(es). View

Thanks for the review @yched

  1. All removed this works because now CMI has all the correct settings in the Standard profile :) - nothing in code anymore.
  2. Fixed
  3. Yeah but automatic table creation will fix this. And also content blocks don't need this - that patch is green and waiting for review - #2374125: Create a persistent block_content body field storage

The interdiff is post rebase with conflicts in forum.install.

yched’s picture

Thanks @alexpott.

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

alexpott’s picture

FileSize
1.91 KB
27.85 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,154 pass(es). View

Yep they do :)

yched’s picture

Status: Needs review » Reviewed & tested by the community

Thanks !

catch’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • catch committed 150cbcf on 8.0.x
    Issue #2374087 by alexpott, benjy: Fixed Create a persistent comment...

Status: Fixed » Closed (fixed)

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