Creation of node body field in postSave() incompatible with default config and overrides

Changed my mind. :)

Fixing some of the tests.

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.

Removing all the create_body logic from NodeType

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

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 ?

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

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

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

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

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

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 ?

@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

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

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

rerolled

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

Done.

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.

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

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.

will look at it tomorrow for the remaining failures

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 ?

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.

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

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

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]

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.

This is not postponed anymore

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.

Fixes that migrate test.

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 ;))

@Berdir thanks for the review.

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

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 :)

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

Or @amateescu :)

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

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

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

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.

Missed one (or two) :)

+1 RTBC

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!

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 ?