Entity embeds cache separately from the host entity's field that holds the CKEditor. This is unnecessary, and it makes it trickier to have per embed settings such as alt and title, which are rendered within the DOM of the embedded entity.
This comes from feedback from Wim Leers here: #2924391-66: [media entities] Regardless of @EntityEmbedDisplay plugin, Media entities representing a `image/*` MIME type should be able to have a per-embed `alt` and `title` (see comment 2)
Do not cache the embed separately.
None
TBD
Not relevant
| Comment | File | Size | Author |
|---|---|---|---|
| #14 | 3058872-14.patch | 6.38 KB | wim leers |
| #14 | interdiff.txt | 4.02 KB | wim leers |
| #12 | entity-embed-no-cache-3058872-12.patch | 6.35 KB | oknate |
| #12 | 3058872--interdiff-10-12.txt | 1.89 KB | oknate |
| #10 | entity-embed-no-cache-3058872-10.patch | 7.22 KB | oknate |
Comments
Comment #2
oknateComment #3
oknateComment #4
wim leersPatch extracted from #3058872: Entity Embed render array shouldn't cache separately from host field.
Comment #5
wim leersComment #6
oknateFor the test coverage, how do you think we should check the caching? I was thinking we could do a query against the cache_render table to look for the cids. We could assert that only the host entity (or field) was cached there.
Comment #7
wim leersThat could work: we could check there's only one cache item for multiple requests. However, that is diving into details.
I think this can be tested much more simply, without making the tests coupled to implementation details:
altis inherited.alt.altis the overridden one.With HEAD, this should fail, with this patch, it should pass.
Comment #8
wim leersSee #2924391-114: [media entities] Regardless of @EntityEmbedDisplay plugin, Media entities representing a `image/*` MIME type should be able to have a per-embed `alt` and `title`. We can directly copy/paste that test coverage over once #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` lands. But actually, we shouldn't keep that as functional JS test coverage, we should turn it into a functional PHP test: rather than inserting that test HTML in CKEditor, then saving the form, we can just create an entity like that in PHP,
GETits canonical page and make our HTML assertions just like #114's interdiff was doing.Comment #9
wim leersI just committed #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`. Let's do this now!
Comment #10
oknateComment #11
oknateI see I have some extraneous code for filter format.
Comment #12
oknateRemoving filter format. That was unused code that I had brought over from my work on #3052482: Expand EntityEmbedFilter's test coverage: test many more edge cases, change to kernel test
Comment #14
wim leersRight; this is technically not a bug in the filter, but it is exposed via the filter. I guess this could also have been part of #3052482: Expand EntityEmbedFilter's test coverage: test many more edge cases, change to kernel test.
But we're trying to make #3052482 solely about adding test coverage, and extracting any discovered bugs into an issue of its own will make it much easier to understand commit history too.
#12 looks great, but I found a few nits and some dead code, which I fixed.
P.S.: Note that this particular test could easily be converted to a kernel test, but I'll leave that to #3052482.
Comment #15
wim leers🥳
Comment #18
oknateComment #19
oknate