Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
When you create an embed button and set the display plugins to be used to some view modes, there should be a dependency on the config that declares that view mode. That is not the case.
This is noticeable when using the embed button config in an install profile: importing it fails.
Example of faulty file:
langcode: en
status: true
dependencies:
module:
- entity_embed
- media_entity
label: Media
id: media
type_id: entity
type_settings:
entity_type: media
bundles: { }
display_plugins:
- 'view_mode:media.embedded'
- 'view_mode:media.overlay'
entity_browser: ckeditor_media_browser
entity_browser_settings:
display_review: false
icon_uuid: null
The config part should contain this as well:
config:
- core.entity_view_mode.media.embedded
- core.entity_view_mode.media.overlay
Comment | File | Size | Author |
---|---|---|---|
#32 | interdiff.txt | 1.45 KB | Wim Leers |
#28 | entity-embed-view-mode-configs-2841964-28.patch | 2.86 KB | oknate |
| |||
#8 | entity-embed-view-mode-configs-2841964-7.patch | 886 bytes | oknate |
Comments
Comment #2
daften CreditAttribution: daften at District09 commentedComment #3
daften CreditAttribution: daften at District09 commentedSeems to be in src/Plugin/EmbedType/Entity.php around line 290. It calculates the dependencies of the Entity Embed displays, but doesn't depend on those view modes itself.
Comment #4
daften CreditAttribution: daften at District09 commentedComment #5
marcoscanoI can confirm this issue still exists with latest -dev.
Couldn't dive too deep but the @todo in FieldFormatterEntityEmbedDisplayBase.php may have something to do:
Comment #6
mstef CreditAttribution: mstef commentedStill exists; FYI
Comment #7
Wim LeersComment #8
oknateHere's a patch that adds the missing configs.
Comment #9
Wim LeersThanks! 👌 Will review in the morning. For now I'll do manual testing.
But ideally this would also have actual test coverage. If you'd like to work on that too, then even better! 🙏
Comment #10
oknateAdding a test for this.
Comment #11
oknateHmm, I had tested locally and I didn't get all these failures. Investigating.
Comment #12
oknateUpdating test to use another approach. It worked this time. :)
Comment #13
oknateComment #14
phenaproximaA great start, but I think we need to do a little more here :)
There's a bit of extra white space above the doc comment and below the function signature, can we fix that?
How do we know these view modes actually exist? I think we should do a loadByProperties() here so that we declare dependencies on config that *actually* exists, rather than stuff that may or may not exist. Let's also make sure the test covers this.
There's an extra line here.
Can we not have a blank line after each line in this test? It's a bit easier to read "stanzas" of code, rather than many isolated lines with a lot of white space :)
This can be
$this->assertContains('core.entity_view_mode.node.teaser', $dependencies['config'])
.And likewise, this can be assertNotContains().
Additionally, it would be helpful to post a fail patch -- that is, a patch which does not fix the bug, but has a test that triggers the bug, thus causing the test to fail. That way, we can be 100% sure that the bug is fixed before this issue is committed. :)
Also removing the defunct D8Media tag.
Comment #15
Wim LeersClosed #2906894: Config import does not work for custom view modes if bundles not selected as a duplicate; it is a consequence of this bug.
Comment #16
Wim LeersComment #17
Wim LeersComment #18
oknateUpdating patch based on feedback.
Comment #19
oknateupdating last patch, I saw it was using entityManager, not entityTypeManager
Comment #20
oknateComment #21
phenaproximaLooking even better! Thanks, @oknate. And thanks for the added test coverage!
Total nitpick, but can we rename this to $view_mode_storage, since it's a storage handler?
I think that $storage->loadMultiple() might be a preferable approach here.
Nit: There's a superfluous blank line here.
We also need to test that deleting the actual view mode entity removes it from the dependencies as well.
Comment #22
oknateUpdating patch based on feedback.
Comment #23
phenaproximaLooks like a reroll is needed here :(
Comment #24
oknateAdded test for deleting view mode but it's currently failing.
Comment #25
oknateTesting update to functional test.
Comment #26
oknateTesting update to functional test.
Comment #27
oknateTesting update to functional test.
Comment #28
oknateCoding standard fixes, minor changes.
Comment #29
oknateComment #30
Wim LeersGetting very late here, assigning to @phenaproxima for review :)
Comment #32
Wim LeersReviewed in detail. The code looks good, and more importantly: the test coverage does too! So I looked at this in detail, and found a few minor things:
Language nits here. Fixed.
This stood out to me as bizarre upon further inspection: why would deleting the teaser view mode remove the dependency? Deleting the teaser view mode should not even be possible if there is a dependency on it.
This is only actually possible because in the previous test, you're already removing the dependency, and THEN you're removing the view mode, then artificially restoring it.
This test coverage can simply be deleted :)
Comment #33
oknateThe reason I did it in that order is that if you delete the view mode with a dependency on it, it would delete the embed button.
I figured there could be a case where you have two developers working together and one deletes a view mode, and the second has an embed button that they commit that has a hanging dependency on a deleted view mode.
But I agree that we don't really need to test for it, since in a previous assertion, we've already proven that a display plugin that references a non-existing view mode won't add a dependency or throw a fatal error.
Comment #34
Wim LeersIf they're running tests like they should, they'd notice it being broken. Work done in parallel must still be tested in sequence, precisely for this reason.
I love that you're thinking about all the possible edge cases though :)