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.

Files: 
CommentFileSizeAuthor
#18 2374125.18.patch15.99 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,735 pass(es). View
#17 2374125.17.patch16 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,198 pass(es). View
#17 14-17-interdiff.txt2.02 KBalexpott
#14 2374125.14.patch17.34 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,235 pass(es), 1 fail(s), and 0 exception(s). View
#14 10-14-interdiff.txt6.19 KBalexpott
#10 8-10-interdiff.txt2.45 KBalexpott
#10 2374125.10.patch19.79 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,206 pass(es). View
#8 2374125.8.patch19.08 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,202 pass(es), 16 fail(s), and 0 exception(s). View
#8 6-8-interdiff.txt4.44 KBalexpott
#6 2374125.6.patch16.34 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,200 pass(es), 4 fail(s), and 4 exception(s). View
#6 3-6-interdiff.txt2.82 KBalexpott
#3 2374125.3.patch13.52 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,184 pass(es), 14 fail(s), and 12 exception(s). View
#3 1-3-interdiff.txt10.01 KBalexpott
#1 2374125.1.patch5.83 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,139 pass(es), 14 fail(s), and 12 exception(s). View

Comments

alexpott’s picture

Status: Needs work » Needs review
FileSize
5.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,139 pass(es), 14 fail(s), and 12 exception(s). View

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

Status: Needs work » Needs review
FileSize
10.01 KB
13.52 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,184 pass(es), 14 fail(s), and 12 exception(s). View

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

Component: comment.module » custom_block.module

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

alexpott’s picture

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

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

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

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

Status: Needs work » Needs review
FileSize
19.79 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,206 pass(es). View
2.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.

larowlan’s picture

Assigned: Unassigned » larowlan

Putting on my list to review

yched’s picture

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

FileSize
6.19 KB
17.34 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,235 pass(es), 1 fail(s), and 0 exception(s). View
  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

@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

Status: Needs work » Needs review
FileSize
2.02 KB
16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,198 pass(es). View

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

alexpott’s picture

FileSize
15.99 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,735 pass(es). View

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

Status: Needs review » Reviewed & tested by the community

Can't see anything wrong with this - thanks!

catch’s picture

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.