statamic / cms

The core Laravel CMS Composer package
https://statamic.com
Other
3.7k stars 508 forks source link

Deleting Fieldset/Field causes Exception in Blueprint #3061

Closed daylightstudio closed 9 months ago

daylightstudio commented 3 years ago

Bug Description

Exception appears if you first delete a Fieldset and then go to a Blueprint that was using that Fieldset.

How to Reproduce

  1. Create a Fieldset and add it to a blueprint.
  2. Delete the Fieldset
  3. Go back to the Blueprint to remove it.

Note an Exception appears (at least it did for me). Current workaround is to simply delete it from the Blueprint yaml file (which is easy enough but still seems like a warning should appear).

Extra Detail

Environment

Statamic 3.0.36 Pro Laravel 8.20.1 PHP 7.3.7 rias/statamic-redirect 1.3.1 spatie/statamic-responsive-images 1.7.0 statamic/seo-pro 2.1.0

Install method (choose one):

duncanmcclean commented 3 years ago

There's not really any way to fix this problem. Statamic can't really go through every blueprint, checking for fieldset usage and change it/remove it.

The best that can probably be done here is to display a warning on the delete prompt that is displayed when you delete a modal.

daylightstudio commented 3 years ago

Is it because it would be too expensive to comb through all the blueprints or do you think it technically can't really be done (I realize this would be pretty difficult to implement a "cascade delete" type feature)?

Perhaps there's a separate utility to prune dead field/fieldset references (again, I realize this is not easy)?

This isn't so much an issue for a developer, however, if a non-developer (someone who doesn't have access to edit the files) accidentally deletes a field used somewhere else, they are stuck with a broken screen (and yes, there is the argument of proper user permissions in the first place in Statamic for having a user that doesn't have direct access to the files able to delete).

duncanmcclean commented 3 years ago

Not that it can't be done but it's certainly not that easy. Who knows where the fieldset and any of the fields from inside it are used.

It's been said before that they're not going to implement the same kind of thing for deleting roles or deleting assets.

In my opinion, I don't think a non-developer should have permission to be able to delete things like fieldsets or blueprints in case they make a mistake and mess the site up without knowing what they are doing.

jonassiewertsen commented 3 years ago

Instead of returning null and thereby throwing an exception when the fieldset could not be found, we could return a html fieldset with a message, that the imported fieldset does not exist.

A first draft approach (which does not cover everything), can be found here with more explicit information:

3314

This would be the best approach in my opinion.

samburgers commented 2 years ago

Just chiming in here as a first time Statamic user... this issue has really thrown my confidence in the CMS a number of times while evaluating and exploring the best way to setup fieldsets, blueprints, collections for my project. My assumption from other CMS is that no amount of control panel configuration should really ever result in a server error... but that has happened many times leading me to hard reset the entire site and content (it also breaks the graphql endpoint entirely).

Good to understand the problem however, and next time it happens will try hand removing the blueprint fields. I agree with the approach in https://github.com/statamic/cms/pull/3314 - highlighting the broken field in the cp, and allowing you to clean it up in the cp would make a lot of sense. hope this can be bumped up the priority queue 👍

godismyjudge95 commented 11 months ago

I hate to bring up this issue again, but I think having a graceful way to fail when fieldsets don't exist is kind of important for stability. Also this issue doesn't just affect the blueprint editor it affects any edit page using the fieldset.

There is already precedent for how to handle set errors in bard: https://github.com/statamic/cms/blob/4.x/resources/js/components/fieldtypes/bard/Set.vue#L9 https://github.com/statamic/cms/blob/4.x/resources/js/components/fieldtypes/bard/Set.vue#L151-L153

If I get the time I'm going to draft up a PR for this type of graceful failure.