Problem/Motivation

For the embed button to work, the requiredContent attribute has to match the allowed html attributes configuration. Right now, requiredContent uses a * wildcard so it requires any attributes. The documentation for allowed HTML tags says you need to explicitly specify the list of data-* attributes. If you do that, then the button doesn't work.

If you change that to then the button works, you can select and place something... but it won't show up. Because our HTML filtering doesn't allow the wildcard like that.

Proposed resolution

Explicitly specify all required attributes in requiredContent instead of *.

Remaining tasks

Would be nice if it would configure that automatically, but I couldn't figure out what is needed for that. There's also #2554687: <drupal-entity> tag not being automatically added when an Entity Embed button is added to active editor toolbar, I suppose that will get easier once it actually works again, so lets do that there.

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#8 interdiff_2_8.txt880 bytesslashrsm
#8 2638788_8.patch1.43 KBslashrsm
#3 interdiff.txt2.46 KBslashrsm
#3 2638788_3.patch2.46 KBslashrsm
#2 entity_embed-required-content-2638788-2.patch778 bytesBerdir
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Berdir created an issue. See original summary.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Status: Active » Needs review
FileSize
entity_embed-required-content-2638788-2.patch778 bytes
slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented
Issue tags: +D8Media, +Media Initiative
FileSize
2638788_3.patch2.46 KB
interdiff.txt2.46 KB

What about something like this? This should allow attributes like data-entity-id to support legacy data, but it doesn't require it (at least if I understand correctly how this rules work).

I also tested adding to the list of allowed tags and it works. It is simpler than listing all attributes, but I am not sure if there are any security considerations that we should account for.

Reinmar’s picture

Reinmar CreditAttribution: Reinmar commented

The patch looks OK. The "required content" must define the minimum HTML elements and attributes which this feature requires to work at all. So in this case I would ask if The entity embed feature requires data-entity-embed-display and data-entity-embed-settings. If any of them is optional, then it does not need to be specified in the "required content" property.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at Examiner.com commented 
+++ b/js/plugins/drupalentity/plugin.js
@@ -27,8 +27,8 @@
+        allowedContent: 'drupal-entity[data-entity-*]',

I guess this means adding id, classes and other data- attributes won't be allowed based on this. Do we want that?

Reinmar’s picture

Reinmar CreditAttribution: Reinmar commented

Yes, the current setting wouldn't allow that if CKEDITOR was used outside Drupal. But the funny thing is that Drupal reads the allowedContent settings from features in order to create its own filter, but then defines these filters in a more generic way when initialising editors - in this case it would be `drupal-entity[*]`. So most likely, as long as the admin hasn't modified filter's settings in Drupal's panel, everything would work automatically, because if config.allowedContent was defined during editor initialisation it will override what features set.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

I'm not a fan of #3. I prefer explicitness. I prefer whitelisting over just allowing everything.

Therefore I prefer #2.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
FileSize
2638788_8.patch1.43 KB
interdiff_2_8.txt880 bytes

#2 it is then. Should we add same stuff to widget part?

kmoll’s picture

kmoll CreditAttribution: kmoll at Appnovation for Pfizer, Inc. commented

@slashrsm I updated the code, the above patches were missing the 'data-entity-id' was causing the entity not to render. I tested the ticket and it seems to work. I created a PR, but let me know if you rather I post the patch, or just want to update yours.

https://github.com/drupal-media/entity_embed/pull/204

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented
Priority: Normal » Major
Status: Needs review » Reviewed & tested by the community

Lets get this fixed, it's clearly a major if not critical bug, preventing this module from being used properly.

slashrsm’s picture

slashrsm CreditAttribution: slashrsm at MD Systems GmbH commented
Status: Reviewed & tested by the community » Fixed

Agreed. Merged with reworded commit message to include everyone that helped.

EDIT: Yay! Good karma :)

  • slashrsm committed 9940466 on 8.x-1.x authored by kmoll 
    Issue #2638788 by slashrsm, Berdir, Reinmar, Wim Leers, kmoll: Required...
Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

+1, at best this is the final solution, at worst this is a great step forward.

EDIT: yay, cross-posted with the commit! :)

  • Devin Carlson committed 6eea3e1 on 7.x-3.x 
    Issue #2638788 by slashrsm, Berdir, Reinmar, Wim Leers, kmoll: Required...

  • Devin Carlson committed d576737 on 7.x-2.x 
    Issue #2638788 by slashrsm, Berdir, Reinmar, Wim Leers, kmoll: Required...

  • Devin Carlson committed f883bb3 on 7.x-1.x 
    Issue #2638788 by slashrsm, Berdir, Reinmar, Wim Leers, kmoll: Required...

Status: Fixed » Closed (fixed)

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