FieldException on content type creation if the Body field is still getting purged

[allsite]

I found an old D7 bug that also exists in Backdrop. Steps to reproduce (in strict order):

Install backdrop
delete both Articles and Posts content types
create a new custom content type, hit save
The following error occurs when saving any/all new content types:

FieldException: Attempted to create an instance of field body, but that field does not exist.
in field_create_instance() (line 515 of /home/mysite/public_html/core/modules/field/field.crud.inc).

This is a common series of events that results in a damaged site. It also chimes into the discussion over "Config defaults" and why less is more in terms of default settings. In this case the Body field default setting actually breaks the site. I feel a simple fix would be to not add a Body field by default and not many people will miss it.

[indigoxela]

I'm having trouble to reproduce this bug. I followed your instruction step by step (made sure that there's no field config left), created a new content type - and it got created without problems, including a body field.

What am I missing?

[allsite]

Did you test vs fresh install?

[indigoxela]

Did you test vs fresh install?

Yes, exactly that, the install has been created right before my test. Vanilla install, standard profile, zero config changes.

[allsite]

Interesting. I just spun up a new site and reproduced it again.

[allsite]

Also reproduced it on a backdropcms.org Demo site.

[indigoxela]

Weird...

Some screenshots from my attempt:

[allsite]

Very weird. I've reproduced it 3 times in backdropcms.org demos as well as twice on my personal server. I also have a production D7 site created a few years ago that suffers the issue. Maybe you give it a try on a backdropcms.org demo instance?

[indigoxela]

Yes, on the demo I can also reproduce it. But not locally... 😕

I found one odd thing: on the demo site I went to admin/config/development/configuration/single/export after deleting both content types and attempting to create a new one (with Exception).

This is the content of field.field.body.json:

{
    "_config_name": "field.field.body",
    "field_name": "body",
    "type": "text_with_summary",
    "entity_types": [
        "node"
    ],
    "module": "text",
    "active": 1,
    "locked": 0,
    "cardinality": 1,
    "translatable": 0,
    "deleted": 1,
    "settings": [],
    "storage": {
        "type": "field_sql_storage",
        "settings": [],
        "module": "field_sql_storage",
        "active": 1
    }
}

So, the field definition exists, but still the Exception. Hm... race condition when creating? The field type isn't available yet, but it's attempted to create the instance? Wild guessing, I know.

[indigoxela]

Another thought. In the demo box the field and field_instance config does not get deleted. I don't have access to the file system, of course, but assume that the files still exist.

On my local testing instance (note: Backdrop 1.20.x-dev) all field files do get deleted, I also can't access any field/field_instance via config export. So this seems to be the main difference.

@allsite did you also try with the latest dev? Maybe that problem got fixed "drive by".

[allsite]

There's clearly something magical about your server!

I blew up several more demo sites. There's more than one way to encounter the issue, all of which seem related to removing the last instance of Body field. After getting the error I navigate to Manage Fields on the newly created content type that threw the error and notice there is no "Add existing field option". In one way or another the Body field is "gone".

[indigoxela]

There's clearly something magical about your server!

Ha! See. 😆

I actually think it's about the Backdrop version, but we still need to verify. PHP, Apache, Mariadb/MySQL versions slightly differ, but IMO that's not related.

[allsite]

Where would I find Backdrop 1.20.x-dev?

I'm certain this bug came from D7 and almost certain it isn't specific to any PHP version or other server config. Today, I spun up a fresh site with the sole purpose of testing for the bug and was unfortunately successful. It's somewhat comical being a new person in the backdrop community and mostly all I've contribute thus far is pointing out the worst of the worst of D7 that is residual in backdrop. Nobody can fix it until someone breaks it! 😁

[indigoxela]

Where would I find Backdrop 1.20.x-dev?

On GitHub 😉 either:

wget https://github.com/backdrop/backdrop/archive/refs/heads/1.x.zip
or
git clone https://github.com/backdrop/backdrop.git

You always tested with the latest stable so far?

[allsite]

Correct, latest stable so far. I'll give dev a run this weekend and see what happens.

[indigoxela]

I'll give dev a run this weekend and see what happens.

Cool, please report your findings back here.

[indigoxela]

Out of curiosity I ran cron in that demo instance I was able to reproduce the problem earlier.

And: content type created seamlessly, including the body!

So, as long as the old field.*.json files still exists, Backdrop throws an exception. As soon as the json files got removed, new content types get created without problems.

Related core function:

• field_attach_delete_bundle
• field_delete_instance
• field_purge_field

Backdrop's workflow:

Function field_delete_instance() sets a "deleted" marker in the json file, but doesn't yet delete it. That's because there still might be database table entries. These get deleted in batches on cron runs. After all those records are gone, the field.instance.*.json file also gets deleted.

But until then - if there are zero fields available (all content types deleted), creating a new content type seems to fail. So, this seems to be the bug. Some sloppy or too greedy validation when creating a node type. But where?

Hunting season is opened. 🦌 hunting season closed. 😆

Wait a minute. That's correct behavior because fields get a database table with a name based on field name. If a table still exists, it's impossible to create that table, so the Exception makes sense.

But... that's a bad user experience (FieldException). It would be way more helpful to show a message why it's currently impossible to create that field instance - database table name collision (field_data_body, field_revision_body still exist).

[allsite]

I ran cron before deleting the content types and sure enough, no error. I also repeated steps to get the error, then apparently resolved it by running cron. There's still a bug here but at least it feels much less critical and more understood.
I suspect in D7 running cron didn't resolve it but will confirm when I have more time to break stuff.
Nice investigative work!

[indigoxela]

I'm not sure how often that problem really occurs, but throwing an exception because of a temporary problem lets Backdrop look unnecessarily unstable. And it's very easy to catch in node_type_form()!

If the body field isn't purged yet, just don't add it automatically - we know this would result in a FieldException, anyway.

A PR is ready for testing and review. Improvement tips for message wording are welcome.

Testing can be a bit tricky, if you have a "magical" server like mine 😁, which purges the body field very quickly. In that case - if the field.field.body.json file is already gone, you could add it manually in your active config directory.
Use the contents as posted in my previous comment (note the "deleted": 1,).

The steps for testing (locally, but the sandbox should also work using the configuration manager):

• Delete all content types (post, page)
• Immediately add a new content type using the form on admin/structure/types/add
• Verify that the message is shown and saving the content type works smoothly (and that new type has no "automagic" body field)
• Run cron after that and verify that the message on admin/structure/types/add is gone and the body field get's added again to new types

[indigoxela]

And here's a screenshot of the warning (again, hints for better wording are welcome):

[laryn]

@indigoxela It's not bad, but maybe it could be slightly shorter/simpler:

The Body field has been marked for deletion so it has not been added.

[indigoxela]

The Body field has been marked for deletion so it has not been added.

Definitely better - shorter is always better. 👍 It's on the form, though, so before not adding it.

[laryn]

@indigoxela

The Body field has been marked for deletion so it will not be added here.

?

[indigoxela]

... so it will not be added here.

Seems right, but there's a small problem: the message from the form gets repeated after submission. So it shows up on the form and after content type creation. I don't think, there's a way to prevent that - setting $repeat to FALSE has no effect here.

[jenlampton]

For the body field "added" makes sense when it is added automatically, but for other fields, maybe "available" is better, since it needs to be added manually? I also feel strongly that telling people there's something wrong, without telling them how to fix it, doesn't actually help very much. They'll already be able to see that the field is not there (and I already voiced my concern about "marked for deletion"). But I agree that having too many words in the message is also a problem. Maybe we can work on trying to shorten my suggestion, or come up with an alternative message that still tells people what needs to be done?

…

On Sat, Aug 7, 2021, 5:12 PM docwilmot ***@***.***> wrote: the message from the form gets repeated after submission. We could do a fake message using FormAPI. Layout module does this a fair bit: $form['messages'] = array( '#theme' => 'status_messages', '#messages' => $messages, ); As far as the language, I like "The Body field has been marked for deletion so it will not be added here." Concise. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#5141 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AADBER47JZ5ORHLYEAENIK3T3XDW5ANCNFSM5BIDMPDA> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email> .

[indigoxela]

For the body field "added" makes sense when it is added automatically, but for other fields

Note that this issue is exclusively dealing with exact that Body field. No other field can cause trouble here, as it's the only one that's automatically attached to new content types.

Maybe we can work on trying to shorten my suggestion...

Sure, as long as we don't get stuck on wording... 😁

The Body field has been marked for deletion and purging is still incomplete. Run cron if you want an automatic Body field on new types.

... and the native speakers can improve that again. 😄

[jenlampton]

The Body field has been marked for deletion and purging is still incomplete. Run cron if you want an automatic Body field on new types.

Better!

• I'm still weary of "marked for deletion" -- though technically correct, the language isn't very human (in English), and the meaning will be lost on people who don't understand what Backdrop does when that happens.
• I'd also like to provide a solution for those who are unable to run cron. Maybe "try again later".
• I think we might be able to shorten it even more...

Previous Body field values are still being deleted. For the Body field to appear on new content types, Run cron, or try again in 1 hour.

or maybe...

Body field values are still being deleted. To include Body on new content types, Run cron, or try again in 1 hour.

I personally have never liked "try again later" as it's frustrating not to know how long to wait. This is why I recommended including the current cron configuration value. (in place of "1 hour", above)

[docwilmot]

The Body field was previously deleted. Run cron to restore it, or try again in 1 hour.

[laryn]

Would we be getting too complex to calculate the number of minutes to wait from the cron configuration value and the state_get('cron_last') value?

[jenlampton]

The Body field was previously deleted. Run cron to restore it, or try again in 1 hour.

Nice and short! Thanks @docwilmot :)

Would we be getting too complex to calculate the number of minutes to wait from the cron configuration value and the state_get('cron_last') value?

I was thinking the same thing! If we don't already, we could create a helper function for this and then replace all instances of "try again later" in core :D (filing a follow-up issue now...)

[indigoxela]

Thanks to @docwilmot's trick the message now only renders once. The message text has been updated, and I fixed an "undefined index" by also setting the "body" value.

Ready for testing and review. 😃

Screenshot:

[docwilmot]

Sorry to be a bikeshedder, but I kind of assumed the "1 hour" was just example text; we cant leave that, since we cant know cron will run in an hour. (Re #5144, if the site's on Poor Mans Cron, then we can grab that but there's no way to determine server cron intervals is there?) So I suspect we'll need to stick to "try again later" or be explicit and say try again once cron has run.

Also, we may be asking a low-level (eg editor) role to ron cron here. Is it OK to tell a non-admin user to run an action they dont have permissions for?

Would complicate the PR I agree but perhaps the message needs some branching.

• if user has no permissions to run cron, leave off the "Run cron" bit
  • if site cron is on, calculate the time and use "try again in x time"
  • if not, then say "try again later" or the dreaded "contact the administrator" (@jenlampton will be having a nervous breakdown by now)
• if the user has permissions and site cron is on, use "try again in x time", otherwise explicitly say "it will be restored after server cron has run. /n <link>Run cron now</link>

[indigoxela]

@docwilmot maybe you want to take over?

TBH, I'm not a big fan of too much discussion / bike sheding. 🤣

[docwilmot]

I said that, but I think my concern is reasonable this time, dont you think? I dont think we can leave the "1 hour" bit in the PR.

[jenlampton]

What about something like this? backdrop/backdrop#3681

I think that for resolving the fatal error, it doesn't have to be perfect (especially with the follow-up issue where we can sort that out). I've liked "Run cron" if the user has access to that page, and I've used the cron_safe_threshold for the time limit. That's the maximum time between cron runs, so it should be a safe assumption that trying again that much later will have resolved the issue.

[jlfranklin]

How about a more robust solution than waiting on cron. Two possibilities:

1. Rename the field tables to be purged when they are originally deleted. The field_purge_data_body and field_purge_revision_body tables can be deleted via cron, and won't be in the way when a new body field is created.
2. Run the purge of those columns as a batch job when a new column of the same name is created.