Rename data-entity-embed-settings attribute

• +++ b/src/Tests/EntityEmbedFilterTest.php
  @@ -177,6 +177,21 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
  +    // data-entity-embed-settings is
  +    // replaced with data-entity-embed-display-settings. Check to see if
  +    // data-entity-embed-settings is still working.
  

  Fix wrapping.

• BC-compatibility is not working. You must have been testing with the default configuration values. Here is how I tested it: embed node with "Label" plugin, disable link, embedded node display correctly, apply patch, rebuild caches, reload and see how embedded node is now displayed with a label that is a link, edit, edit embedded node, see link being enabled since old settings were not parsed correctly.
• We will also need to update text formats. If there is a HTML filter enabled on the same format as the embed button we need to add the new attribute to the list of allowed attributes. We would also need to test that.

I thought that we agreed in the past that having data structures where we throw anything that comes to mind isn't the way to go. Having responsibilities clearly separated has been a guideline for a while and is also a general best practice.

Could a tool that allows overriding of certain fields/data use its own attribute? Is there an issue for that where we could discuss the architecture? Has any development work been done with regards to that? It is impossible to plan further development and roadmap if we do not communicate ideas through standard channels.

Semantics and clear data model are compelling enough for something like this. Specially this close to beta as we are at the moment. Clear standards improve DX and encourage people to use the tools we provided in a way they were meant to be used.

1. +++ b/src/Plugin/Filter/EntityEmbedFilter.php
   @@ -104,6 +106,13 @@ class EntityEmbedFilter extends FilterBase implements ContainerFactoryPluginInte
   +        // data-entity-embed-settings is deprecated, make sure we convert it to
   +        // data-entity-embed-display-settings.
   +        if ($settings = $node->getAttribute('data-entity-embed-settings')) {
   +          $node->setAttribute('data-entity-embed-display-settings', $settings);
   +          $node->removeAttribute('data-entity-embed-settings');
   +        }
   +
   

   Let's check and do this only if the data-entity-embed-display-settings does not exist.

2. +++ b/src/Tests/EntityEmbedFilterTest.php
   @@ -177,6 +177,21 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
   +    // data-entity-embed-settings is
   +    // replaced with data-entity-embed-display-settings. Check to see if
   +    // data-entity-embed-settings is still working.
   +    $content = '<drupal-entity data-entity-type="node" data-entity-uuid="' . $this->node->uuid() . '" data-entity-embed-display="default" data-entity-embed-settings=\'{"view_mode":"full"}\' data-align="left" data-caption="test caption">This placeholder should not be rendered.</drupal-entity>';
   

   Comment wrapping.

   I'd also make sure non-default display settings are used (full view mode will be used by default AFAIK). Only in that case we prove that they are really respected.

3. +++ b/src/Tests/EntityEmbedUpdateHookTest.php
   @@ -40,4 +41,24 @@ class EntityEmbedUpdateHookTest extends UpdatePathTestBase {
   +        'allowed_html' => 'data-entity-embed-settings',
   

   Let's add full drupal-entity tag instead just one argument. For the paranoia sake...

4. +++ b/tests/fixtures/update/entity_embed.update-hook-test.php
   @@ -23,6 +48,7 @@ $extensions = $connection->select('config')
   +$extensions['module']['embed'] = 8000;
   

   Do we need to rely on this?

5. +++ b/src/Plugin/Filter/EntityEmbedFilter.php
   @@ -104,6 +106,13 @@ class EntityEmbedFilter extends FilterBase implements ContainerFactoryPluginInte
   +        // data-entity-embed-settings is deprecated, make sure we convert it to
   +        // data-entity-embed-display-settings.
   +        if ($settings = $node->getAttribute('data-entity-embed-settings')) {
   ...
   +          $node->removeAttribute('data-entity-embed-settings');
   +        }
   

   Let's check and do this only if data-entity-embed-display-settings doesn't exist.

6. +++ b/js/plugins/drupalentity/plugin.js
   @@ -82,8 +82,8 @@
   -        allowedContent: 'drupal-entity[data-entity-type,data-entity-uuid,data-entity-embed-display,data-entity-embed-settings,data-align,data-caption]',
   
   +++ b/src/Form/EntityEmbedDialog.php
   @@ -154,12 +154,12 @@ class EntityEmbedDialog extends FormBase {
   -      'data-entity-embed-settings' => array(),
   -    );
   +      'data-entity-embed-display-settings' => isset($form_state->get('entity_element')['data-entity-embed-settings']) ?: [],
   +    ];
   

   Doesn't work correctly. Sets 'data-entity-embed-display-settings' to boolean instead of array of configuration in case of the old attribute name. (isset()...)

This should fix the failing test.

Tested this manually and I can confirm that backward compatibility works as expected.

I will tentatively RTBC this due to questions that were raised. Let's try to resolve any difference in views but let's try to do it in a timely manner. Specially due to the beta blocker status of this issue.

Committed.