Problem/Motivation

When creating a new node type through the API (for example as default configuration import) the body field always gets attached. In the case of the default configuration a module ships this is problematic because this module can not provide different settings via field.instance.node.NODE_TYPE.body.yml.
This happens because the field instance is first created by the node system before the configuration is imported and then issue https://www.drupal.org/node/2140511 comes into effect and produces a fatal error.

Proposed resolution

  • Move the body field creation from the node type creation into the form logic.
  • Create node body field for provided types through CMI

Remaining tasks

Review

User interface changes

none

API changes

node types created directly through the API do not get the body field by default.

Files: 
CommentFileSizeAuthor
#58 interdiff.txt1.24 KBamateescu
#58 2321385-58.patch42.35 KBamateescu
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,120 pass(es). View
#57 2321385-new-body-storage.57.patch42.15 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,127 pass(es). View
#57 51-57-interdiff.txt510 bytesalexpott
#51 2321385-new-body-storage.51.patch42.16 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,239 pass(es). View
#51 49-51-interdiff.txt5.16 KBalexpott
#49 2321385-new-body-storage.49.patch42.24 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,176 pass(es). View
#49 47-49-interdiff.txt745 bytesalexpott
#47 2321385-new-body-storage.47.patch41.51 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,221 pass(es), 1 fail(s), and 1 exception(s). View
#28 2321385-new.28.patch32.83 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,101 pass(es), 2 fail(s), and 0 exception(s). View
#28 26-28-interdiff.txt4.14 KBalexpott
#25 2321385-25.patch32.67 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,834 pass(es), 101 fail(s), and 59 exception(s). View
#24 2321385-24.patch35.62 KBswentel
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,178 pass(es). View
#13 2321385.13.patch35.9 KBalexpott
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,686 pass(es). View
#7 5-7-interdiff.txt7.02 KBalexpott
#5 2321385.5.patch26.31 KBalexpott
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,968 pass(es), 40 fail(s), and 4 exception(s). View
#5 1-5-interdiff.txt20.61 KBalexpott
#1 node_body_out_of_api-2321385-1.patch5.7 KBbircher
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,430 pass(es), 108 fail(s), and 6 exception(s). View

Comments

bircher’s picture

Assigned: bircher » Unassigned
Status: Active » Needs review
FileSize
5.7 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 73,430 pass(es), 108 fail(s), and 6 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 1: node_body_out_of_api-2321385-1.patch, failed testing.

xjm’s picture

Category: Feature request » Task
Issue tags: +Configuration system
xjm’s picture

Title: Node body field is created in postSave » Creation of node body field in postSave() incompatible with default config and overrides
Category: Task » Bug report
Priority: Normal » Major

Changed my mind. :)

alexpott’s picture

Status: Needs work » Needs review
FileSize
20.61 KB
26.31 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 74,968 pass(es), 40 fail(s), and 4 exception(s). View

Fixing some of the tests.

Status: Needs review » Needs work

The last submitted patch, 5: 2321385.5.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
7.02 KB
33.33 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,176 pass(es), 8 fail(s), and 3 exception(s). View

So this fixes all the tests. What is super-interesting about this patch is that the node supplies a storage with no instances. This makes sense since book, forum et al all depend on the storage existing and only one module can provide it. But an instance-less storage is impossible to create through the UI.

Also I think we can get rid of the create_field property on NodeType now since the API does not do this for you anymore.

alexpott’s picture

FileSize
3.53 KB
34.86 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,289 pass(es), 9 fail(s), and 5 exception(s). View

Removing all the create_body logic from NodeType

The last submitted patch, 7: 2321385.7.patch, failed testing.

Status: Needs review » Needs work

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

benjy’s picture

FileSize
1.04 KB

I think a custom NodeType destination will fix all your migrate tests. See interdiff attached.

yched’s picture

This also happens with the view_mode but that should be a separate issue.

Then it means this is a more general issue : we currently cannot auto-create a config entity on creation of a config entity ?

Do we accept that and remove all places where we do this (view mode, body field on comments...), or do we try to adress the issue ?

alexpott’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
35.9 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,686 pass(es). View

@yched I think we should be minimising all side effects from ConfigEntity classes since this makes importing much more tricky than it should.

@benjy yep that works! Sweet.

bircher’s picture

@yched: It is a more general issue yes.
We do currently auto-create a config entity on creation of a config entity.
The question is who is responsible for auto-creating the other config-entities?

@alexpott: yes you are right, the node module providing only the storage is a strange situation and will get us in trouble. In fact, you can provoke this to become a problem if you install the default profile, delete the page and article (and with it the body and its storage) and then enable book or forum (which depend on the body field storage to be there) and then BAM!

Maybe someone could say: "bircher should have known and should have added create_body: false to his node type yml, that way the default body doesn't get created. This works as designed."
And that would be half true, it does work (without a patch) when you provide your own content type, say create_body: false and then give it a field.instance.node.NODE_TYPE.body.yml anyway.
That is, as long as you are in the same situation in which book and forum work withthe patch (ie if the body field exists already on another node type and hence the storage exists already.)
That then breaks of course if you would add said module to the installation profile for example.
And this is just for the body field, the same applies to other auto generated config entities (for example entity.view_display.node.NODE_TYPE.default.yml and entity.form_display.node.NODE_TYPE.default.yml.)

So if this stays as it is, we have this awesome config system, but we will have to do resort to the install hook for a some of the default configuration. Even though Forum does that, it just doesn't feel right.

swentel’s picture

+++ b/core/modules/config/src/Tests/ConfigImportRecreateTest.php
@@ -65,6 +65,7 @@ public function testRecreateEntity() {
+    node_add_body_field($content_type);

Just a general remark: maybe we should move this to a method on the NodeType class ? Could be follow up of course.

Looks good to me otherwise.

alexpott’s picture

@swentel I'm not sure about adding a new method to the node type class - since the whole point is that creating other configuration entities should not be the concern of a configuration entity.

swentel’s picture

Yeah, I was thinking in line of comment, but it's on the CommentManager there, so ignore me :)

andypost’s picture

Related issues: +#2106243: Use yml files to create the forum module's comment and taxonomy term fields, +#2032699: Preserve taxonomy_forums field when uninstalling forum module

#13 clean a lot of wtf! and needs re-roll.
I think better to remove node_add_body_field and make a helper in *Test* class

Also the forum has separate issues because the behavior is changed - all forum staff is removed on uninstall except node type.

Berdir queued 13: 2321385.13.patch for re-testing.

yched’s picture

I guess I'm fine with keeping the "auto-create body field" for node types created in the UI.

- The patch also does things about view and form displays - it adds config files, but I don't see where it removes the auto-creation ?
Do we really want to deal with EntityDisplays in this patch ?
- If so, what about the auto-created comment body ?
What about possibly other cases in core ?

But more generally, If we consider that auto-created config entities is not something we can support because they inherently cause nasty conflicts down the line, then I'd think we should prevent them from happening to begin with and have the Config layer throw an exception ?

Berdir’s picture

@yched: node_add_body_field() automatically created the view and form displays if they did not exist before. So if you provide a node type, you now need storage + instance + view display + form display to make the body not only exist but also show up by default

andypost’s picture

@Berdir but there's no code that deletes all this stuff when node types removed

yched’s picture

@Berdir: ah, ok, makes sense. Then I guess the patch is fine as is, and we need separate issues for the comment body, and the more general question of allowing chained creates at all ?

@andypost: config fields ("instances") and entity displays are automatically removed when a bundle gets deleted. Field storages are removed when the last field using it is removed.

swentel’s picture

FileSize
35.62 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,178 pass(es). View

rerolled

bircher’s picture

FileSize
32.67 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,834 pass(es), 101 fail(s), and 59 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 25: 2321385-25.patch, failed testing.

Berdir’s picture

The entity form/view displays now need to be named core.entity_view/form_display. That should fix those errors.

alexpott’s picture

Status: Needs work » Needs review
FileSize
4.14 KB
32.83 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,101 pass(es), 2 fail(s), and 0 exception(s). View

Done.

Status: Needs review » Needs work

The last submitted patch, 28: 2321385-new.28.patch, failed testing.

bircher’s picture

FileSize
31.94 KB
915 bytes

The field_ui test is fixed by not patching it :)
The other one beats me.

Berdir’s picture

One problem I've seen with config import all and entity displays is when the weights are not unique. Then it will be re-ordered on save and result in differences.

bircher’s picture

Status: Needs work » Needs review
FileSize
32.48 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 76,048 pass(es), 5 fail(s), and 0 exception(s). View
2.85 KB

some more things cleaned up.
I am not sure about core/modules/forum/config/install/field.storage.taxonomy_term.forum_container.yml.

Status: Needs review » Needs work

The last submitted patch, 32: 2321385-31.patch, failed testing.

Berdir’s picture

The last interdiff needs to be reverted, that helper method does not exist in kernel tests.

alexpott’s picture

Priority: Major » Critical
Issue tags: +beta target, +Needs issue summary update

Changing this will affect modules that supply content types - we should get this in the beta early so modules that do this can fix themselves early.

swentel’s picture

Assigned: Unassigned » swentel

will look at it tomorrow for the remaining failures

yched’s picture

I'm fine with removing that, but it's still not clear to me why we're doing this only for "the node body field".
If we can't support the creation of config entities creating other config entities, then we should also fix all the other places we do that ?

Also - do we still have a case for isSyncing() then ? IIRC, we introduced it only for this kind of cases, that we don't intend to support anymore ?

bircher’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
12.14 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 75,851 pass(es), 204 fail(s), and 9 exception(s). View

Ok, I finally got it to work a bit better.
I left the creation of the field storage to the NodeType->postSave because otherwise modules that expect the body field storage to exist (forum, blog, custom node types) will fail when the field storage gets removed when the last instance gets removed.

Status: Needs review » Needs work

The last submitted patch, 38: 2321385-38.patch, failed testing.

Berdir’s picture

I was worried about the problem of "who provides the field storage too". Adding it by default is not a solution, that still makes it impossible to use a different field type, for example.

swentel’s picture

Had a quick chat with yched. Another idea would be that we make body a base field. You can hide it on form display if desired.
One problem here is that you can't change the label though, so that could be moved to the node edit screen, just like for title. That's less nice I guess.

Or we should proceed with #2285781: Add option to softly rename field labels in UI ..

alexpott’s picture

Issue tags: +D8 upgrade path

This probably will involve changes that block a simple upgrade path.

alexpott’s picture

Discussed the idea of making the body field a base field on the CMI call on 30 Oct 2014. @xjm pushed back strongly saying that there are so many reasons to not have a body field on a node. For example, @xjm pointed out why have a body base field if it is not used. Personally, I'm not sure I think this is a compelling argument since we have other base fields that might not be used - how many sites actually make use of the revision_log field i'd make a bet that it's far far less than use the default body field.

If we have forum and book modules and the wish to create new node type's with a body field that all shared the same storage (including the node types created by standard profile) then we have to do something that guarantees that the field storage exists. Options are:

  • Provide it as a base field
  • Somehow lock the body field storage and provide it with the node module

We can not go down the provide a new node body field feature module to do this since that won't work for the new node type use case - maybe we could do this if we check if this module exists in node_add_body_field()and use its body field otherwise don't add one - but then we'd still have to do the locking solution since the default storage would be deleted if all the fields that used were deleted. We can't go down the provide it in code route because that does not work with providing new node types in default config that use the default body storage (this is the problem this issue has uncovered).

Actually the current node_add_body_field() has some problems. Consider following:

  1. Create a new node type
  2. Delete the body field
  3. Create a new body field of the type options list
  4. Create another node type... and and the display formatter on the body field will be set to text_textarea_with_summary?!?!?!

We have two competing requirements here and we have to choose one

  • The ability for node bundles to not have a body field
  • The ability for node bundles to share the same body field storage

Both are common but I would argue that the second use case is far more usual for sites.

Conclusion

The current solution in HEAD for locked field storages is to use base fields. That is why that solution is worth considering. However, looking at the two possible solutions I would argue that only Somehow lock the body field storage and provide it with the node module is only way forward at this point. The upgrade path for existing site would be horrific for the base field solution - and I think we'd have to provide something to help people. The only downside of locking is that we have no way in the field ui to say that the name body is taken before you create a field - but essentially this situation is only theoretical since on creating the first node type through the UI it will add a body field so I can't see a "real" problem here.

I will proceed with writing a patch to add a locked property to FieldStorage which makes it undeleteable.

[edited for clarity]

alexpott’s picture

Status: Needs work » Postponed

Postponing on #1426804: Allow field storages to be persisted when they have no fields. since that issue is about providing and persisting field storages with field instances.

alexpott’s picture

swentel’s picture

Status: Postponed » Active

This is not postponed anymore

alexpott’s picture

Status: Active » Needs review
FileSize
41.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,221 pass(es), 1 fail(s), and 1 exception(s). View

New patch post #1426804: Allow field storages to be persisted when they have no fields.

  • Book's body field and entity display modes move into config
  • Forum's fields and entity display modes moved into config apart from comment_forum related stuff since that needs #2374087: Create a persistent comment body field storage
  • Page and article body fields and entity display modes moved into config

There is no more auto creation of body fields in the NodeType entity.

Status: Needs review » Needs work

The last submitted patch, 47: 2321385-new-body-storage.47.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
FileSize
745 bytes
42.24 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,176 pass(es). View

Fixes that migrate test.

Berdir’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/book/config/install/core.entity_view_display.node.book.default.yml
    @@ -0,0 +1,26 @@
    +  links:
    +    weight: 100
    +  body:
    +    label: hidden
    +    type: text_default
    +    weight: 101
    

    links should be below the body? Bartik forces it to be at the end, but the default template afaik doesn't do that. Probably like this for other displays too.

  2. +++ b/core/modules/forum/config/install/field.field.node.forum.body.yml
    @@ -0,0 +1,23 @@
    +uuid: dc40d431-7116-427c-8305-5ff6ee709563
    
    +++ b/core/modules/forum/config/install/field.field.node.forum.taxonomy_forums.yml
    @@ -0,0 +1,23 @@
    +uuid: 7f4c8bc0-4dbb-487c-b207-7f0228335c9f
    

    :)

  3. +++ b/core/profiles/standard/config/install/field.field.node.article.body.yml
    @@ -0,0 +1,22 @@
    diff --git a/core/profiles/standard/config/install/field.field.node.page.body.yml b/core/profiles/standard/config/install/field.field.node.page.body.yml
    
    diff --git a/core/profiles/standard/config/install/field.field.node.page.body.yml b/core/profiles/standard/config/install/field.field.node.page.body.yml
    new file mode 100644
    
    new file mode 100644
    index 0000000..57bb0b0
    
    index 0000000..57bb0b0
    --- /dev/null
    

    A bit confused why the file mode is explicitly set in the patch?

Was a bit confused that there are no changes to the default config in node.module and the node_add_body_field() function, but that's because those changes already happened in the other issue.

Apart from those minor things, I think this is ready and seems like a great solution, as it gives install profiles full control (I should be able to override the default body field storage and use text_long instead of test_with_summary, which is what I want and is annoying me atm in several ways ;))

alexpott’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update
FileSize
5.16 KB
42.16 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,239 pass(es). View

@Berdir thanks for the review.

  1. Fixed
  2. Fixed
  3. Not sure how to stop this and if this is even a problem.
Berdir’s picture

Ok, this is RTBC from my point of view. But I'm a bit detached from core for one more week, so I'd appreciate if someone else could check this as well and set it to RTBC. @swentel or @yched would be great candidates I think :)

amateescu’s picture

  1. +++ b/core/modules/field_ui/src/Tests/ManageDisplayTest.php
    @@ -364,11 +364,10 @@ function testSingleViewMode() {
    -    $this->drupalCreateContentType(array(
    +    entity_create('node_type', array(
           'type' => 'no_fields',
           'name' => 'No fields',
    -      'create_body' => FALSE,
    -    ));
    +    ))->save();
    

    This change seems to swim against the current :)

  2. +++ b/core/modules/forum/config/install/core.entity_form_display.taxonomy_term.forums.default.yml
    @@ -1,8 +1,14 @@
    +langcode: und
    

    All the others are 'en', why is this one different?

  3. +++ b/core/modules/forum/config/install/core.entity_view_display.taxonomy_term.forums.default.yml
    @@ -1,8 +1,15 @@
    +langcode: und
    

    Same here.

  4. +++ b/core/modules/forum/forum.install
    @@ -22,65 +22,15 @@ function forum_install() {
    +    $term = entity_create('taxonomy_term', array(
    

    No biggie but.. Term::create()?

Berdir’s picture

Or @amateescu :)

1. Note the create_body => FALSE. this *is* the opposite of all other usages.

amateescu’s picture

@Berdir, right.. I didn't notice that the patch also changes drupalCreateContentType() to also add the body.

larowlan’s picture

+++ b/core/modules/node/src/NodeTypeForm.php
@@ -230,6 +230,7 @@ public function save(array $form, FormStateInterface $form_state) {
+      node_add_body_field($type);

damn, wish we had a NodeManager like comment module

I think the only question remaining here is why does entity_view_display.taxonomy_term.forums.default.yml get und language instead of en. With that answered I think this is rtbc.

alexpott’s picture

FileSize
510 bytes
42.15 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,127 pass(es). View

Well it's und currently in head because the existing config entity in HEAD does not have a langcode - but here is the fix. Before release we should have an issue to re-export and check all config entities.

amateescu’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
42.35 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 81,120 pass(es). View
1.24 KB

Missed one (or two) :)

larowlan’s picture

+1 RTBC

  • catch committed 0cde5d7 on 8.0.x
    Issue #2321385 by alexpott, bircher, amateescu, swentel, benjy: Fixed...
catch’s picture

Status: Reviewed & tested by the community » Fixed

Changes look fine. A lot more default config getting shipped, but less custom logic in PHP, seems good to me. Fixes a critical and unblocks other critical issues so great per #2350615: [policy, no patch] What changes can be accepted during the Drupal 8 beta phase?.

Committed/pushed to 8.0.x, thanks!

yched’s picture

Finally got aroud to reviewing this - got committed meanwhile, but confirming RTBC :-)

Side remarks :

  1. Patch added explicit node_add_body_field() calls to all KernelTest tests that create a node type.
    We have drupalCreateContentType() on WebTestBase, that takes care of calling node_add_body_field(). Maybe we should add it to DrupalUnitTestBase too ?
  2. +++ b/core/modules/forum/forum.install
    @@ -22,65 +23,15 @@ function forum_install() {
    +    // Create a default forum so forum posts can be created.
    +    $term = Term::create(array(
    +      'name' => t('General discussion'),
    +      'description' => '',
    +      'parent' => array(0),
    +      'vid' => 'forums',
    +      'forum_container' => 0,
    +    ));
    +    $term->save();
    

    Different issue, but that stays inside the if (!\Drupal::service('config.installer')->isSyncing()) { check.
    We're not creating that "default forum" term when deploying the installation of forum.module - so that term is never created ?

Status: Fixed » Closed (fixed)

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