Change automatic body field creation to use formatted text field instead of text_with_summary

Created on 26 September 2024, about 2 months ago

Problem/Motivation

πŸ“Œ Stop automatic storage creation of body field Active will not make it in by the Starshot release, so we need to consider what is the least disruptive in the meantime.

Proposed resolution

The data model will use the regular formatted text field instead of text_with_summary, so we discussed at DrupalCon Barcelona that that the best strategy is to change the automatic field to be a regular formatted text field instead of using text_with_summary.

Remaining tasks

  1. Agree on the name for the field (body vs. something else)
  2. Make the change (and update tests if needed?)
  3. Review
  4. Commit and flourish

User interface changes

N/A

Introduced terminology

N/A

API changes

N/A

Data model changes

N/A

Release notes snippet

N/A

πŸ“Œ Task
Status

Active

Version

11.0 πŸ”₯

Component

field system

Created by

πŸ‡¦πŸ‡ΊAustralia pameeela

Live updates comments and jobs are added and updated live.
  • Starshot blocker

    A potential blocker for Drupal Starshot. More information: http://www.drupal.org/project/starshot

Sign in to follow issues

Merge Requests

Comments & Activities

  • 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!

  • Pipeline finished with Failed
    21 days ago
    Total: 570s
    #326042
  • First commit to issue fork.
  • Pipeline finished with Failed
    21 days ago
    Total: 603s
    #326061
  • Pipeline finished with Failed
    21 days ago
    Total: 96s
    #326076
  • Pipeline finished with Failed
    20 days ago
    Total: 554s
    #326807
  • Pipeline finished with Failed
    20 days ago
    Total: 588s
    #326825
  • Pipeline finished with Failed
    20 days ago
    Total: 483s
    #326848
  • Pipeline finished with Failed
    20 days ago
    Total: 413s
    #326855
  • Pipeline finished with Failed
    20 days ago
    Total: 453s
    #326868
  • Pipeline finished with Failed
    20 days ago
    Total: 139s
    #326889
  • Pipeline finished with Canceled
    20 days ago
    Total: 412s
    #326882
  • Pipeline finished with Failed
    20 days ago
    Total: 528s
    #326892
  • Pipeline finished with Canceled
    20 days ago
    Total: 1279s
    #326900
  • Pipeline finished with Failed
    20 days ago
    Total: 85s
    #326922
  • Pipeline finished with Failed
    20 days ago
    Total: 481s
    #326926
  • Pipeline finished with Failed
    20 days ago
    Total: 562s
    #326946
  • Pipeline finished with Canceled
    20 days ago
    Total: 409s
    #326970
  • Pipeline finished with Canceled
    20 days ago
    Total: 126s
    #326981
  • Pipeline finished with Failed
    20 days ago
    Total: 497s
    #326985
  • Pipeline finished with Failed
    20 days ago
    Total: 501s
    #327021
  • Pipeline finished with Canceled
    20 days ago
    Total: 75s
    #327059
  • Pipeline finished with Failed
    20 days ago
    Total: 467s
    #327060
  • Pipeline finished with Failed
    20 days ago
    Total: 830s
    #327153
  • Pipeline finished with Canceled
    20 days ago
    Total: 391s
    #327167
  • Pipeline finished with Failed
    20 days ago
    Total: 602s
    #327170
  • Pipeline finished with Failed
    19 days ago
    Total: 680s
    #327278
  • Pipeline finished with Failed
    19 days ago
    Total: 658s
    #327285
  • πŸ‡¬πŸ‡§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.

  • Pipeline finished with Failed
    19 days ago
    Total: 731s
    #327670
  • Pipeline finished with Success
    19 days ago
    Total: 788s
    #327681
  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    All green!

  • πŸ‡¬πŸ‡§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

    Created 2 CRs thoughts?

  • πŸ‡ΊπŸ‡ΈUnited States smustgrave

    Also wonder what the roadmap for deprecating text_with_summary looks like now?

  • Pipeline finished with Failed
    17 days ago
    Total: 1090s
    #329129
  • πŸ‡¦πŸ‡ΊAustralia pameeela
  • πŸ‡¦πŸ‡Ί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 alexpott πŸ‡ͺπŸ‡ΊπŸŒ

    Re #20 ah I see we want to change the type from type: text_with_summary to type: text_long. Hmmm... not being able to rely on node creating a body field storage is likely to have interesting impacts.

  • πŸ‡¬πŸ‡§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.

  • πŸ‡¬πŸ‡§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.

Production build 0.71.5 2024