Move all entity display interfaces to the core component

#2: d8_interface.patch queued for re-testing.

Rerolled for today's HEAD.

This should be it.

Should be better: I had missed one replacement.

Might be a transient error: the test which fails here passes locally on my machine and passes on simpletest.me too. Retrying.

#9: 0001-Issue-2031725-by-fago-fgm-Move-entity-API-interfaces.patch queued for re-testing.

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AddFeedTest.php
@@ -24,6 +24,7 @@ public static function getInfo() {
   function testAddFeed() {
     $feed = $this->createFeed();
+    debug(gettype($feed), "Feed");
 

+++ b/core/modules/aggregator/lib/Drupal/aggregator/Tests/AggregatorTestBase.php
@@ -59,6 +59,7 @@ function createFeed($feed_url = NULL, array $edit = array()) {
     $this->assertTrue(!empty($fid), 'The feed found in database.');
+    debug($fid, __METHOD__ . "/$fid");

Left-over debugs()

Ooooppppsssss....

Tagging.

This seems to do it. Does it still pass ?

Looks like I missed one replacement.

Forgot to attach interdiff with previous patch.

Rerolled after conflicting commits to 8.x yesterday. No changes.

Rerolled accordingly.

oops, patch upload didn't make it.

Discussed this a bit with fago and we agreed that there's really not much use for the new Entity[Form]DisplayConfigEntityInterface provided by the entity module. I guess only field_ui could typehint with them if we decide to do something crazy related to the config storage side, but I'd prefer to wait until that happens. In the meantime, they seem a bit over the top so let's remove them :)

Only users were EntityDisplay and EntityFormDisplay. Rerolled according to #26.

Thanks, I think it's ready now.

#27: 0001-Issue-2031725-by-fago-fgm-Move-entity-API-interfaces.patch queued for re-testing.

Rerolling in a moment.

Rerolled on latest head. Does it still pass ? The only change appears to be the TranslatableInterface on TypedData.

Seems I has missed one line to remove.

Patch is almost identical to the previous one, just a tiny context change.

#34: 0001-Issue-2031725-by-fago-fgm-Move-entity-API-interfaces.patch queued for re-testing.

Re-roll.

#38: field-interfaces-2031725-38.patch queued for re-testing.

1. +++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
   @@ -2,17 +2,15 @@
   -interface EntityDisplayBaseInterface extends ConfigEntityInterface {
   +interface EntityDisplayInterface {
   

   I like that, that makes a ton of sense. Both the naming and the removed interface.
   ++

2. +++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
   @@ -23,7 +21,7 @@
   +   * @return \Drupal\Core\Entity\Display\EntityDisplayInterface
   
   @@ -56,7 +54,7 @@ public function getComponent($name);
   -   * @return \Drupal\entity\Entity\EntityDisplay
   +   * @return \Drupal\Core\Entity\Display\EntityDisplayInterface
   
   @@ -67,7 +65,7 @@ public function setComponent($name, array $options = array());
   -   * @return \Drupal\entity\Entity\EntityDisplay
   +   * @return \Drupal\Core\Entity\Display\EntityDisplayInterface
   

   If this were simply following the rename it would be EntityViewDisplay now. But a) this is in EntityDisplayInterface, which knows nothing EntityViewDisplay(Interface) b) typehinting the interface is of course preferred.

   In other words, this should have been EntityBaseDisplayInterface before, so it is in fact a (minor) bugfix.

   Nice!

3. +++ b/core/lib/Drupal/Core/Entity/EntityRenderController.php
   @@ -136,7 +136,7 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   -   * @param \Drupal\entity\Entity\EntityDisplay $display
   +   * @param \Drupal\Core\Entity\Display\EntityDisplayInterface $display
   
   @@ -145,7 +145,7 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   -  protected function alterBuild(array &$build, EntityInterface $entity, EntityDisplay $display, $view_mode, $langcode = NULL) { }
   +  protected function alterBuild(array &$build, EntityInterface $entity, EntityDisplayInterface $display, $view_mode, $langcode = NULL) { }
   
   +++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockRenderController.php
   @@ -19,7 +19,7 @@ class CustomBlockRenderController extends EntityRenderController {
   -  protected function alterBuild(array &$build, EntityInterface $entity, EntityDisplay $display, $view_mode, $langcode = NULL) {
   +  protected function alterBuild(array &$build, EntityInterface $entity, EntityDisplayInterface $display, $view_mode, $langcode = NULL) {
   
   +++ b/core/modules/comment/lib/Drupal/comment/CommentRenderController.php
   @@ -146,7 +146,7 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
   -  protected function alterBuild(array &$build, EntityInterface $comment, EntityDisplay $display, $view_mode, $langcode = NULL) {
   +  protected function alterBuild(array &$build, EntityInterface $comment, EntityDisplayInterface $display, $view_mode, $langcode = NULL) {
   
   +++ b/core/modules/node/lib/Drupal/node/NodeRenderController.php
   @@ -80,7 +80,7 @@ public function buildContent(array $entities, array $displays, $view_mode, $lang
   -  protected function alterBuild(array &$build, EntityInterface $entity, EntityDisplay $display, $view_mode, $langcode = NULL) {
   +  protected function alterBuild(array &$build, EntityInterface $entity, EntityDisplayInterface $display, $view_mode, $langcode = NULL) {
   
   +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/TermRenderController.php
   @@ -51,7 +51,7 @@ protected function getBuildDefaults(EntityInterface $entity, $view_mode, $langco
   -  protected function alterBuild(array &$build, EntityInterface $entity, EntityDisplay $display, $view_mode, $langcode = NULL) {
   +  protected function alterBuild(array &$build, EntityInterface $entity, EntityDisplayInterface $display, $view_mode, $langcode = NULL) {
   

   This, though should be EntityViewDisplayInterface, if I'm not mistaken.

4. +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityDisplay.php
   @@ -31,7 +32,7 @@
   -class EntityDisplay extends EntityDisplayBase implements EntityDisplayInterface {
   +class EntityDisplay extends EntityDisplayBase implements EntityDisplayInterface, ConfigEntityInterface {
   
   +++ b/core/modules/entity/lib/Drupal/entity/Entity/EntityFormDisplay.php
   @@ -31,7 +32,7 @@
   -class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayInterface, \Serializable {
   +class EntityFormDisplay extends EntityDisplayBase implements EntityFormDisplayInterface, ConfigEntityInterface, \Serializable {
   
   +++ b/core/modules/entity/lib/Drupal/entity/EntityDisplayBase.php
   @@ -8,12 +8,13 @@
   -abstract class EntityDisplayBase extends ConfigEntityBase implements EntityDisplayBaseInterface {
   +abstract class EntityDisplayBase extends ConfigEntityBase implements EntityDisplayInterface {
   

   I'm pretty sure EntityDisplay should extend EntityViewDisplayInterface and by extension, be called EntityViewDisplay.

   EntityDisplayBase can then be just EntityDisplay. Also that should directly implement ConfigEntityInterface I think.

5. +++ b/core/modules/system/entity.api.php
   @@ -426,7 +426,7 @@ function hook_entity_query_alter(\Drupal\Core\Entity\Query\QueryInterface $query
   - * @param \Drupal\entity\Entity\EntityDisplay $display
   + * @param \Drupal\Core\Entity\Display\EntityViewDisplayInterface $display
   

   Also, below:
   Interfaces++
   (Again, this is a minor fix of the previous code)

Trying to reroll #38

Take 2, re-roll #38

In DisplayOverviewBase.php instead of
+use Drupal\entity\EntityDisplayInterface;
should be probably
+use Drupal\Core\Entity\Display\EntityDisplayInterface;
Despite that, are there any other issues you see with this re-roll?

Applying quick fix #47

#41.3 and #41.4 are not adressed as far as I can see.

I've just done re-roll of previous patch since I was not quite sure how to implement those mentioned in #41.3 and #41.4
Can you please guide me by giving some steps of execution, what exactly needs to be done?

Would be nice to update the issue with a summary with of the renames / moving around that the patch does ?

Also, minor:

+++ b/core/lib/Drupal/Core/Entity/Display/EntityDisplayInterface.php
@@ -23,7 +21,7 @@
-   * @return \Drupal\entity\Entity\EntityDisplay
+   * @return \Drupal\Core\Entity\Display\EntityDisplayInterface

@@ -56,7 +54,7 @@ public function getComponent($name);
-   * @return \Drupal\entity\Entity\EntityDisplay
+   * @return \Drupal\Core\Entity\Display\EntityDisplayInterface

@@ -67,7 +65,7 @@ public function setComponent($name, array $options = array());
-   * @return \Drupal\entity\Entity\EntityDisplay
+   * @return \Drupal\Core\Entity\Display\EntityDisplayInterface

Apparently we now can use "@return self" for those cases.

Rerolling #49 + inline doc update from #52. It's my first reroll… lets see if this passes…

53: field_interfaces-2031725-53.patch queued for re-testing.

+++ b/core/modules/block/custom_block/lib/Drupal/custom_block/CustomBlockViewBuilder.php
@@ -7,9 +7,9 @@
+use Drupal\Core\Entity\Display\EntityViewDisplayInterface;

Why are we changing the name of this interface when the name of the entity class stays the same? Can't we just move the interfaces like the issue title says and discuss whether this renaming is a good idea?

Well, I'm not sure I see why we keep specific empty Entity(View)DisplayInterface and EntityFormDisplayInterface interfaces for the "view" and "form" flavors ?

The design is that they work just the same with the same APIs, we could just keep the single EntityDisplayBaseInterface (and rename it to EntityDisplayInterface) ?

That would be even better.

EntityDisplayBase extends ConfigEntityBase, thus implements the interface already. So what's wrong is that the specific class do so *again* - removed that.

I totally missed that, you are of course correct. Thanks for setting me straight.

I disagree with removing the specialized interfaces. It's awesome that we have the generic interface which should be used in many cases. But in certain cases it's just wrong to type hint the generic interface. hook_entity_view() for example is conceptually tied to view displays. Receiving a form display there is bogus.

I'm sorry but I don't see how a "combined" EntityDisplayInterface ties anything to a view or a form display.

Both entity types (EntityDisplay and EntityFormDisplay) have the same methods and share a lot of code already through the base class..

I wasn't able to find a generic use of getRenderer() either.

The use case for having a single method name for both of them (which, looking in retrospective, is pretty weak) is the Field UI base form class for the "Manage Display" and "Manage Form Display" forms, but those could just switch to use an internal helper method.

getRenderer() seems weird to me. There is no RendererInterface object or something that returns, so instead there should be getFormatter() and getWidget() methods in the respective interface

The intent of the generic getRenderer() is that it will return a FieldGroupRendererPlugin (or whatever field_group.module D8 will chose to call its "field groups formatter" plugins) on components that are field_groups.

#1875974: Abstract 'component type' specific code out of EntityDisplay is all about making the "components" in EntityDisplays be of different, pluggable types - field_groups, Display suite custom fields, ...
So, yes, that's intentional, there's no RendererInterface, what the getRenderer() method returns depends on the component type. EntityDisplay lets the ComponentHanlders generate them, and persists them throughout the multiple entity rendering.

And so, no, there's no intention to add EntityViewDisplay::getFormatter() / EntityFormDisplay::getWidget()...

Also:

the display object could even take over form element generation in future, i.e. receive a getFormElements() method

Yes, not sure we'll get there in D8, but that could totally be the case at some point - and then the ComponentHandler takes care of applying the "renderer" internally, and just return a render array.
Still, Field UI's "Manage dispay" screens need to present a list of heterogeneous rows (fields, field groups...) and display settings form - hence the need to access a 'render plugin for the row", whatever that is.

Re #69: I was arguing that by only having a single interface, you cannot type-hint whether you want to receive a view display or a form display in your functions. entity_view() is one case where this is weird, because it conceptually makes little sense to pass a form display.

@tsoeckler: true - I don't see that as a big issue, but TBH I have no strong opinion.

As long as the patch renames / gets rid of EntityDisplayBaseInterface, I'm fine with two separate interfaces. And also +1 on renaming entity_display / EntityDisplay to entity_view_display / EntityViewDisplay, here or later.

So unless @amateescu is strongly opposed, RTBC for me.

Well, since it appears that we have an agreement on renaming EntityDisplay to EntityViewDisplay in a followup, then renaming only the interface here doesn't hurt anyone :)

+++ b/core/lib/Drupal/Core/Entity/Display/EntityFormDisplayInterface.php
@@ -2,16 +2,14 @@
-interface EntityDisplayInterface extends EntityDisplayBaseInterface {
+interface EntityFormDisplayInterface extends EntityDisplayInterface {

+++ b/core/lib/Drupal/Core/Entity/Display/EntityViewDisplayInterface.php
@@ -2,16 +2,14 @@
-interface EntityFormDisplayInterface extends EntityDisplayBaseInterface {
+interface EntityViewDisplayInterface extends EntityDisplayInterface {

git diff is just stupid... :-)

Looked through the patch again, and all is well. RTBC++

83: d8_entity_display.patch queued for re-testing.

RTBC if green.
Several patches are working on EntityDisplay stuff, would be good to have this cleanup first.

We need to ensure that any existing change notices that reference these interfaces are updated.

Committed dd663c4 and pushed to 8.x. Thanks!

I looked through all existing change notices that contained the word 'display' and updated some of them for the new interface name:
https://drupal.org/node/2018597/revisions/view/2731159/6808489
https://drupal.org/node/1907792/revisions/view/2549206/6808513
https://drupal.org/node/1875952/revisions/view/2721457/6808523

I'm not sure whether we need a separate change notice for this, #90 seems to imply that not to be the case.

I also remembered that we wanted to do: #2165835: Rename EntityDisplay to EntityViewDisplay in accordance with its interface

Note: #2175517: Entity displays are themselves config entities did re-add the extends ConfigEntityInterface to EntityDisplayInterface and I can see why, if you type hint on the interface, then you can not use methods from EntityInterface, which is a problem.