Logs flooded with warning messages "Invalid display settings encountered"

+1

Oh man this was driving me bananas, thank you @bkosborne!

Love the added comma ;-)

Should the "empty" check come first.....? Or is it no difference either way?

Patch applies cleanly *and* quells the flood of messages for me. (! screams in relief on the inside !)

Tested and working. Thanks for the patch.

Hi,

Attaching a patch to filter empty $context values before merge default values.

@dragos-dumi: Would you be willing to?:

A) Explain why you think you need the change you're proposing. At first glance, it's causing lots of existing tests to fail, which smells like a regression.

B) Provide interdiffs with your patches.

C) Use a follow-up issue to work on your change. #3 is RTBC for fixing a very common bug affecting a bunch of sites. While you/we sort out what you're trying to accomplish, at least folks could get relief from the bug described/fixed here. Just looking at patch #9 it seems basically unrelated to what's happening here.

D) Write a test that shows the bug you're trying to fix.

I'm re-uploading exactly patch #3 (so the testbot doesn't get confused) and restoring the RTBC status.

I've been using this patch in production and it's a huge win for silencing bogus warnings in the log. Patch looks clean. We could complain there's no test coverage showing the bug we're fixing. ;) But I'm not sure we need to hold up progress for that. I'll leave that for the maintainers to decide...

Thanks,
-Derek

p.s. Another note to committers: d.o is now confused about issue credits and patch authorship here. Please list @bkosborne as the author of this fix and give them first listing in the commit message. IMHO, I'd remove credit/authorship from @dragos-dumi for this issue, and give them credit in the forth-coming follow-up issue if/when that's fixed.

Just, FWIW -- #10 works great, thank you all!

Patch in #10 worked for me as well.

#10 fixes the issue, retriggered tests as the failures looked unrelated.

Patch #10 fixed this for me as well. We've been running this in production for a while.

See https://www.drupal.org/project/entity_embed/issues/3069448#comment-13315334

Hello,

Patch #10 fixed the issue for me too. Thanks!

Note this patch isn't fixing anything, it's merely working around displaying the warning in the logs. The underlying issue is solved via a combination of; https://www.drupal.org/project/entity_embed/issues/3069448#comment-13315334

...and editing your content to remove any instances of where the filter has previously entered;

data-entity-embed-display-settings=""

This is the actual issue - as the empty string is obviously then not an array.

Hello,

Patch #10 works for me.

Thanks

Is there any chance we could get automated test coverage for this? Even a simple kernel or unit test would be sufficient, IMHO.

Again, this is only fixing the symptoms of the problem - it isn't fixing the problem.

Comment #10 does prevent flooding the logs, so it is useful to a certain extend.
However why not adding the extra argument to the original if-statement,
that would clean up the patch file as well since you only change one line.

@pobster We should definitely also solve the actual problem. However, even if that would prevent the issue for future additions, if you already have a site filled with entries with this problem, you're up a certain creek without a paddle. I don't think trying to convert existing entries containing the problem in an update hook is in any way practical, nor is asking a client to go through all content to resave content. Is there an alternative where we don't hide the problem but also do not flood the logs?

Use the patch from #3069448: Array to string conversion for Media Image to remove empty data-entity-embed-display-settings attribute for new/ updated content and this patch for existing content.

Hii I upgraded php 7.4 to php 8.0 for testing on my dev environment. When i was testing the pages while using dblog module , I caught this error:

Invalid display settings encountered. Could not process following settings for entity type "media" with the uuid

so I was trying to find workaround to solve this issue. I found this patch and added it. Patch #10

diff --git a/src/EntityEmbedBuilder.php b/src/EntityEmbedBuilder.php
index 892ec3a..1b1188c 100644
--- a/src/EntityEmbedBuilder.php
+++ b/src/EntityEmbedBuilder.php
@@ -59,15 +59,19 @@ class EntityEmbedBuilder implements EntityEmbedBuilderInterface {
       'data-entity-embed-display-settings' => [],
     ];
 
-    // If the data-entity-embed-display-settings isn't an array reset  it,
+    // If the data-entity-embed-display-settings isn't an array, reset it,
     // otherwise we'll encounter a fatal error when calling
     // $this->buildEntityEmbedDisplayPlugin() further down the line.
     if (!is_array($context['data-entity-embed-display-settings'])) {
-      \Drupal::logger('entity_embed')->warning('Invalid display settings encountered. Could not process following settings for entity type "@entity_type" with the uuid "@uuid": @settings', [
-        '@settings' => $context['data-entity-embed-display-settings'],
-        '@entity_type' => $entity->getEntityTypeId(),
-        '@uuid' => $entity->uuid(),
-      ]);
+      // Log a warning if it was set to a some non-empty value, which is an
+      // invalid config value that admins should be made aware of.
+      if (!empty($context['data-entity-embed-display-settings'])) {
+        \Drupal::logger('entity_embed')->warning('Invalid display settings encountered. Could not process following settings for entity type "@entity_type" with the uuid "@uuid": @settings', [
+          '@settings' => $context['data-entity-embed-display-settings'],
+          '@entity_type' => $entity->getEntityTypeId(),
+          '@uuid' => $entity->uuid(),
+        ]);
+      }
       $context['data-entity-embed-display-settings'] = [];
     }

After adding the patch , I tested on my local environment then dev environment. It is working for me perfectly. No issues found as expected

Thank You!!

Confirm error appears on php 8.1

Applying the patch in #25 suppresses the stuffing of the log with warnings.

re #21 and #17 I mean, those are fixes but we can't just grep a database for every time something wrote a bad value in the past. Empty stream isn't an array but its clearly empty so we handle the value gracefully and only report _real_ errors. In that respect this fixes a bug and as you mention there are others.

Re #22 because $context['data-entity-embed-display-settings'] = []; is still only in the outer diff so they can't be combined.

Lot of happy people and we've been using this for a long time agree the code itself is RTBC but leaving it NR because of the testing.

re #21 and #17 I mean, those are fixes but we can't just grep a database for every time something wrote a bad value in the past.

What are error messages for, if not to tell you that something has been entered incorrectly? This is a genuine error - what's been entered is wrong. You're just saying, "I want to see error messages, just not this one" - it's not ideal for sure but it's also not that difficult to loop through content, indeed I only did it last week in an update hook.

/**
 * Example batch update hook.
 */
function just_an_example(&$sandbox) {
  $entity_type_manager = \Drupal::entityTypeManager();
  $node_storage = $entity_type_manager->getStorage('node');
  $limit = 100;

  // Set total results.
  if (!isset($sandbox['total'])) {
    $string_long_fields = array_map(fn ($field) =>
    $field->getName(),
      $entity_type_manager
        ->getStorage('field_storage_config')
        ->loadByProperties([
          'entity_type' => 'node',
          'type' => 'string_long',
        ])
    );
    $text_long_fields = array_map(fn ($field) =>
    $field->getName(),
      $entity_type_manager
        ->getStorage('field_storage_config')
        ->loadByProperties([
          'entity_type' => 'node',
          'type' => 'text_long',
        ])
    );
    $text_with_summary_fields = array_map(fn ($field) =>
    $field->getName(),
      $entity_type_manager
        ->getStorage('field_storage_config')
        ->loadByProperties([
          'entity_type' => 'node',
          'type' => 'text_with_summary',
        ])
    );
    $sandbox['fields'] = $string_long_fields + $text_long_fields + $text_with_summary_fields;

    $sandbox['bundles'] = [];
    foreach ($sandbox['fields'] as $key => $field_name) {
      $sandbox['bundles'] += $entity_type_manager
        ->getStorage('field_storage_config')
        ->load($key)
        ->getBundles();
    }

    $sandbox['current'] = $sandbox['count'] = $sandbox['changed'] = 0;

    $sandbox['nids'] = [];
    foreach ($sandbox['fields'] as $field) {
      $sandbox['nids'] = array_merge(
        $sandbox['nids'],
        \Drupal::database()->select("node__{$field}", 'f')
          ->fields('f', ['entity_id'])
          ->condition("f.{$field}_value", 'data-entity-embed-display-settings=""', 'REGEXP BINARY')
          ->condition('f.bundle', $sandbox['bundles'], 'IN')
          ->execute()
          ->fetchCol()
      );
    }
    $sandbox['nids'] = array_unique($sandbox['nids']);
    sort($sandbox['nids']);
    $sandbox['total'] = count($sandbox['nids']);
  }

  $nids = $node_storage->getQuery()
    ->accessCheck(FALSE)
    ->range(0, $limit)
    ->condition('nid', $sandbox['nids'], 'IN')
    ->condition('nid', $sandbox['current'], '>')
    ->sort('nid', 'ASC');
  $nodes = $nids->execute();
  $nodes = $node_storage->loadMultiple($nodes);

  // Cycle through each node.
  foreach ($nodes as $node) {
    $languages = $node->getTranslationLanguages();

    // Cycle through each language to check/ update the field values.
    foreach ($languages as $langcode => $lang_obj) {
      // Cycle through each translation (you may need hasTranslation() here if you don't use them).
      $cs_node = $node->getTranslation($langcode);
      $changed_node = FALSE;
      // Cycle through our fields.
      foreach ($sandbox['fields'] as $field_name) {
        if ($cs_node->hasField($field_name)) {
          $value = $cs_node->get($field_name)->getValue();
          $changed_field = FALSE;
          foreach ($value as &$content) {
            if (preg_match_all('/data-entity-embed-display-settings=""/', $content['value'], $matches)) {
              foreach ($matches[0] as $replacement) {
                // Blank out the match and flag to action a save operation.
                $content['value'] = str_replace($replacement, '', $content['value']);
                $changed_field = $changed_node = TRUE;
              }
            }
          }
          if ($changed_field) {
            $cs_node->set($field_name, $value);
          }
        }
      }
      if ($changed_node) {
        $sandbox['changed']++;
        $cs_node->save();
      }
    }
    $sandbox['current'] = $node->id();
    $sandbox['count']++;
  }

  $sandbox['#finished'] = empty($sandbox['total']) ? 1 : ($sandbox['count'] / $sandbox['total']);
  \Drupal::messenger()->addStatus($sandbox['count'] . ' nodes processed out of ' . $sandbox['total']);
  \Drupal::messenger()->addStatus($sandbox['changed'] . ' nodes altered');
}

Obviously, this is untested as the original code was for something else, but this is not a difficult task.

I hate to be that guy, but updating nodes is not an option.

We have a 6 different sites with the main one topping at 4k nodes, grepping and stripping stuff from the entire prod databases gets me fired instantly and frowned upon from other devs.

Batching through all nodes and re-saving everything messes up the content view/last work for authors, we've done once previously and swore we never ever do it again not even under death threat.

What is the proper solution then because as it stands now, every time an entity is embedded using the button it comes through with data-entity-embed-display-settings as empty string.

So, this is not even user mistake from what I can see on our end, even more, why user-generated content is not gracefully managed? Feels like everyone has an easy way to trigger server errors by just edit the attribute.

Thanks for #10, much appreciated.

I hate to be that guy, but updating nodes is not an option.

mysql> SELECT COUNT(nid) FROM node;
+------------+
| COUNT(nid) |
+------------+
|     446061 |
+------------+
1 row in set (1.26 sec)

What is the proper solution...

I've already stated the proper solution and my views on it - but if hiding the error works for you, that's also fine? Go for it, it doesn't have to be a perfect solution - but still, it's a workaround rather than a fix. Hence this patch shouldn't be merged.

Ok, let me rephrase the issue here:

I have an entity embed button on my CKEditor, everytime the content managers press the embed entity button the tag gets generated with an empty attribute as default.
My authors are absolutely non-HTML savvy therefore the text format I am offering to my authors doesn't have any way to access the field text as source. They are great at writing text but not editing attributes on tags.

So, let's assume I grep all node but this won't solve future content being written. Grepping all content is not the solution here.

Ah! Now we get to the root of the problem! This is what I mentioned in a previous comment - the fix is here: https://www.drupal.org/project/entity_embed/issues/3077225#comment-14016784 / https://www.drupal.org/project/entity_embed/issues/3069448

Yes, this module has written bad config into your content - I can't see why you don't think that removing bad/ invalid config from your content is not the solution. If you can't node save in an update hook, then don't - just do it with a straight database query. Or ... as I said before, apply the patch to hide the warning - I just don't believe that a patch which hides a legitimate error should be merged into the project. But it isn't my project so who cares!!!

Yeah, maybe its possible to automate updating nodes but it doesn't actually fully address the problem because you'd actually be looking at updating any field-able entity. This means users, paragraphs, webforms, ECK entities, custom site specific entities, etc. Those updates also have side effects so its not without risk.

Technically even if you figured that out and fixed ever field-able entity you still wouldn't have solved the problem though because you can use filters outside of Fields API. For example I've got a site that has a full text field stored as a pair of values in KV because its a site setting but not something that can be stored in config for deployment reasons.

So I agree that when possible this module should fix the value. I just strongly disagree "fix all the field in an update" is the correct solution. Filters need to be as resilient as possible and the intent of the empty config is clear and can be handled without causing site side effects.

I'll go a step farther and say even if fixing all the previous bad values in an update was going to be implemented, this issue stands as its own fix that should be implemented because as I said, filters should be as resilient as possible.

So I agree that when possible this module should fix the value. I just strongly disagree "fix all the field in an update" is the correct solution.

Same! This is why I didn't bother putting that code in a patch file. I stuck it in here so people could decide whether or not they want to use it - in no way should it be forced on people to run in the module code. About your other point, obviously, it can cycle through all available entities too - it was just a base point to show how it's possible to "surface" all text-based fields for processing.

As I said before if this patch suits you - use it. Just, I don't feel like it's a solution that should be merged into the module, because it is hiding a legitimate error which yes, this module introduced - but nonetheless, it's still a legitimate error.

I faced the same issue and fixed it, instead of applying patch #10 I decided to know what is the root cause.
In my case I found out another related error which helped me out

Drupal\entity_embed\Exception\EntityNotFoundException: Unable to load embedded media entity 36beb6db-45f7-40a9-8799-b4fa8388cbc1. in Drupal\entity_embed\Plugin\Filter\EntityEmbedFilter->process() (line 162 of /var/www/html/xxx/web/modules/contrib/entity_embed/src/Plugin/Filter/EntityEmbedFilter.php).

and followed the below comment
https://www.drupal.org/project/entity_embed/issues/2982322#comment-12866696

Coming back to this issue after a long time, it seems like #3069448: Array to string conversion for Media Image is the correct fix. With that patch I can re-edit an embedded media entity and it correctly removes the empty string settings variable.