Remove the <drupal-entity>/<div> placeholder instead of embedding the entity as a child

CONS:
Quotes or other text references often need to be inline elements (entity_embed would be a great step forward to ease the creation of hypermedia website for scholarly associations or learned societies)

Yeah, we've seen in File Entity that having the wrapping div is very much undesirable in a lot of cases. If there is a failure (like the entity fails to load) then we should log it.

Maybe we could do the best of both worlds and make it configurable from the button what behavior to use (replace contents inside and leave wrapping div, or replace div entirely)?

I'm starting to feel stronger and stronger about replacing always.

I feel strongly about not copying the data attributes still. If someone wants that behavior, I think they should be able to do so using hook_entity_embed_alter(). I'd rather just replace the content like all other filters do.

I doing this this might have been a mistake.

The first thing I wanted to do was create a class and add it to the "styles" dropdown in CK so I could style embedded entities. This was not possible because there was no wrapping div.

Assuming a use case (as I am) in which the author does not have the ability to view source, in order to style an embedded entity they need to:

1. Create some text
2. Style the text from the styles dropdown with div element style
3. select the text
4. click the embed link and embed the entity which replaces the text and is now wrapped in the div with the class

After this the embedded entity is still not styled in the WYSIWYG, but is styled on the live page.

If it's wrapped in a div: select it, style it, done. That seems like a far more usable feature.

#8:

I feel strongly about not copying the data attributes still. If someone wants that behavior, I think they should be able to do so using hook_entity_embed_alter(). I'd rather just replace the content like all other filters do.

+1000

#11:

The first thing I wanted to do was create a class and add it to the "styles" dropdown in CK so I could style embedded entities. This was not possible because there was no wrapping div.

You're misunderstanding this. We're talking about the markup that Entity Embed transforms: <drupal-entity data-entity-type="node" data-entity-uuid=… />. Currently, the content that that the filter should put in its place, is not replacing that <drupal-entity> tag. It's being added as the child of the <drupal-entity> tag.

What you're talking about is that any class attribute set on <drupal-entity> should persist, should make its way onto the final HTML.

i.e.:

<drupal-entity class="FANCY-KEVIN-ARTICLE" data-entity-type="node" data-entity-uuid=… />

->

<article role="article" class="FANCY-KEVIN-ARTICLE node node--type-page node--promoted node--view-mode-teaser">

That is true, and is a separate problem. Entity Embed should behave like \Drupal\filter\Plugin\Filter\FilterCaption does in that case: it correctly passes on classes.

I ran into this bug in yet another way: via the test coverage while working on #2628358: Disallow <div>, only allow <drupal-entity>:

    // Test that tag of container element is not replaced when it's not
    // 'drupal-entity'.
    $content = '<not-drupal-entity data-entity-type="node" data-entity-id="' . $this->node->id() . '" data-view-mode="teaser">this placeholder should not be rendered.</not-drupal-entity>';
    $settings = array();
    $settings['type'] = 'page';
    $settings['title'] = 'test entity embed with entity-id and view-mode';
    $settings['body'] = array(array('value' => $content, 'format' => 'custom_format'));
    $node = $this->drupalCreateNode($settings);
    $this->drupalget('node/' . $node->id());
    $this->asserttext($this->node->body->value, 'embedded node exists in page');
    $this->assertRaw('</not-drupal-entity>');

It says "test that anything else than is NOT replaced" … but then it tests that it DOES get replaced! Yet it also tests that </not-drupal-entity> still exists… And both tests pass! That is not logically possible!

Except this bug makes it possible: because we never ever replace the original DOM node, we also cannot reliably test some cases, because the original HTML is never actually replaced.

Working on this.

This blocks #2628358: Disallow <div>, only allow <drupal-entity>.

Better title.

Just using \Drupal\embed\DomHelperTrait::replaceNodeContent() instead of \Drupal\embed\DomHelperTrait::setNodeContent() is all that's necessary!

Test coverage updated.

Note that these bits of test changes are due to #12 and they will be fixed in #2628358: Disallow <div>, only allow <drupal-entity>:

@@ -102,6 +106,9 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
 
     // Test that tag of container element is not replaced when it's not
     // 'drupal-entity'.
+    // @todo This does NOT actually work, despite the test comment having
+    //   claimed otherwise for more than a year. The test is simply wrong. It
+    //   will be fixed in https://www.drupal.org/node/2628358
     $content = '<not-drupal-entity data-entity-type="node" data-entity-id="' . $this->node->id() . '" data-view-mode="teaser">this placeholder should not be rendered.</not-drupal-entity>';
     $settings = array();
     $settings['type'] = 'page';
@@ -109,8 +116,8 @@ class EntityEmbedFilterTest extends EntityEmbedTestBase {
     $settings['body'] = array(array('value' => $content, 'format' => 'custom_format'));
     $node = $this->drupalCreateNode($settings);
     $this->drupalget('node/' . $node->id());
-    $this->asserttext($this->node->body->value, 'embedded node exists in page');
-    $this->assertRaw('</not-drupal-entity>');
+    $this->assertText($this->node->body->value, 'embedded node exists in page');
+    $this->assertNoRaw('</not-drupal-entity>');

But… HAH! That won't actually work, because embed's replaceNodeContent() is broken: see #2602316: Allow any HTML as content for replaceNodeContent. It will only actually import the first DOM node. And the first DOM node when rendering an article template happens to be… a text DOM node with a newline in it. The second DOM node is the <article class="type-node" …> HTML that we expect.

So, with the above, tests actually fail for both patches because embed is broken…

Now working on #2602316: Allow any HTML as content for replaceNodeContent.

As predicted, both patches failed.

Forgot to add a related issue.

@Wim Leers

What you're talking about is that any class attribute set on should persist, should make its way onto the final HTML.

IIRC the class was not even added to drupal-entity when I attempted to do so via the styles dropdown, but maybe there was just no visual feedback?

Embed patch was committed. Re-testing.

Fail was expected. No idea why bot didn't recognize test only patch. Committing this as it blocks us on other issues.

If we still want to preserve classes let's do that in a separate issue. Keeping in rendered output isn't option IMO.

Thank you all!

I will open a separate issue for the class attribute persistence support.

@tkoleary: opened that separate issue as promised: #2628712: Persist the classes specified in <drupal-entity class> onto the rendered entity.