Entity Embed 8.x-1.0.0-rc1 release

Given #2593315: Improve documentation of hook_entity_embed_alter() and #2593379: Analyze/improve/fix handling of bubbleable metadata in entity_embed, I think we need to first seriously scrutinize the APIs of Entity Embed, to prevent API changes after RC?

Any update on a possible RC release? Is there any issue I can assist with that may be preventing an RC release?

This of course blocks #2577887: Entity Embed 8.x-1.0.0 release.

I've been triaging the entity_embed issue queue to help this module get to a stable release. Acquia is sponsoring me to do this to help get Media in Drupal core to stable, which requires solving #2801307: [META] Support WYSIWYG embedding of media entities, which in turn requires moving a subset of entity_embed's functionality into core.

I've told the main module maintainer about this, and he was supportive: https://twitter.com/davereid/status/1061052785297448963 🎉

So … now that I've triaged the majority of the issue queue and patterns have started to emerge, I'm going to start updating this issue to get clarity. Expect lots of updates here in the coming weeks.

There are currently 69 open entity_embed 8.x-1.x issues.

• Of those, 6 are fixed. That leaves 63 effective open issues.
• Of those, 15 are RTBC. That leaves 48 effective open issues.
• Of those, 16 are postponed and 3 need more info. That leaves 29 issues that need to be worked on.
• Of those, 2 are "plan" issues, leaving only 27 actual issues. These 27 are all Active, Needs work or Needs review
• Of those 27 actual issues, I made a selection in this issue.

I selected 24 issues (see issue summary), 7 of which are postponed, and 7 are already RTBC.

These are the ones that I believe that are either crucial for the short-term stabilization of Entity Embed or are crucial for #2801307: [META] Support WYSIWYG embedding of media entities to happen in core, which is going to contain a crucial subset of the Entity Embed module.

That means we have exactly ten issues that need attention now and would materially help Entity Embed get closer to stability.

Note that I am not an official maintainer. I am merely asked to help the Media Initiative with #2801307: [META] Support WYSIWYG embedding of media entities, which requires moving a subset of Entity Embed into core. I felt triaging the Entity Embed issue queue is essential, because it maximally informs us about hitherto unknown unknowns: we now have a very good idea about which areas in Entity Embed need to be stabilized, which bugs are getting in the way: we know what already works well and what does not yet.

I pinged @Dave Reid about this a few weeks ago, and he responded very positively: https://twitter.com/davereid/status/1061052785297448963 — I hope you still feel the same way, Dave 😊

A lot of things got committed! :) Refreshing linked issue statii.

Removing issue marked as duplicate.

Removing another duplicate: #2628712: Persist the classes specified in <drupal-entity class> onto the rendered entity is now considered a duplicate of #2917132: EntityEmbedBuilder removes data- attributes set by other text filter plugins!

Yay, everything in the Text filter plugin bucket is now DONE, as well as several other issues! @phenaproxima and I already committed a bunch of patches last Friday evening (because @slashrsm granted us commit access). Yesterday and today @marcoscano, @phenaproxima and I have been collaborating on a bunch of patches at the Media sprint in Barcelona: https://twitter.com/_marcoscano_/status/1069932097119444992

Now everything in Configuration (missing dependencies) is also done!

Added

6. #3011228: Missing `data-quickedit-entity-id` attribute prevents in-place editing of embedded entities
7. #2614364: Quick Edit of host entity "leaks" into embedded entities: disable Quick Edit

to Text filter plugin, since that is likely to be a common expectation.

Most of Test coverage is now done (all the issues directly affecting Text filte rplugin, and the issues added in #21 are also done. 🎉

I have added an additional test coverage class for #2466013: Write test for CKEditor integration. Please review.

@oknate Thanks! Will do :)

Would it be useful to release a new beta? There hasn't been a new release in 2 1/2 years.

#2614350, #3011228 and #2560787 were closed and marked "works as designed".

#2990624 was marked as "cannot reproduce".

Sorry. I feel very bad for the silence here. I asked to not yet ship RC a few months ago because I was uncertain about one of the changes we committed in December, and would rather not see them get shipped in a release.

With JSON:API shipped as part of Drupal 8.7, getting Media embedding added to Drupal core is now my #1 priority. And since it's going to be based on Entity Embed's code, I can promise significant progress on this in the coming days and weeks!

I think this module needs some additional maintainers. Wim Leers is the only active maintainer and very busy with JSON:API.

Thank you, Wim! I see some activity this morning!

@Wim: Thanks again.

Indeed :) I’m off today and Monday is a holiday here in Belgium, but expect much more progress next Tuesday!

I already committed one regression fix that would otherwise have appeared in the RC. I think there is one more.

#3027445: Add tests to ensure `data-langcode` works when that translation does not exist; should fall back to default translation is another fix we need for the text filter plugin.

Removing:

5. #2614364: Quick Edit of host entity "leaks" into embedded entities: disable Quick Edit

per #2614364-8: Quick Edit of host entity "leaks" into embedded entities: disable Quick Edit since it does not affect media embeds, which is the majority use case for the Entity Embed module.

#2752253: Document correct filter_html_image_secure + filter_align + filter_caption + entity_embed filters order, write tests, and ensure correct by default landed!

Thanks to @oknate, #2466013: Write test for CKEditor integration is very close!

And I'm now digging in to #2544020: CKEditor plugin not written with CKEditor >=4.4 in mind; rewrite it based on core's `drupalimage` plugin. Expect more progress this week.

#2466013: Write test for CKEditor integration landed, this provides a huge confidence boost in this module. Thanks so much for taking the lead on that, @oknate! 👏

Added #3052482: Expand EntityEmbedFilter's test coverage: test many more edge cases, change to kernel test.

To not keep the >36K entity_embed-using sites waiting for the fixes that have landed in the past few years since https://www.drupal.org/project/entity_embed/releases/8.x-1.0-beta2, I just tagged https://www.drupal.org/project/entity_embed/releases/8.x-1.0-beta3 per this discussion:

wimleers (he/him) [5:23 PM]
@marcoscano @seanb @davereid Hiya! @phenaproxima (he/him) is on vacation. I’ve been working on https://www.drupal.org/project/entity_embed for much of the past week. But I’ll be going on vacation on Monday, I’ll be back mid-May.

Not everything in https://www.drupal.org/project/entity_embed/issues/2577891 is done yet. But much is. My current focus is the CKEditor plugin; I’m currently working on making it match core’s `drupalimage` plugin, add undo support, add captioning support, etc. I won’t be able to finish that before going on vacation.

On the one hand I’d like to finish that first. But on the other hand, delaying the tagging of RC1 is not great: we’ve already made people wait so long (we first said we’d tag it in December). The good news is that thanks to @oknate’s test coverage contributions, we _know_ for a fact that the current CKEditor integration despite its basicness _is_ working reliably.

So I think it might be wise to tag RC1, and then I’ll continue working on it when I get back.

Thoughts?

marcoscano [5:25 PM]
@wimleers (he/him) the last beta is from several years ago. What about we tag a new beta? That would allow people to take benefit of the several fixes from the last weeks/months, but we can still keep the plan for rc1 (edited) 

wimleers (he/him) [5:26 PM]
That works for me too.

oknate [5:26 PM]
I don’t really see the difference between a new beta and a rc
can someone explain the difference?

wimleers (he/him) [5:26 PM]
RC = API freeze

oknate [5:26 PM]
yeah, that’s what I was thinking
I don’t think we should do that yet
there’s still some improvements that could be made
there’s more test coverage that could be added
I think a new beta is good

No influx of new bug reports against beta3, despite it being the first release in 2.5 years and already ~8500 sites having updated to it (out of a total of ~44K, or about 20% — see https://www.drupal.org/project/usage/entity_embed).

More important: most remaining issues have progressed significantly: the CKEditor integration: dialog (PHP) ones (#2998005: [PP-1] Support Drupal core's Media Library + #2924391: [media entities] Regardless of @EntityEmbedDisplay plugin, Media entities representing a `image/*` MIME type should be able to have a per-embed `alt` and `title`), but also the Test coverage one (#3052482: Expand EntityEmbedFilter's test coverage: test many more edge cases, change to kernel test). That leaves "just" the CKEditor integration: widget (JS) ones, which I've started on yesterday. And 12 additional issues have already been committed. Thanks to @jurgenhaas, @dpi and especially @oknate.

As of a few days ago, https://www.drupal.org/project/issues/entity_embed?version=8.x fits on a single page (<= 50 issues). And now the remaining bug reports are mostly those for the D7 branch; the D8 branch has less than half of the remaining bug reports! (And most of those are for the https://www.drupal.org/project/entity_browser integration

Hopefully we'll be able to continue to make good progress, which would mean RC1 next week. Unless @phenaproxima, @marcoscano or @oknate think doing a beta4 first is more appropriate. Personally, I think it's about time we do an RC.

I think an rc1 is fine. I don't think we'll be changing any APIs.

I don't feel strongly about it either way, to be honest. At this point, we shouldn't be changing APIs anyway, regardless of stability; Entity Embed has been in wide use for years, and changing its API (even if we technically can) would be detrimental.

For me, the sticking point would be tests -- do we have enough coverage for an RC? Maybe not, but even if we don't, I still don't think it matters that much. Our tests are not part of our API, and we can do whatever we want with them, whenever we want. So hey...why not go for RC? :)

+1 for RC1!

Huge thanks to @oknate, @Wim Leers and all others that are knocking things out here!

I have a few issues that we might want to discuss before creating an rc1. There is a bit of a disorganized custom context handling around which plugins are shown on the button configuration form and within the EntityEmbedDialog. In fact there is a @todo item to update the plugin manager to use ContextAwarePluginManagerTrait. The plugins also have some context functionality that should be use the core trait and methods (if not overridden). Also, there are usability issues that could benefit from the EntityEmbedDisplay plugins being configurable.

Here are some issues I think we should consider exploring before releasing an RC 1. If we don't think it would change any APIs, then they could wait. But the idea of allowing the plugins to be configurable when creating an embed button would require schema changes and perhaps an upgrade path.

• #3058972: EntityEmbedDisplayManager should use ContextAwarePluginManagerTrait
• #3058425: Add ContextAwarePluginInterface to EntityEmbedDisplayInterface
• #3058846: EntityEmbedDisplay plugins should be configurable on the embed button form
• #3058872: Entity Embed render array shouldn't cache separately from host field

On the other hand, because development slowed on this module, and it's used by so many sites. Perhaps it's time to move towards a stable release, and breaking changes could be added to a new release branch.

On the other hand, because development slowed on this module, and it's used by so many sites. Perhaps it's time to move towards a stable release, and breaking changes could be added to a new release branch.

That's my reasoning too... d.o usage reports ~40K sites on the D8 version of this module (which would indicate an even higher real usage out there...). At this point, regardless of the 1.x branch status, any breaking changes should go into a 2.x branch, IMO.

At this point, regardless of the 1.x branch status, any breaking changes should go into a 2.x branch, IMO.

Agreed.

I think #3058972: EntityEmbedDisplayManager should use ContextAwarePluginManagerTrait and #3058425: Add ContextAwarePluginInterface to EntityEmbedDisplayInterface in #44 therefore would have to happen in a 2.x branch. As far as I'm concerned, we can start the 2.x branch as soon as 1.0 is released; if this is going to be the only breaking change, then that's still fine.

#3058846: EntityEmbedDisplay plugins should be configurable on the embed button form can be done without breaking BC AFAICT.

#3058872: Entity Embed render array shouldn't cache separately from host field wouldn't break anything.

#2924391: [media entities] Regardless of @EntityEmbedDisplay plugin, Media entities representing a `image/*` MIME type should be able to have a per-embed `alt` and `title` just landed, which leaves only three areas:

1. #2998005: [PP-1] Support Drupal core's Media Library — I think this actually should happen post-1.0. This is a new non-essential feature. It's not yet clear how we'll make the existing alignment/caption per-embed choices from EntityEmbedDialog available in this scenario, since it's not designed to be part of multi-step forms. (Not to mention alt/title per-media-image-embed, which just became available in #2924391.) It also will need a way for sites to opt in to this alternative selection UI, because otherwise it'll disrupt existing workflows (different UX, and no more per-embed overrides yet).
2. #3052482: Expand EntityEmbedFilter's test coverage: test many more edge cases, change to kernel test — I'd love to see this land before RC1, but I don't think it should block it. It's "just" a matter of finishing the work there. Things are already working mostly fine, as evidenced by the ~40K D8 sites out there using it. Any bugs that are found would need to be fixed either way. That's no reason to further delay getting tens of thousands of sites on a security-supported stable release of Entity Embed.
3. The CKEditor widget issue cluster: #2544020: CKEditor plugin not written with CKEditor >=4.4 in mind; rewrite it based on core's `drupalimage` plugin + #2813813: CKEditor's "undo" functionality not working + #2511404: Image entities/fields embedded using Entity Embed cannot be linked in CKEditor + #2282957: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog). #2813813 landed last week. The other three are very much in progress and were reviewed by the CKEditor team yesterday.

Given the above, I intend to tag RC1 this week! (Either tomorrow or on Sunday.)

#2998005: [PP-1] Support Drupal core's Media Library — I think this actually should happen post-1.0. This is a new non-essential feature.

Agreed.

#3052482: Expand EntityEmbedFilter's test coverage: test many more edge cases, change to kernel test: test many more edge cases, hopefully change to kernel test — I'd love to see this land before RC1, but I don't think it should block it.

Agreed. Tests should only block releases when they are related to a critical bug.

With regard to the CKEditor issue cluster...I'm not really qualified to weigh in on whether those should block RC1.

#2544020: CKEditor plugin not written with CKEditor >=4.4 in mind; rewrite it based on core's `drupalimage` plugin, #2282957: Caption should work like the drupalimage plugin (editable in WYSIWYG, not in dialog) and #2511404: Image entities/fields embedded using Entity Embed cannot be linked in CKEditor just landed. Time for RC1!

https://www.drupal.org/project/entity_embed/releases/8.x-1.0-rc1

Thanks, Wim!