Map data types to interfaces to make typed data discoverable

As discussed, keeping the annotations on the classes and require an interface definition is I think more natural for plugin annotations.

Hm, thinking about it, we might not be able to/have to/want to make it required, as we simply can't provide interfaces for dynamic/derived types like entity bundles.

I would simply say that it is required *if* you want to make things like param converting work or other things that rely on it.

If that's the case, then this is also an API addition and not a change (I know it's required for param converted, we should still aim for avoiding API changes when possible)

So re-titling this to be less restrictive.

#1906810: Require type hints for automatic entity upcasting is progressing well without this issue. Do we still need it?

Let's start with it.

This is now major, because after #2190313: Add $EntityType::load() and loadMultiple() to simplify loading entities you can no longer replace an entity type without subclassing the base class.

I.e. if I want to provide a different implementation for nodes i.e. BetterNode, it should suffice to implement NodeInterface, but since the aforementioned issue, loading nodes in that case won't work unless I also subclass Node. I.e. Node::load() won't work.

That is because there is no relation from an entity type to its interface.

I don't think that will work in cases where we subclass entity types and expose them as different entities, the test entity classes for example.

And as discussed in that issue, one of the reasons we added those complex checks an verifications is to explicitly support things like ECK that will want to dynamically expose entity types based on configuration (this is also why we pass the entity type to baseFieldDefinitions() for example). Requiring a unique interface for each entity type will prevent that from working at all.

Yes, code generation/scaffolding tools will possibly become more common in 8.x than dynamically defining them, but I think we should still aim to support that. I have similar use cases in 7.x too, where I want to dynamically expose external entities to Drupal

And at least for content entities, there's no reason to subclass/replace entity classes IMHO. I don't think there's anything that you can't do by altering fields/hooks?

And at least for content entities, there's no reason to subclass/replace entity classes IMHO

Well that's one half of the API.

I'm really frustrated about that issue going in, it flies in the face of all the work we've done to use interfaces properly.

Why wouldn't ECK and modules like it be able to discover entity types with reflection instead of configuration. Why does it have to be through configuration? I've often thought it would be better if ECK generated code anyway. That would make using it a lot more versatile.

Extended but this is still not complete.

This is a complete list.

Ups.

Is this issue still possible to be worked on? In general we need quite some reroll here.

I think this is probably beta deadline? But still possible.

Here's a reroll. Not claiming to understand it :)

I'm not 100% sure about the new changes I made, they should definitely be looked over by someone who knows more about what's happening - see interdiff. But this should cover all @*EntityType annotations.

Oh darn, I was working on the reroll too. I should've assigned this to me :)

Well, it's good change to review by comparing our work, I did run our patches diff in my local and they were identical apart from the interface identified under core/modules/system/tests/modules/entity_test:
I used \Drupal\user\EntityOwnerInterface whereas you used \Drupal\Core\Entity\ContentEntityInterface.

(+minor changes which was the $interface property declaration location)

There were 16 cases which I/we checked on.
Files that were missing:

• core/modules/breakpoint/src/Entity/Breakpoint.php
• core/modules/breakpoint/src/Entity/BreakpointGroup.php
• core/modules/contact/src/Entity/Category.php
• core/modules/field/src/Entity/FieldInstanceConfig.php
• core/modules/language/src/Entity/Language.php

Files that were moved:

• core/modules/entity/src/Entity/EntityFormDisplay.php => core/lib/Drupal/Core/Entity/Entity/EntityFormDisplay.php
• core/modules/entity/src/Entity/EntityFormMode.php => core/lib/Drupal/Core/Entity/Entity/EntityFormMode.php
• core/modules/system/src/Entity/DateFormat.php => core/lib/Drupal/Core/Datetime/Entity/DateFormat.php

New @ConfigEntityType cases:

• core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php => interface = "Drupal\Core\Field\FieldConfigInterface"
• core/modules/contact/src/Entity/ContactForm.php => interface = "Drupal\contact\ContactFormInterface"
• core/modules/field/src/Entity/FieldConfig.php => interface = "Drupal\field\FieldConfigInterface"
• core/modules/language/src/Entity/ConfigurableLanguage.php => interface = "Drupal\language\ConfigurableLanguageInterface"

New @ContentEntityType cases:

• core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraintViolation.php
• core/modules/system/tests/modules/entity_test/src/Entity/EntityTestFieldOverride.php
• core/modules/system/tests/modules/entity_test/src/Entity/EntityTestUpdate.php

I'm now going through and make sure that the interface names haven't actually changed.

Found five namespace changes.

core/lib/Drupal/Core/Datetime/Entity/DateFormat.php
- *   interface = "Drupal\system\DateFormatInterface",
+ *   interface = "Drupal\Core\Datetime\DateFormatInterface",

core/lib/Drupal/Core/Entity/Entity/EntityFormMode.php
- *   interface = "Drupal\entity\EntityFormModeInterface",
+ *   interface = "Drupal\Core\Entity\EntityFormModeInterface",

core/lib/Drupal/Core/Entity/Entity/EntityViewMode.php
- *   interface = "Drupal\entity\EntityViewModeInterface",
+ *   interface = "Drupal\Core\Entity\EntityViewModeInterface",

core/modules/menu_link_content/src/Entity/MenuLinkContent.php
- *   interface = "Drupal\menu_link_content\Entity\MenuLinkContentInterface",
+ *   interface = "Drupal\menu_link_content\MenuLinkContentInterface",

core/modules/node/src/Entity/Node.php
- *   interface = "Drupal\Node\NodeInterface",
+ *   interface = "Drupal\node\NodeInterface",

See interdiff.txt and new patch with updated ones (continuing from #19, ignore #20 patch but consider my our difference on the interface identified between #19 and #20).

Now, there's documentation we probably need to update. Here's my initial change in entity.api.php.

diff --git a/core/modules/system/entity.api.php b/core/modules/system/entity.api.php
index 09860a6..d003410 100644
--- a/core/modules/system/entity.api.php
+++ b/core/modules/system/entity.api.php
@@ -277,6 +277,8 @@
  *   content entity type that uses bundles, the 'bundle_label' annotation gives
  *   the human-readable name to use for a bundle of this entity type (for
  *   example, "Content type" for the Node entity).
+ * - The 'interface' annotation refers to an interface which the class should
+ *   implement.
  * - The annotation will refer to several controller classes, which you will
  *   also need to define:
  *   - list_builder: Define a class that extends

I'm not sure whether we should add interface entry into annotations example in core.api.php:1560-1569:

 * To annotate a class as a plugin, add code similar to the following to the
 * end of the documentation block immediately preceding the class declaration:
 * @code
 * * @ContentEntityType(
 * *   id = "comment",
 * *   label = @Translation("Comment"),
 * *   ...
 * *   base_table = "comment"
 * * )
 * @endcode

Oops, well no sweat and thanks for taking this even further @iMiksu! I was just inspired this morning to try and push a beta deadline issue forward.

Can we get a clear explanation of the problems this issue is solving? Especially since #1906810: Require type hints for automatic entity upcasting got solved without this.

I do not think this should be in beta-deadline, as adding an entity interface can continue to be an optional thing. Yes it's best practice, but nothing is going to break if you not add it. You may not receive some of the benefits of APIs that build upon on it, but all the existing base-functionality like #1906810: Require type hints for automatic entity upcasting continuous to work without.

This would be very useful to allow token API to discover data types and work based on typed data API without breaking it. Related: #1920688: Support multiple instances of the same token type in token replacement.

Re-roll.

Reviewed the existing code.

1. +++ b/core/lib/Drupal/Core/Field/Entity/BaseFieldOverride.php
   @@ -20,6 +20,7 @@
     *   id = "base_field_override",
   + *   interface = "Drupal\Core\Field\FieldConfigInterface",
   

   Looks like this interface does not belong to this entity type, so it should not be added.

2. +++ b/core/lib/Drupal/Core/TypedData/Annotation/DataType.php
   @@ -106,4 +106,13 @@ class DataType extends Plugin {
   +  protected $interface;
   

   Needs to be public.

3. +++ b/core/lib/Drupal/Core/TypedData/TypedDataManager.php
   @@ -396,4 +396,63 @@ public function clearCachedDefinitions() {
   +   * @param mixed @param
   +   *
   ...
   +   */
   +  public function getPluginIdForData($data) {
   

   Wrong docs. Maybe should be better named, e.g. discoverDataType().

4. +++ b/core/modules/config/tests/config_test/src/Entity/ConfigQueryTest.php
   @@ -12,6 +12,7 @@
   + *   interface = "Drupal\config_test\ConfigTestInterface",
   

   Looks like this is the wrong one.

5. +++ b/core/modules/field_ui/tests/modules/field_ui_test/src/Entity/FieldUITestNoBundle.php
   @@ -14,6 +14,7 @@
   + *   interface = "Drupal\Core\Entity\EntityInterface",
   

   pointless.

6. +++ b/core/modules/system/tests/Drupal/system/Tests/TypedData/TypedDataManagerUnitTest.php
   @@ -0,0 +1,86 @@
   +    $this->cache = $this->getMock('\Drupal\Core\Cache\CacheBackendInterface');
   

   should just use nullcache?

7. +++ b/core/modules/system/tests/Drupal/system/Tests/TypedData/TypedDataManagerUnitTest.php
   @@ -0,0 +1,86 @@
   +
   +    $this->moduleHandler = $this->getMock('\Drupal\Core\Extension\ModuleHandlerInterface');
   +
   ...
   +
   

   weird empty lines.

8. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTest.php
   @@ -19,6 +19,7 @@
   + *   interface = "Drupal\Core\Entity\ContentEntityInterface",
   
   +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestBaseFieldDisplay.php
   @@ -15,6 +15,7 @@
   + *   interface = "Drupal\Core\Entity\ContentEntityInterface",
   

   pointless.

9. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestCache.php
   @@ -12,6 +12,7 @@
   + *   interface = "Drupal\Core\Entity\ContentEntityInterface",
   

   pointless

10. +++ b/core/modules/system/tests/modules/entity_test/src/Entity/EntityTestConstraintViolation.php
    @@ -14,6 +14,7 @@
    + *   interface = "Drupal\Core\Entity\ContentEntityInterface",
    

    pointless. (and quite some more of them ;)

Wrong docs. Maybe should be better named, e.g. discoverDataType().

How is that wrong exactly?

should just use nullcache?

No, because we need to feed the definitions to the manager somehow.

weird empty lines.

What do you mean?

Not sure how we'd move on here to also support type inheritance for primitives:

timestamp is a integer
integer -> IntegerInterface?
timestamp is a datetime
datetime -> DateTimeInterface?

For having a data type 'datetime' such that you can specfiy that as requirement (e.g. context) which then matches with a "timestamp" argument, we need to know that "datetime" does not simply implement DateTimeInterface, but basically is represented by DateTimeInterface. Same for being able to pass "email" typed data to a "string" slot.

Consequently, we need to have the interface annotated for those primitive types also. But then, we have once interfaces which are on the typed data classes and once interface on the data objects. So how to we handle that differentation best?
If it's only about type-matching, there we do not really care on where the interfaces are implemented - but then, still it would be a bit weird if it's not clearly specified where the interface actually is going to be implemented? Thus, I'd rather add two keys: 'value_interface' and 'data_interface' and leverage both for type-matching?

'data_interface' could be just 'type_interface' maybe? So far, it would mostly applies to primitive interfaces and datetime, duration - so type interface would fit here.

Based on #36 and #37 this is needs work?