Problem/Motivation

when an invalid image is selected within the EntityEmbedDialog, there's no specific warning to that effect. The "image:image" display is simply removed from the list of display plugins, and if it's the only one you receive this error:

error example

This is confusing, and it took me a while to figure to out what is going on.

Steps to recreate the bug manually.

1 Create entity embed button with "File" entity selected and "Image" display plugin selected.

langcode: en
status: true
dependencies:
  module:
    - entity_embed
    - file
    - image
label: 'Image Embed'
id: image_embed
type_id: entity
type_settings:
  entity_type: file
  bundles: {  }
  display_plugins:
    - 'image:image'
  entity_browser: ''
  entity_browser_settings:
    display_review: false
icon_uuid: null
icon_path: null

2) Try to embed through a button in the wysiwyg.

Expected, If a valid image is selected, EntityEmbedDialog moves to the "Embed" step. If an invalid image is selected, an error message is displayed, "This entity is not a valid image. Please select another entity."

Actual, Error appears: "No display options available for the selected entity. Please select another entity."

Steps to recreate the issue in code:

use Drupal\file\Entity\File;    

    file_put_contents('public://llama.jpg', '');
    $image = File::create(['uri' => 'public://llama.jpg']);
    $image->save();

$contexts = [
  'entity' => $image,
  'entity_type' => 'file',
];

$plugins = \Drupal::service('plugin.manager.entity_embed.display')->getDefinitionsForContexts($contexts);

dump($plugins); die();

Expected, various image display plugins should be present, but aren't.

actual list of plugins, image not returned.

This is despite the admin form having allowed me to select the "image:image" plugin.

Proposed resolution

A more specific message should be displayed when an invalid image is selected when using the ImageFieldFormatter display plugin.

Remaining tasks

- reviews

Comments

oknate created an issue. See original summary.

oknate’s picture

Title: getDefinitionsForContexts on image file's fails to return selected "Image" plugin. » getDefinitionsForContexts on image file fails to return selected "Image" plugin.
oknate’s picture

oknate’s picture

Issue summary: View changes
oknate’s picture

Issue summary: View changes
oknate’s picture

Issue summary: View changes
oknate’s picture

Title: getDefinitionsForContexts on image file fails to return selected "Image" plugin. » getDefinitionsForContexts not returning image:image
oknate’s picture

Seems to be a bug in ImageFieldFormatter::access().

oknate’s picture

OK, I think this whole issue is a little off.

It turns out if the image isn't valid, instead of telling you the image isn't valid, it tells you that no display plugins are available.

It would make more sense to display an error message "The selected image isn't valid."

oknate’s picture

Title: getDefinitionsForContexts not returning image:image » No invalid image warning, instead confusing message displayed.
Issue summary: View changes
oknate’s picture

Priority: Major » Minor
oknate’s picture

An additional strange thing is that EntityEmbedDisplayManager::getDefinitionsForContexts() iterates through all available plugins and calls their access function when in the context of the EntityEmbedDialog we have an embed button with a list of available plugins.

In my case that was 162 different plugins! When I only had one on the list of available for my embed button!

Oh, on further inspection, it pulls them from cached definitions (DefaultPluginManager::getDefinitions) and allows other modules to alter them, etc. so that's all good, but it still calls the access function on each one, which is quite slow. We can prefilter them in the context of having a shorter list from the embed button's type settings.

I added a separate issue for this: #3022990: EntityEmbedDisplayManager::getDefinitionsForContexts checks access on irrelevant plugins

oknate’s picture

Title: No invalid image warning, instead confusing message displayed. » No image specific warning when image invalid
oknate’s picture

Here's a patch that adds an error message that the image is invalid.

oknate’s picture

Issue tags: +Needs tests
oknate’s picture

StatusFileSize
new8.65 KB

Adding test coverage.

oknate’s picture

Status: Active » Needs review
Issue tags: -Needs tests
oknate’s picture

Issue summary: View changes
oknate’s picture

Title: No image specific warning when image invalid » No image-specific warning displays when invalid image selected
wim leers’s picture

Title: No image-specific warning displays when invalid image selected » For @EntityEmbedDisplay=image plugin: no image-specific warning displays when invalid image selected
Status: Needs review » Reviewed & tested by the community
Issue tags: +Usability
StatusFileSize
new5.55 KB
new10.68 KB

What even is "an invalid image"? This is an area of Entity Embed that still frightens me; I generally don't dare commit things in this area.

However, per point 1 below, the actual problem and fix are obvious once looking at the code. Plus additional test coverage is better than the status quo. So I'm going ahead and committing this :)

  1. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/ImageFieldFormatter.php
    @@ -117,7 +117,14 @@ class ImageFieldFormatter extends FileFieldFormatter {
    +        $isValidImage = $this->imageFactory->get($entity->getFileUri())->isValid();
    

    Nit: should be $is_valid_image per Drupal coding standards.

  2. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/ImageFieldFormatter.php
    @@ -117,7 +117,14 @@ class ImageFieldFormatter extends FileFieldFormatter {
    +          \Drupal::messenger()->addMessage($this->t('The selected image "@image" is invalid.', ['@image' => $entity->label()]), 'error');
    

    Nit: should inject messenger.

    Note: using messenger directly instead of a validation error is problematic, but that's a problem deeper in the Entity Embed's architecture, this pragmatically improves usability in avoiding mysterious errors without any actionable user feedback at all. So it's a step forward.

  3. +++ b/tests/src/FunctionalJavascript/ImageFieldFormatterTest.php
    @@ -0,0 +1,214 @@
    +    // Create Filtered HTML text format and enable entity_embed filter.
    +    $format = FilterFormat::create([
    +      'format' => 'embed_test',
    +      'name' => 'Embed format',
    +      'filters' => [],
    +    ]);
    +    $format->save();
    

    This is not quite the truth :) But since this is testing the behavior of a button, it's fine! The comment is just inaccurate. Removing it.

  4. +++ b/tests/src/FunctionalJavascript/ImageFieldFormatterTest.php
    @@ -0,0 +1,214 @@
    +    $this->assertSession()->responseNotContains('The selected image "' . $this->image->label() . '" is invalid.');
    

    👍

wim leers’s picture

Title: For @EntityEmbedDisplay=image plugin: no image-specific warning displays when invalid image selected » @EntityEmbedDisplay=image plugin: no image-specific warning displays when invalid image selected
oknate’s picture

Sweet, thanks for the update. The interdiff looks good.

I used the language "invalid image" based on:

$this->imageFactory->get($entity->getFileUri())->isValid();

So it's very specific to imageFactory.

  • Wim Leers committed b9eb672 on 8.x-1.x authored by oknate
    Issue #3022754 by oknate, Wim Leers: @EntityEmbedDisplay=image plugin:...
wim leers’s picture

Status: Reviewed & tested by the community » Fixed

👍

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.