Use EntityDeleteForm for entity deletion

Your report here makes it seem that #2860578: Support for blocks in panels is not needed? I haven't tried this yet but when we fix this, we should also add a test that uses the panels module

This is a bug that affects the data integrity, so we should do this before we tag the first beta.

@rmarquest, thanks for #3!

I haven't tried this yet but when we fix this, we should also add a test that uses the panels module

I've actually been thinking about this more and we get requests about panels quite often but I'm a little bit hesitant to add panels-specific tests, while it would be nice to have test-coverage, I haven't used panels in d8 yet and have no idea how it works.

So that task is daunting. Not sure if we should add page_manager or panelizer as well.
I'm afraid to add code that I don't understand. I'll have to ask some of the other maintainers if they use/understand panels well enough to know what to do here.

I'll try to get a hold of the panels team to see if this is something that should be fixed on our end or on theirs.

I think that this is a problem with Page Manager. I also have a page that is displaying facet blocks via Panels. I checked the page variant's configuration. It lists all of the facet blocks as configuration dependencies.

I'd love to verify this issue personally, but I can't right now. The one site I have with this configuration set up is being used this weekend for demo purposes. People would be mad at me if I broke it.

I don't know everything about config dependencies, but generally when a config dependency is deleted then all of its dependent configurations get deleted too. I'm pretty sure that's why this is happening. Based on what I know, Page Manager needs to react to the config delete event in order to remove the facet block (or any other block) from its configuration in a non-destructive manner. I looked and it doesn't.

One of the Page Manager maintainers is in my DUG. I'll ask him about this at this month's meeting. I think you can probably ignore this issue though.

I don't know everything about config dependencies, but generally when a config dependency is deleted then all of its dependent configurations get deleted too. I'm pretty sure that's why this is happening. Based on what I know, Page Manager needs to react to the config delete event in order to remove the facet block (or any other block) from its configuration in a non-destructive manner. I looked and it doesn't.

This is what I figured as well. I hope that we can just close this issue instead of actually having to write test-coverage and code to fix this.

One of the Page Manager maintainers is in my DUG. I'll ask him about this at this month's meeting. I think you can probably ignore this issue though.

That would be awesome. I tried tweeting about this: https://twitter.com/Borisson/status/866710249729806337 https://twitter.com/Borisson/status/855747360420491265 but haven't gotten an answer yet.

If it turns out that we do have to fix this in our own code, I'm happy to give it a try but I'd love to get at least feedback on what modules we should use on our side to get accurate testcoverage.

Thanks @dcam!

@EclipseGc is the person from my DUG. I'll get an answer from him.

I talked to @EclipseGc last night. I confirmed the bug while talking to him (which I should have done beforehand). When I did, I realized that the facet delete form doesn't warn the user that other dependent config entities will be deleted too. So that's bad. Anyway, my page variant was deleted along with the facet due to the dependency. EclipseGc agreed that it's on Page Manager to non-destructively remove blocks from its configuration, but we agreed that it's on Facets to warn people that it's about to happen.

After the meeting I looked into what gives most config delete forms the list of other dependencies that will be deleted. It's done by extending \Drupal\Core\Entity\EntityDeleteForm instead of EntityConfirmFormBase. Turns out I did the same thing earlier this week when I created a custom config entity for the first time. The official docs say that EntityConfirmFormBase should be extended. I wonder if you (or whoever wrote this class) was reading the same thing. I'm going to go see about editing that doc after this.

Anyway, I've attached a patch. Most of the existing class can be removed because it just duplicates what EntityDeleteForm does. Check it out. Let me know if I need to add a test for this. I didn't see a pre-existing one that I needed to modify, but I may have missed it.

Thanks! Good to know all that information. I changed the test so that we now have explicit coverage.

There may be an alternative. I think you can change the facet entity annotation and set the delete form directly to EntityDeleteForm. Then you could have one less class in the module.

The caveat is that the change requires you to have an entity.facets_facet.collection route defined because that's what EntityDeleteForm uses as its cancel URL, so facets.overview would have to be renamed. That means changing ~11 places in the module that refer to the facets.overview route. That could also be disruptive for anyone else who might be using the facets.overview route, but this is still in alpha, so maybe disruptive changes are ok. I don't know. It's up to you. I can write a patch for it if you're interested.

There may be an alternative. I think you can change the facet entity annotation and set the delete form directly to EntityDeleteForm. Then you could have one less class in the module.

Yes, deleting code is the best thing!

The caveat is that the change requires you to have an entity.facets_facet.collection route defined because that's what EntityDeleteForm uses as its cancel URL, so facets.overview would have to be renamed. That means changing ~11 places in the module that refer to the facets.overview route. That could also be disruptive for anyone else who might be using the facets.overview route, but this is still in alpha, so maybe disruptive changes are ok. I don't know. It's up to you. I can write a patch for it if you're interested.

This issue has been marked as a beta blocker for weeks, so it was to be expected that this could lead to potential breaks. I don't think this is very dispruptive either, this is only important for others who've written code that links to the facet overview - and I don't know of anyone doing that (in contrib at least).

Needs work based on #12.

As I ran a find and replace for the old route name I realized that the Facets Summary delete form has the same problem. I'm editing it as well and trying to add a test. I don't think the summary delete form class can be removed entirely though because the summaries won't have their own collection route. In that case, the class will remain and only override the getCancelUrl() method.

I added a new test for facet summary block deletion.

Sorry. Status.

Looks great! Thanks.

The title wasn't representative of the suggested change anymore since the Facet delete form was removed entirely. Though it's kind of redundant now. It won't bother me if you change it to something else.

Committed and pushed.

It seems that problem is still actual. I use latest dev-version of facets and when I delete facet, page variant is deleted too. May be solution must be more complex and more obvious, something like this:

if (\Drupal::moduleHandler()->moduleExists('page_manager')) {
 $pageVariantStorage = \Drupal::entityTypeManager()->getStorage('page_variant');
 $block_id = 'facet_block:' . $this->entity->id();
 $page_variants = $pageVariantStorage->loadMultiple();
 /** @var \Drupal\page_manager\Entity\PageVariant $page_variant */
 foreach ($page_variants as $page_variant) {
  $settings = $page_variant->get('variant_settings');
  if (!isset($settings['blocks'])) {
    continue;
  }
  foreach ($settings['blocks'] as $uuid => $block) {
    if ($block['id'] !== $block_id) {
      continue;
    }
    unset($settings['blocks'][$uuid]);
    $page_variant->set('variant_settings', $settings);
    $page_variant->save();
  }
}
 $this->entity->delete();

This must be placed in FacetDeleteConfirmForm in submitForm method.

Please open a new issue and relate it to this one instead of commenting on a year-old, closed, issue.