- Issue created by @pameeela
- π¬π§United Kingdom catch
I think this is a good interim step - it will mean we no longer use text_with_summary in core.
Once we no longer use it, it should be possible to factor it out to a module for contrib removal, and separately work on removing the automatic body field creation too.
- πΊπΈUnited States smustgrave
So for step 1 of renaming the field I think that would be very difficult no? Thinking of all the instances we have of $node->body and if we changed to say $node->field_body
Personally I think field_body makes since but also not picky.
- πΊπΈUnited States smustgrave
That said for this particular purpose would it be easier to just change the type and leave the name as body in this ticket?
- π¬π§United Kingdom catch
That said for this particular purpose would it be easier to just change the type and leave the name as body in this ticket?
Yes, we can just change the type here - it might require some additional changes for the lack of summary, but it's worth starting there.
- πΊπΈUnited States smustgrave
Should have time in the next day to give it a shot.
Only other question, this may break migration tests I imagine. Do we fix those or is there some backwards compatibility aspect to consider?
- π¬π§United Kingdom catch
@smustgrave we should just update the tests for the new logic - the body + summary field type is still available for people to use and migrate into.
- πΊπΈUnited States smustgrave
Will tr and get started on this today!
- First commit to issue fork.
- π¬π§United Kingdom catch
I've picked off two kernel tests - one needed node_add_body_field() to more closely match the original shipped config so was a real bug.
Another one just needed the test assumptions updating because it's a small behaviour change with the config being shipped vs created on the fly - we're going to deprecate/remove the auto-creation in π Stop automatic storage creation of body field Active and a lot of the changes being made here are prerequisites for that.
I removed one very, very old functional node view modes test that was added in 2012 or earlier because it relied entirely on the text_with_summary field to handle assertions, and we have plenty of generic and better view mode testing in core elsewhere at this point.
Just the one nightwatch test to go now.
- π¬π§United Kingdom catch
Bumping priority since this blocks starshot.
We're definitely down to just the nightwatch test but I can't even get nightwatch tests to run locally at the moment and out of time for today.
- π¬π§United Kingdom catch
Haven't reviewed the whole MR but definitely one thing missing - need to document the new body storage creation / removal of the default config since it'll potentially affect contrib kernel tests.
- πΊπΈUnited States smustgrave
Also wonder what the roadmap for deprecating text_with_summary looks like now?
- π¦πΊAustralia pameeela
Just for fun I manually tested this too, looking good!
- π¬π§United Kingdom alexpott πͺπΊπ
Added a review comment to the MR.
- π¬π§United Kingdom alexpott πͺπΊπ
I have some concerns about not shipping field.storage.node.body.yml with the node module. Why is that necessary? Also https://www.drupal.org/node/3485412 β incorrectly uses the name field.field.node.page.body.yml - that was never a part of the node module config as far as I know.
The reason the node module ships with the body field storage is so that modules can really on a body storage being available for any new node types. It allows views of nodes with different types to rely on the body field. I'm not sure why changing the formatter needs the storage change.
- π¬π§United Kingdom catch
@alexpott there are two reasons:
1. We want to deprecate all of this anyway, see π Stop automatic storage creation of body field Active . This issue is the first of about three issues to do that.
2. Migrations from Drupal 7 expect a text_with_summary body field - both real sites that are doing a stock migration with migrate_drupal_ui and also the test coverage and fixtures. Supporting that means either creating the field in PHP as is done in the MR, or would require something like deleting the text_long field then creating a text_with_summary field with the same name which would be very disruptive to tests and also very fragile.
- π¬π§United Kingdom alexpott πͺπΊπ
Well this issue is doing π Stop automatic storage creation of body field Active for every non-standard install of Drupal so is likely to require changes in any other install profile or at least some interesting considerations.
Also I suspect we need to make node_add_body_field() more robust. If you call it will 'text_long' and a body storage already exists with 'text_with_summary' then it is going to use the existing storage. Also should we care if the value in $field_type is nonsense? Will the system error as expected? I guess it'll only error if it actually has to create the field storage.
I also think we need to really think about π Stop automatic storage creation of body field Active - that fact that nodes get a consistent body field is used by views and other modules to allow cross node type functionality. Removing this is likely to break a few assumptions. These might be great to break and tell every one to use view modes instead but it is likely to be complex and hard.
- π¬π§United Kingdom catch
@alexpott I never use the core body field on sites, not for 15 years or more, because it uses text_with_summary, and have never noticed a contrib module relying on it. Do you have an example other than tests?
We can make node_add_body_field() more robust but also I doubt any code outside core calls it.
- π¬π§United Kingdom alexpott πͺπΊπ
There are definitely test usages of node_add_body_field() outside of core.
Here's examples of contrib modules that will be broken when this is released:
https://git.drupalcode.org/project/blog/-/blob/3.x/config/install/field....
https://git.drupalcode.org/project/book/-/blob/2.0.x/config/optional/fie...
- yes both modules we've removed from core (hence making this change both easier and harder) but dependencies on the current implementation of field.storage.node.body are real and this will cause disruption.Using the alternative code search to look for dependencies on field.storage.node.body is revealing... 29 pages of results... http://codcontrib.hank.vps-private.net/search?text=-%20field.storage.nod...
- π¬π§United Kingdom catch
I went through the modules from http://codcontrib.hank.vps-private.net/search?text=-%20field.storage.nod... alphabetically up to E. Not a single module had a stable or even dev release for Drupal 10 or Drupal 11.
Out of 14 projects, the combined usage was 15. 10 usages are from an install profile. So in fact the 13 actual modules have a combined usage of 5. Some of them have Drupal 7 releases and no Drupal 8 releases which could mean the usage is even lower than 5.
Install profiles can very easily add node body field storage same as we've done for the standard profile in this MR - I think this is reasonable with a change record.
Book and blog do ship node types with body fields, however they can add the storage field to config/optional and/or book ships its config as optional anyway. Blog module doesn't have Drupal 11 support so will need to be updated anyway before it can be installed on new sites. Book module has supported any configured content type since Drupal 6, so the shipped node type is really vestigial and could be dropped too.
Maybe there is a high usage contrib module between E-Z in that list, but even for that, the change would only impact installs on new sites, not existing sites which already have the module installed and control their own config anyway, so I think the potential disruption here is pretty low.
- πΊπΈUnited States smustgrave
So before working on this more is this heading in the right direction? Is it something that will be accepted?
- π¬π§United Kingdom catch
@smustgrave @alexpott is concerned this will cause issues with modules that ship config expecting the body field to be available - book and blog are two, there are several others but per #26 after looking through a sample I was unable to find one with a stable release. For me that should be OK with a change record. It won't affect existing sites with any of those modules installed, because they already have whatever config they installed with, it would only affect installing them from fresh on a site (and even then existing sites will have text_with_summary anyway, so it would be on new sites installed on versions after this change).
I personally think we should go ahead here once https://git.drupalcode.org/project/drupal/-/merge_requests/10019#note_40... is fixed but it might need more discussion - tagging for Needs Release Manager Review and I'll bring it up with other committers.
- π³πΏNew Zealand quietone
I went through the modules from http://codcontrib.hank.vps-private.net/search?text=-%20field.storage.nod... alphabetically from z to w, inclusive. I then checked a few 'views' related projects. All these have a release for Drupal 10, and a few for Drupal 11.
- https://www.drupal.org/project/y_lb_article β
- https://www.drupal.org/project/wxt β
- https://www.drupal.org/project/wsdl_docs β
- https://www.drupal.org/project/ws_event β
- https://www.drupal.org/project/widget_engine β
- https://www.drupal.org/project/webpage β
- https://www.drupal.org/project/webdoc β
- https://www.drupal.org/project/webdam β
- https://www.drupal.org/project/webblog β
- https://www.drupal.org/project/views_streaming_data β
- https://www.drupal.org/project/views_rss β -- Drupal 11
- https://www.drupal.org/project/views_kanban β - Drupal 11
- π¬π§United Kingdom catch
I checked the two modules with 11.x releases.
First views_rss: The body field is referenced by
tests/modules/views_rss_test_config/config/install/views.view.test_views_rss_feed.yml
. Depending on what the test does, this may or may not require changes but either way it would only affect the test module, no runtime code.I also checked views_kanban, the body field is in
Demo/views_kanban_demo/config/install/field.field.node.task.body.yml
so not quite a test module, but a demo module.
- π¬π§United Kingdom catch
This should probably get official product manager sign-off too, although that may already have happened on one of the 2-3 related issues.
- πΊπΈUnited States smustgrave
With π Remove node_add_body_field from NodeTypeForm Active landed what needs to happen here?
- π¬π§United Kingdom catch
π Deprecate node_add_body_field() Active is probably the smallest next step we can take - only affects tests.
We still need to do this issue (or take it even further) to deprecate text_with_summary.
- πΊπΈUnited States smustgrave
So we'll need a new module that will house that stuff right?
- π¬π§United Kingdom catch
Yeah I think #34 is right.
I wonder if we can create new core module node_body_field that includes the default config - possibly just using text_with_summary instead of text_logn.
Modules that ship a body field can then require that module (which should be OK in a minor release with a CR) and nothing should change for them.
We can then deprecate the module. This will need a CR to encourage those modules to switch to recipes or something similar instead of relying on the config.
Then we can mark the module obsolete.
- Assigned to smustgrave
- Status changed to Needs work
26 days ago 6:31am 26 November 2024 - π¦πΊAustralia pameeela
No longer a blocker since π Remove node_add_body_field from NodeTypeForm Active landed.