Mark missing embedded entities in WYSIWYG

That would be indeed a good thing to have. I'd even adventure to say it is a bug that we don't show an inconsistent state clearly to the user.

Agreed, ideas on what should be displayed? What should we display when the entity was loadable but the user doesn't have access to it? Should that still remain not visible? Can we ensure that this only happens when being used in the WYSIWYG context and not during normal rendering? I don't think I was able to solve that latter part originally, so that's the key issue here.

Oh true, we shouldn't mess with the normal rendering, and this might be tricky to differentiate...

Maybe we can simplify the approach if we show an "error/broken" element in both cases, but only for users with a specific admin permission? This way only selected users would see the errors everywhere, but supposedly they are the ones that can correct them too.

@gngn followed up at #2914468-16: EntityNotFoundException in EntityEmbedFilter ("Unable to load embedded file entity"):

[…]
In my case it was indeed a (later on deleted) media inserted in a WYSIWYG field via entity_embed.

In case anyone is interested, that's what I did:

• First I dumped my database with drush sql-dump --ordered-dump > dump.sql
  This results in one-line-per-insert statements
• I searched in the dump for the uuid: grep 'ec06e3d8-1995-48e6-9173-4b40bca7e2ca' dump.sql
• which gave me several lines like
  INSERT INTO `paragraph__field_text` VALUES ('pt_text',0,34260,34276,'en',0,'[...] <drupal-entity data-embed-button=\"embed_document\" data-entity-embed-display=\"view_mode:media.download\" data-entity-type=\"media\" data-entity-uuid=\"ec06e3d8-1995-48e6-9173-4b40bca7e2ca\"></drupal-entity>[...]'
  so it was a paragraph field.
• From there I dug in the database to get the entity embedding the paragraph and edited it in the UI
• But there I cannot see the deleted entity and therefor cannot delete it - see 2982322
• So I had to change to "Full HTML" and "view source" to edit the plain text, remove the <drupal-entity...</drupal-entity>, change back to my restricted filter format and save
• That's it ;)

[…]

(emphasis mine)

My response at #2914468-17: EntityNotFoundException in EntityEmbedFilter ("Unable to load embedded file entity"):

In my case it was indeed a (later on deleted) media inserted in a WYSIWYG field via entity_embed.

Thanks for confirming! Thank you very much for taking the time to not only investigate how to prove this, but then also document the steps! This is super super super valuable. THANK YOU! 👏

The ideal solution here would be to have usage tracking and then forbid deleting a Media entity if it has any usages. But there's a super complex cluster of critical Drupal core issues wrt usage tracking. So we shouldn't block ourselves on that. So yes, this just further confirms the need for #2982322: Mark missing embedded entities in WYSIWYG!

I'm marking this issue fixed and using #16 + the analysis in this comment to elevate the priority of #2982322: Mark missing embedded entities in WYSIWYG. I'll also make sure you are credited on that issue for your research!

That means that without this, we're left with a massive, major usability hole. The ideal solution I described in #17 is not feasible in the short or medium term, which leaves this issue as the only short term solution. Hence bumping to Major. Rewrote the issue summary to assemble all information in one place.

Module maintainers, please credit @gngn on this issue!

#3015730: Demote EntityNotFoundException from error to warning was opened a few days ago, it's a duplicate of this.

I'm proposing another approach:

1. Store the broken embeds in a table, when discovered.
2. Create a report page that lists the tracked orphans.
3. Offer an action (UI batch + drush) to repopulate this table.
4. Offer an action (UI batch + drush) to massively clean the broken embeds by deleting the broken <drupal-entity />, overall or by entity type. VBO could be considered also.

Some more explanation:

1) We could introduce a new table, named like entity_embed_broken or entity_embed_orphan(s) with these fields:

• host_entity_type
• host_entity_id
• host_field_name
• target_entity_type
• target_entity_id

(uuid could also be used)

Each time we end up to an EntityNotFoundException, we db_merge a row on that table.
A message could be shown to the user (permission-based) also.

2) A new report page can list all the existing broken/orphan embeds. So a user with proper permission can manually visit the entities and edit them.
Maybe, additionally, another row in status report, stating that there are orphaned entity embeds.

3) I haven't thought yet how this could be implemented.

4) The "clean up" action (UI batch + drush) can edit the host entities and remove the broken embeds. It can be done by entity type.
Also we can give several options here, e.g. about revisions, update timestamp etc.

What would you think about an approach like this?

That approach sounds awesome, but perhaps would be better in a separate module entity_embed_garbage_collection.

#9 that sounds lovely but …

1. We know that core's file usage tracking is horribly broken, in no small part due to complex interactions with revisions and translations.
2. It'd be largely reimplementing https://www.drupal.org/project/entity_usage.

Hoping @marcoscano can chime in on this one :)

Yes, I agree that we should try to keep the scope here separate from the "data integrity" problemspace, which can quickly go out of hand.

Entity Usage has currently a feature where you can opt-in to a warning message being shown to users whenever an entity (which is referenced from other places) is being deleted. It doesn't prevent deletion, but if necessary custom/contrib code could leverage the info stored there to do that without much overhead.

Entity Embed however should provide a sensible alternative to users when broken relationships do happen. I think figuring out a way to display a placeholder to admins (basically improving the UX for manually removing the reference) would be the ideal path forward to this issue.

I think figuring out a way to display a placeholder to admins (basically improving the UX for manually removing the reference) would be the ideal path forward to this issue.

+1

If it serves as inspiration, this is what core does when an embedded image is missing:

It's worth noting though that this problem in core is significantly mitigated by the fact that there is *no* UI for deleting a permanent file in drupal core. You would only get to the "broken" scenario above if you use some contrib/custom code to somehow get an image deleted outside of the context of the node it was embedded into.

If it serves as inspiration, this is what core does when an embedded image is missing:

That's not what that is though; it's \Drupal\filter\Plugin\Filter\FilterHtmlImageSecure stripping away images that it considers insecure.

Nonetheless, I agree, that could be an example to look at. I'd be careful though; because that also means that it's going to be difficult for a content author to distinguish between "image still exists but is insecure" and "image does not exist anymore"

Ha, interesting! True, it is the filter adding the placeholder if the image is considere insecure or (as in this case) couldn't be retrieved. (I did remove the file entity by code before taking that screenshot).

I agree that a unique placeholder will make more sense in any case.

Thanks!

Rather than make it behave differently in the text editor and in the filtered text shown to the end user, I think it makes more sense to have it behave the same in both situations.

Not only because the existing filter system does not allow to specify arbitrary context (we'd need to know whether we're filtering "for real" or "for preview inside the text editor"). But also because when the end user is reading text and some embedded entity is just omitted without any indication, the text will probably seem broken (since it is likely referring to the embedded entity).

I propose this:

(This is reusing core Media's core/modules/media/images/icons/no-thumbnail.png icon.)

#17: +1!

+++ b/src/Plugin/Filter/EntityEmbedFilter.php
@@ -158,6 +158,7 @@ class EntityEmbedFilter extends FilterBase implements ContainerFactoryPluginInte
+            $entity_output = '<img src="' . file_create_url('core/modules/media/images/icons/no-thumbnail.png') . '">';

Minor: would it be worth adding an alt text to this image such as alt="Deleted content" or something like that?

#18: yeah, I was already doing exactly that, as well as setting width +height attributes and copying the image from the core module into the Entity Embed module.

Actually … that icon has existed in Drupal core since June 2017 (in #2831937: Add "Image" MediaSource plugin), for nearly 1.5 years, it shipped with Drupal 8.4. Drupal >=8.5 (8.5+8.6) are the only supported Drupal core releases. So … I think we don't actually need to copy the file over; we can assume it exists.

And here's a less confusing screenshot for in the IS.

Actually … that icon has existed in Drupal core since June 2017 (in #2831937: Add "Image" MediaSource plugin), for nearly 1.5 years, it shipped with Drupal 8.4. Drupal >=8.5 (8.5+8.6) are the only supported Drupal core releases. So … I think we don't actually need to copy the file over; we can assume it exists.

+1

1. +++ b/entity_embed.info.yml
   @@ -7,5 +7,6 @@ dependencies:
   +  - drupal:system (>=8.4.0)
   

   👏

2. +++ b/src/Plugin/Filter/EntityEmbedFilter.php
   @@ -158,6 +158,7 @@ class EntityEmbedFilter extends FilterBase implements ContainerFactoryPluginInte
   +            $entity_output = '<img src="' . file_create_url('core/modules/media/images/icons/no-thumbnail.png') . '" width="180" height="180" alt="Deleted content encountered, site owner alerted" />';
   

   Super nitpick: Can we add a final period to the alt sentence? :)
   It apparently helps by telling screen readers to take a short break before proceeding.
   https://ux.shopify.com/considerations-when-writing-alt-text-a9c1985a8204...

#23.2: hah, that's exactly why I didn't add a period! I don't feel strongly either way, and we can change this based on real-world feedback.

Also, that string should be translated! 😅

:)

I think this still needs test coverage in \Drupal\Tests\entity_embed\Functional\EntityEmbedFilterTest?

working on it.

IMHO this is ready now, but it'd be great to get +1s from others.

+1 RTBC. Nice improvement.

🎉

Thank you, that's great.
@Wim Leers sadly I wasn't credited...

@gngn: Oh :| I'm very sorry, @gngn!

I can't change the commit, but I've added issue credit for you now! :)

@Wim Leers: thank you!

The #3019308: Follow-up for #2982322: set `title` attribute for missing entities, to provide useful information upon mouseover follow-up just got committed :)

Would you support a patch here to make it so that the option to output the image to non-admins is configurable? (in a new issue of course)

There's a follow-up issue #3065054: Add permission to allow 'missing media' to be hidden from anonymous users, perhaps we could take up the discussion of modification there?