Follow-up to #2374087: Create a persistent comment body field storage

We can now persist field storages. We should not create block_content body field storage in code since that means it is possible to deploy a field storage different from the type assumed in block_content_add_body_field(). 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.

CommentFileSizeAuthor
#18 2374125.18.patch15.99 KBalexpott
#17 2374125.17.patch16 KBalexpott
#17 14-17-interdiff.txt2.02 KBalexpott
#14 2374125.14.patch17.34 KBalexpott
#14 10-14-interdiff.txt6.19 KBalexpott
#10 8-10-interdiff.txt2.45 KBalexpott
#10 2374125.10.patch19.79 KBalexpott
#8 2374125.8.patch19.08 KBalexpott
#8 6-8-interdiff.txt4.44 KBalexpott
#6 2374125.6.patch16.34 KBalexpott
#6 3-6-interdiff.txt2.82 KBalexpott
#3 2374125.3.patch13.52 KBalexpott
#3 1-3-interdiff.txt10.01 KBalexpott
#1 2374125.1.patch5.83 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
2374125.1.patch5.83 KB

I've move the basic block content type to the Standard profile since this is inline with the Node module and it will fail with this patch because it tries to create the body field before the storage exists. I think we should go the whole way in this issue and remove...

  /**
   * {@inheritdoc}
   */
  public function postSave(EntityStorageInterface $storage, $update = TRUE) {
    parent::postSave($storage, $update);

    if (!$update && !$this->isSyncing()) {
      block_content_add_body_field($this->id);
    }
  }

From the BlockContentType entity and put this functionality in the UI. We should try to avoid all side effects in configuration entities - especially those that create other configuration entities.

Status: Needs review » Needs work

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
1-3-interdiff.txt10.01 KB
2374125.3.patch13.52 KB

We can stop adding body fields by default like so...

Status: Needs review » Needs work

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

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Component: comment.module » custom_block.module

The component name is outdated, but comment is certainly not correct either ;)

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
3-6-interdiff.txt2.82 KB
2374125.6.patch16.34 KB

Thanks @Berdir.

Here's a patch the fixes all the tests. For me the only remaining question is should be introduce (or keep) some generic functionality somewhere to add the body field to block content type. And if the answer is yes - where?

Status: Needs review » Needs work

The last submitted patch, 6: 2374125.6.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
6-8-interdiff.txt4.44 KB
2374125.8.patch19.08 KB

So it turns out migrate has a hard dependency on the basic block content type existing so we need to create one (and it's body field) through a migration. The migration tests are tricky but they caught this so huge++

Status: Needs review » Needs work

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

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
2374125.10.patch19.79 KB
8-10-interdiff.txt2.45 KB

So I think the schema for migrate.source.empty is a bit tricky - we should just ignore the constants here - anything could be in there. This can be used to create anything migrate can create.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Parent issue: #2374087: Create a persistent comment body field storage »
Related issues: +#1426804: Allow field storages to be persisted when they have no fields., +#2374087: Create a persistent comment body field storage
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Assigned: Unassigned » larowlan

Putting on my list to review

yched’s picture

yched CreditAttribution: yched commented

Looks like this needs a reroll - I reviewed on top of 55b8558.

  1. +++ b/core/modules/block_content/src/BlockContentTypeForm.php
@@ -106,4 +109,36 @@ public function save(array $form, FormStateInterface $form_state) {
+  protected function addBodyField(BlockContentTypeInterface $block_content_type) {

    So the three "body fields autocreated on 'bundle create' form submit" in core end up using three different patterns:
    - node body: a legacy node_add_body_field_function()
    - comment body: a public addBodyField() on CommentManager
    - block body: an internal helper method on the BlockContentTypeForm()
    That's a bit unfortunate ? 

  2. +++ b/core/modules/config_translation/src/Tests/ConfigTranslationListUiTest.php
@@ -389,6 +390,23 @@ public function doFieldListTest() {
+    // Create a block content type.
+    $block_content_type = entity_create('block_content_type', array(
+      'id' => 'basic',
+      'label' => 'Basic',
+      'revision' => FALSE
+    ));
+    $block_content_type->save();
+    $field = entity_create('field_config', array(
+      // The field storage is guaranteed to exist because it is supplied by the
+      // block_content module.
+      'field_storage' => FieldStorageConfig::loadByName('block_content', 'body'),
+      'bundle' => $block_content_type->id(),
+      'label' => 'Body',
+      'settings' => array('display_summary' => FALSE),
+    ));
+    $field->save();
+

    Unless I'm missing smthg, looks like having a body field isn't actually needed for that test ?

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
10-14-interdiff.txt6.19 KB
2374125.14.patch17.34 KB
  1. Yep you're right - reverted the removal of block_content_add_body_field() and used it everywhere. So now node and block_content are the same - and this set of related issues has not really changed anything wrt to the difference between them and comment.
  2. This test requires block_content to have a field - without this is does not have one.
yched’s picture

yched CreditAttribution: yched commented

@alexpott: thnks - then aren't there tests modified here that could simply use block_content_add_body_field() instead of receiving new verbose code ?

Other than that, looks fine to me, but I'll let @larowlan review.

Status: Needs review » Needs work

The last submitted patch, 14: 2374125.14.patch, failed testing.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Needs work » Needs review
FileSize
14-17-interdiff.txt2.02 KB
2374125.17.patch16 KB

Silly me. Didn't remove the the method on the form creating a body field.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
FileSize
2374125.18.patch15.99 KB

Rerolled - conflicted with #2346039: Add missing migrations to MigrateDrupal6Test and fix the result in core/modules/migrate_drupal/src/Tests/d6/MigrateDrupal6Test.php

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan commented
Status: Needs review » Reviewed & tested by the community

Can't see anything wrong with this - thanks!

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Reviewed & tested by the community » Fixed

Looks fine.

Committed/pushed to 8.0.x, thanks!

  • catch committed b313cc6 on 8.0.x 
    Issue #2374125 by alexpott: Create a persistent block_content body field...

Status: Fixed » Closed (fixed)

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