Problem/Motivation

I'm reviewing the entirety of embed + entity_embed, for several reasons: cacheability/security, because I helped get the CKEditor Widget system into Drupal core, of which this will likely be one of the most prominent users, etc.

I've got a bunch of patches ready against embed, most are small things, and cleanup. It was very doable to grok embed.

With entity_embed, that's not really the case. You'd think it's "just" a matter of a button in CKEditor that points to a Drupal dialog which allows you to select an entity and a view mode, plus a filter to render the entity in the selected view display on output. That's what I expected. I was very very wrong.

Let me state the problem with Entity Embed's data model by quoting an IRC conversation:

13:41:51 <WimLeers> I understand that File/Image entities require special care, because they don't have entity view displays/entity view builders. Thanks berdir for explaining that.
13:42:22 <WimLeers> But there are lots of other things that are pretty mysterious, i.e. difficult to understand why they work the way they work
13:42:34 <WimLeers> And I fear that getting a release out now will mean supporting those things forever.
13:42:49 <WimLeers> For example, install entity_embed freshly, embed a node, and… you get this:
13:42:57 <WimLeers> <drupal-entity data-align="none" data-embed-button="node" data-entity-embed-display="entity_reference:entity_reference_entity_view" data-entity-embed-settings="{&quot;view_mode&quot;:&quot;default&quot;}" data-entity-id="1" data-entity-label="Node" data-entity-type="node" data-entity-uuid="48b73228-7c8a-4d50-8500-f161a1f6c4c3"></drupal-entity>
13:43:40 <WimLeers> data-embed-button -> I understand this is to apply the same restrictions as the ones associated with the button you originally used. But this is metadata for *the authoring UI*, it doesn't belong in the structured content that we store
13:43:52 <WimLeers> data-entity-embed-display -> this could not possibly have been a longer value
13:44:06 <WimLeers> data-entity-embed-settings -> embedded, HTML-encoded JSON… wow
13:44:24 <WimLeers> data-entity-id -> despite there already being data-entity-uuid, and the docs explicitly recommending to use UUID, not ID…
13:44:43 <WimLeers> data-entity-label -> not clear to me what the point is of this metadata?
13:45:03 <WimLeers> It *used* to be much simpler, much more understandable, much more structured, and therefore much more reusable.
13:45:07 <WimLeers> It used to look like this:
13:45:40 <WimLeers> <drupal-entity data-align="none" data-entity-type="node" data-entity-uuid="48b73228-7c8a-4d50-8500-f161a1f6c4c3" data-entity-view-mode="default"></drupal-entity>
13:46:04 <WimLeers> Plus, it apparently supports <div>s just like it supports <drupal-entity>. Why?
13:46:08 <WimLeers> This is my biggest concern
13:46:16 <slashrsm> WimLeers: we pretty much agree about everything you wrote
13:46:19 <WimLeers> Because the format for those things *will* need to be supported in the long run
13:46:28 <slashrsm> I also discovered some stranginess in form part
13:46:33 <slashrsm> it is over complicating
13:46:55 <slashrsm> a lot of this things are leftovers from the initial development
13:47:08 <slashrsm> when we figured out stuff as we worked on it
13:47:20 <slashrsm> we should definitely take a breath and clean this things up
13:47:25 <slashrsm> some of them should be easy.
13:47:40 <slashrsm> like removing data-entity-id. We just do that. Done.
13:48:25 <slashrsm> some might be more complicated though.... data-entity-embed-settings for example. With the current level of flexibility (using formatters) I don't see any way how to simplify that.
13:48:31 <slashrsm> And I agree it looks awful
13:50:18 <slashrsm> WimLeers: My suggestion would be to open a "clean-up" meta or something like that. Have initial discussion there. Create concrete issues for each individual problem.
13:51:32 <WimLeers> slashrsm: It's not just about "looks" though. It's also about other modules being able to extend/build upon what entity embed does. And about the longevity of your stored data
13:51:52 <WimLeers> slashrsm: Okay. I will file such a clean-up issue.
13:51:56 <WimLeers> with patches/child issues
13:52:04 <WimLeers> slashrsm: I'm glad you're on the same page :)
13:52:15 <slashrsm> WimLeers: of course I am  :)
13:52:22 <slashrsm> WimLeers: thank you for bringing that up
13:52:28 <WimLeers> slashrsm: It *used* to be simpler. Do you know what exactly triggered this change? Perhaps an issue link?
13:52:29 <slashrsm> WimLeers++
13:52:37 <WimLeers> Oh but this is just the start :P
13:53:01 <slashrsm> WimLeers: This specific settings JSON happened when we switched to field formatters for rendering
13:53:19 <WimLeers> slashrsm: and why was that necessary?
13:54:19 <slashrsm> WimLeers: flexibility when it comes to display. Requiring configuration of a view mode if a lot of overhead for some things. When you just want to embed a thumbnail of an image or similar.

Proposed resolution

  1. Disallow <div …>, only allow <drupal-entity>. One way, not two.
  2. Remove data-entity-label. We already have data-entity-type + data-entity-uuid. From that, it's possible to derive the label. Right now, the entity's label is persisted but never updated, i.e. label changes are not reflected here. So, it's a source of misinformation.
  3. Disallow data-entity-id when data-entity-uuid is present.
  4. Better yet: remove data-entity-id altogether.
  5. data-entity-embed-display: not sure yet.
  6. data-entity-embed-settings: not sure yet.

Remaining tasks

  1. Disallow <div …>, only allow <drupal-entity>. One way, not two. Done: #2628358: Disallow <div>, only allow <drupal-entity>
  2. Remove data-entity-label. We already have data-entity-type + data-entity-uuid. From that, it's possible to derive the label. Right now, the entity's label is persisted but never updated, i.e. label changes are not reflected here. So, it's a source of misinformation. Done: #2706045: Remove data-entity-label
  3. Disallow data-entity-id when data-entity-uuid is present. In progress at #2706051: Require UUIDs for embedding with the dialog (remove data-entity-id from embeds).
  4. Better yet: remove data-entity-id altogether.
  5. data-entity-embed-display: not sure yet.
  6. data-entity-embed-settings: not sure yet.

User interface changes

TBD

API changes

TBD

Data model changes

TBD

Comments

Wim Leers created an issue. See original summary.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Title: Entity Embed's data model » Clean up Entity Embed's data model
Issue summary: View changes

My proposal currently is:

  1. Disallow <div …>, only allow <drupal-entity>. One way, not two.
  2. Remove data-entity-label. We already have data-entity-type + data-entity-uuid. From that, it's possible to derive the label. Right now, the entity's label is persisted but never updated, i.e. label changes are not reflected here. So, it's a source of misinformation.
  3. Disallow data-entity-id when data-entity-uuid is present.
  4. Better yet: remove data-entity-id altogether.
  5. data-entity-embed-display: not sure yet.
  6. data-entity-embed-settings: not sure yet.

Also note that data-entity-embed-settings contains the settings specific to the selected display (data-entity-embed-display), therefore this is actually currently misnamed: data-entity-embed-display-settings would make much more sense.

I'd like to go back to data-entity-view-mode, and get rid of Entity Embed Display Plugins. But that's something for a separate issue (issue coming soon). So, for now, let's focus on everything else: points 1 through 4 in this ordered list.

slashrsm’s picture

slashrsm commented
Issue tags: +Media Initiative, +D8Media, +Zurich media sprint 2015

I absolutely support 1. - 4. We can get that done and continue discussing display configuration meanwhile.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#2.1 has been completed: #2628358: Disallow <div>, only allow <drupal-entity>.

Would be great if others could push the other points forward. I likely won't be able to work on this again until beginning of January.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Parent issue: » #2577891: Entity Embed 8.x-1.0.0-rc1 release
adarkling’s picture

adarkling commented

I agree with 1-3.

data-entity-id requires support though, because its been used in previous versions of entity_embed. But I agree that it should not be placed in any new embeds.

slashrsm’s picture

slashrsm commented

data-entity-id requires support though, because its been used in previous versions of entity_embed. But I agree that it should not be placed in any new embeds.

This is easily achievable by leaving support for data-entity-id in the text filter. But is can be completely removed from the CKEditor related code.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

#2 is in progress with #2706045: Remove data-entity-label

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Getting rid of data-entity-id is proving more difficult than expected. The entity_autocomplete element returns an ID, not a UUID, because entity reference fields in core do not reference UUIDs, they reference IDs still. I'm having to fight a lot of code to make UUID just work. We'd also need to make sure that Entity Browser is wired up everywhere to work on a UUID-only basis.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#8: yay! Already committed now even :)

#9: I think going all-in on UUIDs is going to be the wise long-term to do, because it avoids all problems with deploying content that was created on staging as well. That's also why \Drupal\editor\Plugin\Filter\EditorFileReference uses only that.

That made me realize something else: using data-entity-id means no usages are recorded, which means files referenced by ID instead of UUID can actually be deleted by accident.

What exactly is the difficulty? Don't you have access to the entity object in your submit handler? Then you could just call $entity->uuid() to get its UUID. What am I missing?

Finally, I figured you couldn't possibly be the first person to deal with entity UUID + autocomplete, right? Best search results: https://www.drupal.org/project/uuid_entity_autocomplete + #2341357-6: Views entity area config is not deployable and missing dependencies + #2341357-29: Views entity area config is not deployable and missing dependencies (What I'm saying is that we need to enter only IDs in the UI and automatically convert and store them as UUID in config.). No perfect matches, but perhaps something useful?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

Yeah, once I got over my mental block at having the form element still being data-entity-id, it ended up being ok. Task #4 is ready for review at #2706051: Require UUIDs for embedding with the dialog (remove data-entity-id from embeds), but leaves support for data-entity-id in the filter.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Awesome! Posted review.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Priority: Critical » Major

Downgrading based on the progress made so far.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue summary: View changes

Updated IS per #13 and to reflect recent progress. This makes it easier to see what this issue has gotten done and what's still left.

So, what remains:

  1. Disallow data-entity-id when data-entity-uuid is present. In progress at #2706051: Require UUIDs for embedding with the dialog (remove data-entity-id from embeds).
  2. Better yet: remove data-entity-id altogether.
  3. data-entity-embed-display: not sure yet.
  4. data-entity-embed-settings: not sure yet.

#3 is in progress. Dave decided against #4 for BC. I wonder if it would not be better to just provide an upgrade path to update all existing content? There's not even an alpha release of this module yet, so that'd be totally fair? (Actually not having an upgrade path would theoretically even be ok.)

#5 and #6: what are your thoughts on that, Dave?

slashrsm’s picture

slashrsm commented
Issue tags: +beta blocker

We definitely need to decide what we want to do about this before beta.

slashrsm’s picture

slashrsm commented

#14.3 was committed and it seems that we decided not to do #14.4.

#14.5 and #14.6 are remaining. Any thought on those? We won't drop this concept and switch back to view modes. Maybe we can fix inconsistent naming as suggested in #2?

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

If we can't drop this concept in favor of view modes, then

  1. I'd urge to really think about the consequences of embedding JSON.
  2. I think the renaming for consistency that I suggested in #2 would make sense.
slashrsm’s picture

slashrsm commented
  • Assuming we stick with support for field formatters I don't see any nicer way of storing their arbitrary configuration. No matter wether is is JSON, serialized arrays or anything else it is ugly and sub-optimal.
  • Created #2760801: Rename data-entity-embed-settings attribute
slashrsm’s picture

slashrsm commented
Status: Active » Fixed

#2760801: Rename data-entity-embed-settings attribute landed. Nothing else left to be done here.

Status: Fixed » Closed (fixed)

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