Follow-up to #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration

Problem/Motivation

The EntityViewDisplay and EntityFormDisplay config entity would need to implement ThirdPartySettingsInterface if we want to allow contrib modules to store module specific metadata to the entity displays. This will benefit modules like Display suite, Field group etc

Proposed resolution

Implement ThirdPartySettingsInterface and ThirdPartySettingsTrait in EntityViewDisplay and EntityFormDisplay.

Remaining tasks

Agree approach.
Review patch.

User interface changes

None

API changes

None

CommentFileSizeAuthor
#13 interdiff.txt801 bytesswentel
#13 2333501-13.patch5.39 KBswentel
#7 interdiff.txt731 bytesswentel
#7 2333501-7.patch5.4 KBswentel
#6 2333501-6.patch5.4 KBswentel
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented
Issue summary: View changes
Parent issue: #2331019: Implement ThirdPartySettingsInterface in Vocabulary » #2224761: Add a generic way to add third party configuration on configuration entities and implement for field configuration
swentel’s picture

swentel CreditAttribution: swentel commented

+1 ! Should be able to work on this coming sunday

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented
Title: Implement ThirdPartySettingsInterface in EntityViewMode » Implement ThirdPartySettingsInterface in EntityViewDisplay
Issue summary: View changes

After chatting with timplunkett and berdir, who explained the difference between EntityViewMode and EntityViewDisplay, I came to the conclusion that this issue should in fact be about implementing third party settings in EntityViewDisplay, because that is where the display settings of a view mode are stored.

swentel’s picture

swentel CreditAttribution: swentel commented
Title: Implement ThirdPartySettingsInterface in EntityViewDisplay » Implement ThirdPartySettingsInterface in EntityView|FormDisplay
Issue summary: View changes

Ha totally right, I've been misreading the title this whole time. Also, let's do this for entityFormDisplay at the same time as well.

swentel’s picture

swentel CreditAttribution: swentel commented
Issue summary: View changes
swentel’s picture

swentel CreditAttribution: swentel commented
Status: Active » Needs review
FileSize
2333501-6.patch5.4 KB

This should do it

swentel’s picture

swentel CreditAttribution: swentel commented
FileSize
2333501-7.patch5.4 KB
interdiff.txt731 bytes

small doc update

aspilicious’s picture

aspilicious CreditAttribution: aspilicious commented
Status: Needs review » Reviewed & tested by the community

Looks fine, in line with the referenced issue. Has tests and it's a step forward for contrib!

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented

Thanks Swentel, that looks awesome. Please allow me to ask some questions, if only to help me understand what exactly is going on :-)

  1. +++ b/core/config/schema/core.entity.schema.yml
@@ -77,6 +77,14 @@ core.entity_view_display.*.*.*:
+    dependencies:
+      type: config_dependencies
+      label: 'Dependencies'

    I'm not familiar with this, what does 'dependencies' do? 

  2. +++ b/core/modules/entity/src/Tests/EntityDisplayTest.php
@@ -8,14 +8,14 @@
-class EntityDisplayTest extends DrupalUnitTestBase {
+class EntityDisplayTest extends KernelTestBase {

    What is the reason for this change? 

  3. +++ b/core/modules/system/tests/modules/entity_test/entity_test.module
@@ -369,6 +369,10 @@ function entity_test_entity_presave(EntityInterface $entity) {
+  if ($entity->getEntityType()->id() == 'entity_view_display') {
+    $entity->setThirdPartySetting('entity_test', 'foo', 'bar');
+  }

    Shouldn't there be tests for getThirdPartySetting too? It seems to me that you're only testing if third party settings can be read, not written. And what about EntityFormDisplay, can we simply assume that it works if this test passes, or does it need its own tests?

swentel’s picture

swentel CreditAttribution: swentel commented

@marcvangend

1. dependencies is used in every config entity to store the dependencies on either modules, themes. This is automatically calculated by calculateDependencies() method and stores information on which modules this config entity depends. e.g. if a entity views display config entity has information about a formatter which is provided by the image module, you'll find the image dependency there. This change just adds it to the schema because it was missing.

2. KernelTestBase is the successor of UnitTestBase which is deprecated (and will probably be removed one day)

3. The test itself calls getThirdPartySetting on a $display object so that's fine. EntityViewDisplay and EntityFormDisplay both implement EntityDisplayInterface so this should be fine by only testing EntityViewDisplay - unless a core committer decides otherwise.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented

1. Dependencies are used to track dependencies to other config entities. For example, displays depend on the bundle config entity and fields. however, this shouldn't be necessary if we use type: config_entity, which already contains this?

2. DrupalUnitTestBase was renamed to KernelTestBase (it is not a unit test and has nothing in common with unit/phpunit tests.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Reviewed & tested by the community » Needs work

So yes, berdir is right config_entity has dependencies so this patch should not add it. core.entity_form_display still has it though, so we can remove it in the same run there.

swentel’s picture

swentel CreditAttribution: swentel commented
Status: Needs work » Needs review
FileSize
2333501-13.patch5.39 KB
interdiff.txt801 bytes

New patch

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir commented
Status: Needs review » Reviewed & tested by the community

Looks good!

marcvangend’s picture

marcvangend
he/him
Primary language Dutch
Location Amsterdam
CreditAttribution: marcvangend commented

Excellent. I didn't do much here but I learned a lot, thanks guys!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
Status: Reviewed & tested by the community » Fixed

Committed 2735f12 and pushed to 8.0.x. Thanks!

  • alexpott committed 2735f12 on 8.0.x 
    Issue #2333501 by swentel | marcvangend: Implement...
yched’s picture

yched CreditAttribution: yched commented

Just wondering: what kind of use cases do we have in mind for DS / field_group ?

swentel’s picture

swentel CreditAttribution: swentel commented

@yched :

display suite needs to store the template we will switch to for a given type, bundle and view mode. We store some extra variables there as well
as for field group well, we now have a way to store the parents and children without having to come up with our own storage, however, for field group we could use the components if we get that one in soon enough. Aspilicious and I are going to look at that one in amsterdam as well.

yched’s picture

yched CreditAttribution: yched commented

@swentel: thanks, makes sense.
I was indeed more expecting f_g to need the "component types" patch, and getting that one in is definitely in my hitlist for Amsterdam :-)

Status: Fixed » Closed (fixed)

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

yched’s picture

yched CreditAttribution: yched commented

FYI : #2397807: EntityDisplay schema for third_party_settings is wrong