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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

daften created an issue. See original summary.

daften’s picture

Project: Embed » Entity Embed
Version: 8.x-1.0-rc3 » 7.x-3.x-dev
daften’s picture

Version: 7.x-3.x-dev » 8.x-1.0-beta2

Seems 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.

marcoscano’s picture

Version: 8.x-1.0-beta2 » 8.x-1.x-dev
Issue tags: +D8Media

I 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:

  /**
   * {@inheritdoc}
   */
  public function calculateDependencies() {
    $this->addDependencies(parent::calculateDependencies());

    $definition = $this->formatterPluginManager->getDefinition($this->getFieldFormatterId());
    $this->addDependency('module', $definition['provider']);
    // @todo Investigate why this does not work currently.
    // $this->calculatePluginDependencies($this->getFieldFormatter());
    return $this->dependencies;
  }
mstef’s picture

Still exists; FYI

Wim Leers’s picture

oknate’s picture

Here's a patch that adds the missing configs.

Wim Leers’s picture

Issue tags: +Needs manual testing

Thanks! 👌 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! 🙏

oknate’s picture

oknate’s picture

Hmm, I had tested locally and I didn't get all these failures. Investigating.

oknate’s picture

Updating test to use another approach. It worked this time. :)

oknate’s picture

Status: Active » Needs review
phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: -D8Media

A great start, but I think we need to do a little more here :)

  1. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/ViewModeFieldFormatter.php
    @@ -58,5 +58,21 @@ class ViewModeFieldFormatter extends EntityReferenceFieldFormatter {
    +  ¶
    +  /**
    +   * {@inheritdoc}
    +   */
    +  public function calculateDependencies() {
    +
    +    $definition = $this->getPluginDefinition();
    

    There's a bit of extra white space above the doc comment and below the function signature, can we fix that?

  2. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/ViewModeFieldFormatter.php
    @@ -58,5 +58,21 @@ class ViewModeFieldFormatter extends EntityReferenceFieldFormatter {
    +    $view_mode = $definition['view_mode'];
    +
    +    foreach ($definition['entity_types'] as $type) {
    +      $this->addDependency('config', "core.entity_view_mode.$type.$view_mode");
    +    }
    

    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.

  3. +++ b/src/Tests/ViewModeFieldFormatterTest.php
    @@ -53,4 +53,38 @@ class ViewModeFieldFormatterTest extends EntityEmbedTestBase {
    +  public function testViewModeDependencies() {
    +
    +    $button = $this->container->get('entity_type.manager')->getStorage('embed_button')
    

    There's an extra line here.

  4. +++ b/src/Tests/ViewModeFieldFormatterTest.php
    @@ -53,4 +53,38 @@ class ViewModeFieldFormatterTest extends EntityEmbedTestBase {
    +    $config = $button->get('type_settings');
    +
    +    $config['display_plugins'] = ['view_mode:node.teaser'];
    +
    +    $button->set('type_settings', $config);
    +
    +    $button->save();
    +
    

    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 :)

  5. +++ b/src/Tests/ViewModeFieldFormatterTest.php
    @@ -53,4 +53,38 @@ class ViewModeFieldFormatterTest extends EntityEmbedTestBase {
    +    $this->assertTrue(in_array('core.entity_view_mode.node.teaser', $dependencies['config']));
    

    This can be $this->assertContains('core.entity_view_mode.node.teaser', $dependencies['config']).

  6. +++ b/src/Tests/ViewModeFieldFormatterTest.php
    @@ -53,4 +53,38 @@ class ViewModeFieldFormatterTest extends EntityEmbedTestBase {
    +    $this->assertFalse(in_array('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.

Wim Leers’s picture

Issue summary: View changes

Closed #2906894: Config import does not work for custom view modes if bundles not selected as a duplicate; it is a consequence of this bug.

Wim Leers’s picture

Wim Leers’s picture

Title: When using view modes as display plugins, those config dependencies aren't in the export » [config dependencies] When using view modes as display plugins, those config dependencies aren't in the export
oknate’s picture

Updating patch based on feedback.

oknate’s picture

updating last patch, I saw it was using entityManager, not entityTypeManager

oknate’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work
Issue tags: -Needs manual testing

Looking even better! Thanks, @oknate. And thanks for the added test coverage!

  1. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/ViewModeFieldFormatter.php
    @@ -59,4 +59,20 @@ class ViewModeFieldFormatter extends EntityReferenceFieldFormatter {
    +    $viewModeManager = $this->entityTypeManager->getStorage('entity_view_mode');
    

    Total nitpick, but can we rename this to $view_mode_storage, since it's a storage handler?

  2. +++ b/src/Plugin/entity_embed/EntityEmbedDisplay/ViewModeFieldFormatter.php
    @@ -59,4 +59,20 @@ class ViewModeFieldFormatter extends EntityReferenceFieldFormatter {
    +    foreach ($definition['entity_types'] as $type) {
    +      if ($view_mode = $viewModeManager->load($type . '.' . $view_mode)) {
    +        $this->addDependency($view_mode->getConfigDependencyKey(), $view_mode->getConfigDependencyName());
    +      }
    +    }
    

    I think that $storage->loadMultiple() might be a preferable approach here.

  3. +++ b/tests/src/Functional/ViewModeFieldFormatterTest.php
    @@ -53,4 +53,29 @@ class ViewModeFieldFormatterTest extends EntityEmbedTestBase {
    +    $this->assertNotContains('core.entity_view_mode.node.teaser', $dependencies['config']);
    +
    +  }
    

    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.

oknate’s picture

Updating patch based on feedback.

phenaproxima’s picture

Issue tags: +Needs reroll

Looks like a reroll is needed here :(

oknate’s picture

Added test for deleting view mode but it's currently failing.

oknate’s picture

Testing update to functional test.

oknate’s picture

Testing update to functional test.

oknate’s picture

Testing update to functional test.

oknate’s picture

Coding standard fixes, minor changes.

oknate’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Assigned: Unassigned » phenaproxima
Issue tags: -Needs reroll

Getting very late here, assigning to @phenaproxima for review :)

  • Wim Leers committed 7239111 on 8.x-1.x authored by oknate
    Issue #2841964 by oknate, Wim Leers, daften, phenaproxima, mstef,...
Wim Leers’s picture

Assigned: phenaproxima » Unassigned
Status: Needs review » Fixed
Issue tags: +BarcelonaMediaSprint
FileSize
1.45 KB

Reviewed 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:

  1. +++ b/tests/src/Functional/ViewModeFieldFormatterTest.php
    @@ -53,4 +53,43 @@ class ViewModeFieldFormatterTest extends EntityEmbedTestBase {
    +   * Test view mode dependency.
    ...
    +    // Test that removing teaser view mode removes the dependency.
    

    Language nits here. Fixed.

  2. +++ b/tests/src/Functional/ViewModeFieldFormatterTest.php
    @@ -53,4 +53,43 @@ class ViewModeFieldFormatterTest extends EntityEmbedTestBase {
    +    // Test that deleting teaser view mode removes the dependency.
    +    $teaserViewMode = $this->container
    +      ->get('entity_type.manager')
    +      ->getStorage('entity_view_mode')
    +      ->load('node.teaser');
    +    $teaserViewMode->delete();
    +    $config['display_plugins'] = [
    +      'view_mode:node.teaser',
    +      'view_mode:node.full',
    +    ];
    +    $button->set('type_settings', $config);
    +    $button->save();
    +    $dependencies = $button->getDependencies();
    +    $this->assertNotContains('core.entity_view_mode.node.teaser', $dependencies['config']);
    

    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 :)

oknate’s picture

The 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.

Wim Leers’s picture

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.

If 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 :)

Status: Fixed » Closed (fixed)

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