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

allsite commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

Did you test vs fresh install?

indigoxela commented 3 years ago

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 commented 3 years ago

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

allsite commented 3 years ago

Also reproduced it on a backdropcms.org Demo site.

indigoxela commented 3 years ago

Weird...

Some screenshots from my attempt:

new-type

dblog

allsite commented 3 years ago

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 commented 3 years ago

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

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

There's clearly something magical about your server!

Ha! See. :laughing:

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 commented 3 years ago

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 commented 3 years ago

Where would I find Backdrop 1.20.x-dev?

On GitHub :wink: 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 commented 3 years ago

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

indigoxela commented 3 years ago

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

Cool, please report your findings back here.

indigoxela commented 3 years ago

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:

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. :deer:~~ hunting season closed. :laughing:

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 commented 3 years ago

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 commented 3 years ago

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 :grin:, 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 commented 3 years ago

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

warning-node-type

laryn commented 3 years ago

@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 commented 3 years ago

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

Definitely better - shorter is always better. :+1: It's on the form, though, so before not adding it. warning-add-type

laryn commented 3 years ago

@indigoxela

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

?

indigoxela commented 3 years ago

... 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 commented 3 years ago

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.

I included a work-around for avoiding a duplicate message in https://github.com/backdrop/backdrop/pull/2955/files#diff-a403e810ef49abede3d7b616c873b7ea568cd32d578852fe23ded010a3f8aa77R4963

I don't love it, but I think our messages array needs keys (API change?) in order to handle this situation gracefully.

jenlampton commented 3 years ago

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

Two thoughts:

1) Why do we even allow the body field to be marked for deletion?

The body field is not a custom field (as created using the Field UI), instead it was provided by a core system. If someone were to delete it, the only way to get it back would be to write and run an update hook. Sure, you could add custom field named body (machine name: field_body) but it would not be the same thing as the original body field (machine name: body)

It makes sense to me that any Custom field (added or replaceable via Field UI) could be deleted, but the body field is special. Shouldn't it be permanent? (This can be a follow-up issue if you agree)

2) In the message, I think we should also instruct the reader how to resolve the problem.

"The old Image field has been marked for deletion and will not be available for reuse. If you would like to add a new field named field_image, please run cron now, or try again later."

I'm certain this bug came from D7

@allsite We have improved the situation slightly over Drupal 7. In Backdrop, if the field is empty, it will be deleted immediately (without needing a cron run). With Drupal, deleting any field always requires a cron run.

@indigoxela I expect the reason you were unable to reproduce the issue on your local site was that you didn't have any content in that field (started with minimal install profile maybe? Or deleted a different field than Body?)

In that scenario, you would have hit the use-case Backdrop had fixed: deleting a field immediately after adding one (perhaps after making a mistake, or changing your mind on field structure) and attempting to re-add the same field immediately.

I still very much dislike fatal errors -- and I agree with the milestone candidate label -- so I have added this issue to the next bug-fix milestone.

indigoxela commented 3 years ago

Why do we even allow the body field to be marked for deletion?

IMO it's OK to let people do radical things like deleting all content types. After all fields got purged, the Body field will be available again, as it ships with the (required) node module. It's just a short time span, where it's not available.

Re field_image. I think, that field has no special purpose. It's always possible to create new (image) fields, so why warning people about something, that's no actual problem. And where, anyway? Unlike the Body field, it's not attempted to automatically add it to any new content type.

I expect the reason you were unable to reproduce the issue on your local site was that you didn't have any content in that field

I tried the same steps several times on an every time freshly created Backdrop install. So there were the usual two field db table entries for the "first post" and the "about" page. I also kept an eye on the field json files in the files directory and the database tables. I wasn't able to reproduce it locally, as both, the database tables and the field definition json files immediately got purged. But I found my workaround to reproduce the problem. :smile:

@jenlampton many thanks for the link to your message repetition workaround. It looks a little hackish, though. I'm leaning towards letting the message appear again after form submission and trying to let the wording fit both places.

But now back to wording: Should we add some more explanation (mention cron...)? Personally, I'm a friend of short messages. If people are on the form, they're in the middle of their workflow, so I'd assume that too long messages get ignored, anyway. :wink:

docwilmot commented 3 years ago

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.

jenlampton commented 3 years ago

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 https://github.com/backdrop/backdrop-issues/issues/5141#issuecomment-894721582, 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 commented 3 years ago

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

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

jenlampton commented 3 years ago

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 commented 3 years ago

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

laryn commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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

Screenshot: content-type-message

docwilmot commented 3 years ago

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 https://github.com/backdrop/backdrop-issues/issues/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 commented 3 years ago

@docwilmot maybe you want to take over?

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

docwilmot commented 3 years ago

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 commented 3 years ago

What about something like this? https://github.com/backdrop/backdrop/pull/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 commented 3 years ago

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

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.
Run the purge of those columns as a batch job when a new column of the same name is created.

backdrop / backdrop-issues

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