Problem/Motivation

Trying to mock the EntityDisplayInterface will lack the entity methods. PhpStorm doesn't find them either.

Proposed resolution

EntityDisplayInterface should extend ConfigEntityInterface. As the implementations already do this is not a problem (as the passing patch shows).

CommentFileSizeAuthor
#27 2175517_27.patch806 bytesalimac
#14 2175517_14.patch681 bytesbenjy
#1 2175517_1.patch680 byteschx
edi_e_cei.patch683 byteschx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

Status: Active » Needs review
FileSize
680 bytes
chx’s picture

Issue summary: View changes
aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Don't see anything wrong with this

swentel’s picture

+1 from me

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

Berdir’s picture

Hm, #2031725: Move all entity display interfaces to the core component did explicitly remove this with a lot of discussions around this.

webchick’s picture

Status: Fixed » Needs review

Oh, thanks. Was not aware.

Rolled back for now, pending further discussion.

Berdir’s picture

Not sure if it was necessary, I commented over there to point the involved people to this issue

yched’s picture

You don't necessarily have to be a configEntity to do the job of an EntityDisplay ("host display settings for the components of an entity")

Why do we want to mock an EntityDisplayInterface rather than an EntityDisplay or an EntityFormDisplay specifically ?

chx’s picture

Because mocking classes is worse than interfaces, you need to disable the constructor, etc.

Berdir’s picture

@yched: The thing is that we need a single interface to type hint in e.g. entity crud hooks, where we want both the ConfigEntityInterface methods and those of EntityDisplay.

The only thing that could work is having both this interface and one for the entity, that extends from this and ConfigEntityInterface, but that also seems fairly complicated.

Berdir’s picture

1: 2175517_1.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 1: 2175517_1.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review
FileSize
681 bytes

New patch.

Berdir’s picture

Assigned: swentel » Unassigned
Status: Needs review » Reviewed & tested by the community

Here's why I think this makes sense:

Code from EmailFieldTest, but any other snippet is just the same:

    entity_get_display('entity_test', 'entity_test', 'full')
      ->setComponent($field_name, array(
        'type' => 'email_mailto',
      ))
      ->save();

PhpStorm analysis: "Method 'save' not found in class \Drupal\Core\Entity\Display\EntityDisplayInterface".

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2175517_14.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review

14: 2175517_14.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 14: 2175517_14.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review

14: 2175517_14.patch queued for re-testing.

benjy’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC, it was just the test bot playing up.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2175517_14.patch, failed testing.

benjy’s picture

Status: Needs work » Needs review

14: 2175517_14.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Aw testbot...

fago’s picture

hm, that was changed by purpose to be decoupled, but #15 and the entity_get_display() documentation provide good reasons to re-introduce the coupling - thus agreed.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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

alimac’s picture

Status: Closed (fixed) » Needs review
FileSize
806 bytes

Patch in #14 was never committed. Here is the patch for the latest HEAD.

alimac’s picture

Issue tags: +Amsterdam2014
Berdir’s picture

Status: Needs review » Reviewed & tested by the community

Weird, but +1 to the latest patch.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. For real this time. :)

  • webchick committed 145dfbe on 8.0.x
    Issue #2175517 by chx, benjy, alimac: Fixed Entity displays are...

Status: Fixed » Closed (fixed)

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