Problem/Motivation

After #3488742: Stop calling node_add_body_field() from NodeTypeForm node_add_body_field() is only called in tests. Let's remove all the calls and deprecate it.

Proposed resolution

- Deprecate node_add_body_field
- Update tests to use ContentTypeCreationTrait

Remaining tasks

Review

API changes

node_add_body_field is deprecated

Issue fork drupal-3489266

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

catch created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Would we also deprecate/remove the body field config itself then ((or more likely, move to standard/recipes)) or keep it for manually adding the body field?

catch’s picture

catch
he/him
Primary language English
commented

@berdir we're trying to change it to text_long in #3477043: Change automatic body field creation to use formatted text field instead of text_with_summary (which turns out to be hard) but ultimately I think this is something that recipes should handle - shipping a content type (that needs a body field) is recipe-ish already, and recipes can also handle the create-if-not-exists logic that removing the default config would require.

annmarysruthy made their first commit to this issue’s fork.

annmarysruthy’s picture

annmarysruthy commented
Assigned: Unassigned » annmarysruthy

annmarysruthy opened merge request !11194

annmarysruthy’s picture

annmarysruthy commented
Assigned: annmarysruthy » Unassigned

acbramley made their first commit to this issue’s fork.

acbramley’s picture

acbramley commented
Issue summary: View changes
Status: Active » Needs review

Rebased and tidied up a lot of duplication in tests by using ContentTypeCreationTrait, also added a CR.

There were 2 spots in runtime code that are still using this function:
- NodeTypeForm - only using for the testing profile, this should definitely just be removed. We don't want test code in runtime code.
- The EntityNodeType migration destination plugin - I've replaced this with the contents of node_add_body_field

So the outcome here is a little bit more duplication (between the test trait and migration plugin) but I think that's ok.

acbramley’s picture

acbramley commented

Looks like only 6 tests relied on the NodeTypeForm bits https://git.drupalcode.org/issue/drupal-3489266/-/pipelines/463009/test_...

Other failures in that pipeline are random afaict

acbramley’s picture

acbramley commented

Last failure is in Nightwatch and for once doesn't look like a random fail, I'm assuming we need to add a step to it to create the body field

https://git.drupalcode.org/issue/drupal-3489266/-/jobs/4842498

smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Whatever we decide here we should do the same for block_content as pretty sure we got copy and paste code with the same stuff.

Maybe we need some default config in the testing profile for nightwatch? Or is there a utility call to add a field?

acbramley’s picture

acbramley commented
Title: Deprecate node_add_body_field() » [PP-2] Deprecate node_add_body_field()
Status: Needs work » Postponed
Related issues: +#3517024: Convert ckEditor5EditorHeightTest to WebDriver test, +#3517023: Convert ckEditor5CodeSyntaxTest to WebDriver test

Postponing on converting those tests to WebDriver tests.

spooky063’s picture

spooky063 commented

#13 So you've decided to drop the Nightwatch test?

acbramley’s picture

acbramley commented

@spooky063 that's right, it's probably the most straight forward and best overall solution IMO. Nightwatch tests are already on the chopping block, and since it's only 2 tests that can both be more easily tested in PHP (WebDriver) tests it makes sense to move those anyway.

Otherwise we need to either
a) copy the same field creation code into NodeTypeForm again, we don't want to tie that form to the testing profile anyway so this wouldn't do, or
b) add more Nightwatch steps to the 2 tests, further extending the already lengthy test case. That would then need to be ported again when Nightwatch is eventually replaced.

I looked at adding default config to the testing profile but the profile is specifically very barebones so that didn't feel right either.

One of the issues is already up for review with more detail so feel free to review if you can :)

acbramley’s picture

acbramley commented

Whatever we decide here we should do the same for block_content as pretty sure we got copy and paste code with the same stuff.

We should probably also make the body field creation reusable in that case and use it in the NodeTypeCreationTrait

acbramley’s picture

acbramley commented
Title: [PP-2] Deprecate node_add_body_field() » [PP-1] Deprecate node_add_body_field()

We just need to get #3517024: Convert ckEditor5EditorHeightTest to WebDriver test in

acbramley’s picture

acbramley commented
Title: [PP-1] Deprecate node_add_body_field() » Deprecate node_add_body_field()
Status: Postponed » Needs work

Both blockers are in, this needs a manual rebase

acbramley’s picture

acbramley commented
Status: Needs work » Needs review

This is ready to go now

smustgrave’s picture

smustgrave commented

Question, think it would help to have a function like this in a test trait?

acbramley’s picture

acbramley commented
Parent issue: » #3532197: [meta] Deprecate node.module functions

oily made their first commit to this issue’s fork.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work

Reviewed a bit.

acbramley’s picture

acbramley commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Came here from #3535526: Deprecate block_content_add_body_field and stop automatic creation of body field for that trait :)

Didn't close the threads but didn't want to ignore them, with regards to the moderation tests maybe we open a follow up for cleaning up strings that we aren't asserting or removing assertions that really aren't needed.

The trait is a great idea!

I see no open questions so believe this one is ready

  • catch committed 5d33ca31 on 11.x 
    Issue #3489266 by acbramley, annmarysruthy, smustgrave, catch, berdir,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

I think this is a good step forward. We still have the default config to try to get rid of in #3447617: Stop automatic storage creation of body field for node.

Committed/pushed to 11.x, thanks!

catch closed merge request !11194

Status: Fixed » Closed (fixed)

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