Promote EntityType to a domain object

+++ b/core/lib/Drupal/Core/AnnotationReader.phpundefined
@@ -0,0 +1,97 @@
+  public function getClassAnnotations($class_name) {
+    return $this->getFromCache($class_name) ?: $this->doGetClassAnnotations($class_name);
+  }

I try to understand why there is a need for caching on the annotation reading level and not just on the entity manager level.

+++ b/core/lib/Drupal/Core/AnnotationReader.phpundefined
@@ -0,0 +1,97 @@
+  protected function getFromCache($name) {
+    return isset($this->cache[$name]) ? $this->cache[$name] : FALSE;
+  }

As annotations are translatable the cache needs to be per langcode.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.phpundefined
@@ -106,19 +107,12 @@ class ConfigStorageController implements EntityStorageControllerInterface, Entit
-    $this->idKey = $this->entityInfo['entity_keys']['id'];
-
-    if (isset($this->entityInfo['entity_keys']['status'])) {
-      $this->statusKey = $this->entityInfo['entity_keys']['status'];
-    }
-    else {
-      $this->statusKey = FALSE;
-    }
-
+    $this->idKey = $this->entityInfo->getKey('id');

This is simply great in comparison.

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -70,16 +96,34 @@ class EntityType extends Plugin {
+  public function getController($controller) {
+    $controllers = $this->getControllers();
+    return $controllers[$controller];

Is there a way how to better describe that there might be subarrays? Singular feels wrong.

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.phpundefined
@@ -98,7 +144,17 @@ class EntityType extends Plugin {
+  public function getLabel($langcode = NULL) {
+    return $this->values['label']->get();
+       $label = NULL;
+    if (isset($this->values['label_callback']) && function_exists($this->values['label_callback'])) {
+      $label = $this->values['label_callback']($this->values['id'], $this, $langcode);
+    }
+    elseif (!empty($this->values['entity_keys']['label']) && isset($this->{$this->values['entity_keys']['label']})) {
+      $label = $this->{$this->values['entity_keys']['label']};
+    }
+    return $label;

No reason for all that stuff. The label of an entity Type is simple, the other code is about the label of an entity.

Tagging.

Some big fixes here and there.

In general I hate that this removes the documentation on the EntityType Annotation again.

Fixed some small points which prevented drupal from being installed.

Note: installing drupal with this feels much slower!

Just some small bits.

The following keys do not have a method yet:

• route_base_path
• bundle_prefix
• 'list path'
• 'entity_cache_test'
• menu_base_path
• menu_edit_path
• menu_view_path

Straight reroll for HEAD changes. Installation of standard profile fails after installing 30 of 40 modules due to Drupal\field_ui\Routing\RouteSubscriber::routes() accessing $entity_info['route_base_path'] (see #12.1).

Started with separating the annotation object from the used object on runtime.

Due to not directly accessing array data anymore, you made a local variable, except the name was overwriting an unrelated variable.

Here's the section from the interdiff in the actual patch now:

-        $entity_tables[$entity_info['base_table']] = drupal_get_schema($entity_info['base_table']);
+        $entity_base_table = $entity_info->getBaseTable();
+        $entity_tables[$entity_base_table] = drupal_get_schema($entity_base_table);

I like this patch for a lot of things -- like making entityInfo a pretty object instead of an unhelpful array -- except the new YAML files.

To summarize a discussion long ago. You can add metadata to a class in apprxoimately three ways

1. static method
   
2. annotations
3. external file, in Drupal 8, this is a YAML file

We decided to scratch 1. because of concerns on memory and no ability to do any sort of static code analysis. We have decided to scratch 3. in order to keep things belonging together in one file, feeling that PSR-0 creates so many files it's already kinda enough without doubling them.

So we went with 2. with a heavy heart because we knew it's far from perfect but the other two had insurmountable problems.

This patch keeps 2. and adds back 3. I would much rather enforce Drupal\{modulename}\Entity\Type\{entity_type} than make people create two files per entity classes.

I do think that entity types are sufficiently special enough for this. They're not plugins, and they are not numbered in the hundreds like plugins can be.

However, I think instead of *.entity.yml, we should have them in *.info.yml nested under a key like entity_type: or entity_types:

To Tim's point, if we keep the YAML file we really ought to start merging them. We now have a services.yml that has only one top-level key, a routing.yml file that is just for routes, an info.yml to register the presence of the module (which is largely redundant with a composer file, although that's a separate matter), and now potentially one for entities.

At minimum, we can merge the services and entity YAML files. The services.yml file would require no syntax changes to do so. I'm not sure that those belong in the info.yml file, but there's an argument to be made there. (Maybe move the composer-redundant parts to composer and then merge what's left with services and entities? Or setup to do that in the future?) Routing is the one I'd more inclined to leave separate because it's the one I expect module devs to spend the most time in, unless modules start adding a lot of services. (And because it might be be a syntax change to do so, right when we're creating them by the dozen.)

services.yml has two top level keys; just noone registers parameters.

Either the YML file or the annotations should be dropped, I said so above. Both are a solution to a problem which has no good solution; picking two bads is worse than one.

It helps if you can save/load nodes to reduce the amount of failures.

Let's introduce a simple method for now.

I would much rather enforce Drupal\{modulename}\Entity\Type\{entity_type} than make people create/edit two files per entity classes.

than make people create/edit two files per entity classes

For all content entity types and half config entity types, we already have 2-6 files per entity class due to all the storage/list/render/form/etc. controllers. That said, let's clarify what the benefits of YAML-based discovery vs. rigid namespace based discovery are for entity types. Are there any? One that I can think of is if it makes our installation (and consequently, testbot runs) faster, since I think we have to rediscover all entity types after every single module_enable(). Any others?

We decided to scratch [static methods] because of concerns on memory

For plugins in general, yes. If we're moving entity types away from being plugins, which I agree with, then a static method might be worth putting back on the table. One advantage of it would be inheritance. For example, 11 of 22 classes that currently extend ConfigEntityBase also set the storage controller to ConfigStorageController, something that could be centralized within ConfigEntityBase::getInfo().

Some more fixes especially one in entity NG which will help to get down the failures a lot.

Re. #33 let me quote this:

    foreach ($this->directories as $directory) {
      $file = $directory . '/' . basename($directory) . '.' . $this->name . '.yml';

there's still filesystem iteration going on...

OK. I said what I needed to say, you guys will act as you wish, there's nothing I can or want to do. Another unfollowed issue, I guess.

Let's fix quite some bugs which come up if you install using the standard profile.

Overall, I like the way this issue is headed, but I find the premise, "Since entity types don't have the same requirements of plugins (there won't be nearly as many of them)" disturbing. Core has more than 10x the number of entity types from 7.x to 8.x. I think that contrib will end up implementing a lot more entities than plugins. Specifically Config entities.

There are ~20 entity types, there are ~400 plugins

>> than make people create/edit two files per entity classes

> For all content entity types and half config entity types, we already have 2-6 files per entity class due to all the storage/list/render/form/etc. controllers.

What I wanted to say is about the entity class *itself*. If you add a YAML file then the metadata/logic belonging to that specific class will now in be two places. This is why we added annotations. (Static methods are very poor static code analysis wise.) Either remove annotations or remove YAML. Having both is silly.

The current implementation is a bit of a wtf. Let's admit this is major. If a module provides an entity I would expect the entity to contain the module's essential business. If this is buried in lib/Drupal/user/Plugin/Core/Entity/User.php we are doing something wrong.

In D7 we used info hooks to define Entity types. This allowed ECK to save info in the db to generate entity types created through the UI. In D8 we first made Entity Types into plugins which initially seemed not to be able to support ECK's use case, but through derivatives it was shown that it was still possible to save entity type definitions in config that could then be exposed as plugins through derivatives. I went through the last patch a couple of times, and it seems to me that for a module like ECK to work in D8 I will have to generate php classes, is that right?

This is a straight reroll, no changes at all. There are new usages of entity info as an array, I didn't have time to fix them.

@fmizzell, hook_entity_info_alter() is still there.

@twistor, thanks, I see that both entity_info and the alter are still there. That is great news :)

Didn't really do a real review just skimmed some parts. noticed this:

+++ b/core/lib/Drupal/Core/AnnotationReader.phpundefined
@@ -0,0 +1,98 @@
+    $this->cache[$name] = $value;

I don't see cache defined as a property on the object.

Missing entry in system.routing.yml and wrong calls in EntityDeriver.

This installs standard again.

hook_entity_info needs stuff by reference, copying the existing pattern.
Also cleaning up some hook_entity_info() implementations.

Found some more bugs.

Okay, I'm finding plenty of stuff, going to work on this tonight.

More fixes.

Just as a lark, here it is with ArrayAccess.

Any answer to #51 ?

If you add a YAML file then the metadata/logic belonging to that specific class will now in be two places.

Have you seen the YAML files?

diff --git a/core/modules/node/node.entity.yml b/core/modules/node/node.entity.yml
new file mode 100644
index 0000000..b1273a8
--- /dev/null
+++ b/core/modules/node/node.entity.yml
@@ -0,0 +1,2 @@
+node: Drupal\node\Plugin\Core\Entity\Node
+node_type: Drupal\node\Plugin\Core\Entity\NodeType

I wouldn't consider that either metadata or logic.

EDIT: And this patch will also let us move the classes so node.entity.yml could say

node: Drupal\node\Node
node_type: Drupal\node\NodeType

It just doesn't do it all at once. That alone is enough for me to want this one step of registration.

Going to stop crowding this issue and upload patches in #2041073: Helper issue for #2005716 (Remove entity dependency on plugins).

Okay, I can't take this any further. I have NFI why the translation UI tests are failing.

Oh, and here it is without ArrayAccess, to see if that makes a difference.

Alright, now I'm done (turns out the ArrayAccess did help!)
All of the failing classes in #76 are subclasses of ContentTranslationTestBase, so there is probably only that one bug left.
That and cleaning up whatever is left of #77, here's some.

From a quick look to the test results, it seems that #1810350: Remove menu_* entity keys and replace them with entity links might help with getting rid of the translation failures.

I can have a look to them it they are still there after that the issue above is in.

So this patch

1. Makes entity classes use a different way for discovery than the rest of core -- which use AnnotatedClassDiscovery -- and use an additional file. Splitting hairs whether the location of the class is metadata or not is just that -- splitting hairs. The rest of core use self-contained classes.
2. The rest of core uses the service container to do the injection while this annotation reader uses some weird setter injection with a hardwired default . Aside from being inconsistent, this sort of injection raises a lot of questions: do I need to call [the setter method]? When do I need to and when I don't? Is there a configuration perhaps that some caller higher up the chain uses? Compare it with the rest of core: the injection always happens, it's always the service container that does the injection and to find what gets injected I can just read the services YAML. In the issue I filed against this I was told "If the default dependency is what you'll need 99% of the time, this is a much nicer method as you don't have to turn something into a service unnecessarily" -- but the problem is you have no clue whatsoever whether it's 99% or 9.99% when the default dependency is used or not.
3. Given that we are past API freeze I am very hard pressed to see why such another big change needs to happen in Drupal 8 especially one that has happened without any of the entity API maintainers even offering an opinion over it (one comment saying "Tagging" is not a discussion).

Given these: Drupal 9.

Note that 1. and 2. are somewhat easily fixable:

1. One can use the AnnotatedClassDiscovery without actually using plugins if we so dislike plugins. Consistency wins.
2. Convert the annotation reader to a service and be done. Did I mention that consistency wins?

As for 3.: some outreach would be good.

I had my say but it is not my duty to move things to D9. I am sorry for holding this patch up. This is the last issue comment outside of entity storage I am making on Drupal 8 for a long time. I have removed the RTBC queue from the Live RSS extension and removed the Drupal core queue from my dashboard. I hope you'll be able to make a better Drupal 8 without me. If you need me for upgrade issues, I am still around.

I haven't reviewed the patch yet, but quickly on this:

the annotation reader just uses reflection. It's quite a bit faster, and the memory overhead is minimal.

I haven't tested this but I think the token-based annotation reading we're using in core is compatible with the optimize comments feature of OpCache (strips comments to reduce cache size), using reflection for annotations certainly won't be.

Discussed at MWDS with @Dries, @msonnabaum, @alexpott, @effulgentsia. There are three major changes in this patch that need to be split off into separate issues:

1. Turning entity type into its own object.
2. Abstracting annotation parsing into its own service.
3. #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity (already split out)

Let's re-scope this issue to #1 only. That particular API change is approved, but let's reduce the disruption caused by this patch by adding a BC layer with ArrayAccess, and mark the existing code deprecated. This issue should convert one implementation, and then subsequent implementations can be converted in subsequent patches.

My only question: Would we be comfortable with shipping 8.0 with the ArrayAccess still in place?

Still need an answer on my last question.

Discussed on the phone with @Dries, @webchick, @msonnabaum, @tim.plunkett, and @alexpott -- this should be considered a beta blocker given the DX impact.

This makes sense to me and is inline with what we recently did for field and data definitions. However, I think the terminology needs work.

+++ b/core/lib/Drupal/Core/Config/Entity/ConfigStorageController.php
@@ -83,18 +85,11 @@ class ConfigStorageController extends EntityStorageControllerBase {
+  public function __construct($entity_type, EntityType $entity_info, ...) {

This line shows the terminology clash of the proposal: what's now the entity type, $entity_type or $entity_info? I guess $entity_type has to stay as the entity type name, everything else would probably be just unnecessary confusing.
However, EntityType $entity_info maps to our use of the term "definition", it's the definition of the entity type just as @FieldType is the plugin definition of the field type. So what about using EntityTypeDefinition $entity_definition ?

Alltogether, even if entities wouldn't play into plugins at all, this really moves us towards classed plugin definitions. So it would be probably a good idea to at least loosen DiscoveryInterface to allow for mixed plugin definitions, such that classed based plugin definitions are doable later also.

Note that @timplunkett is working on a re-roll in #2160315: Patch testing for 2005716.

Agree with the terminology problem, what about just EntityDefinition? It's FieldDefinition and not FieldTypeDefinition...

Edit: Ah, but FieldDefinition is the definition of an actual field and not a field type, while this is the definition of the entity type... not sure.

Yeah, EntityDefinition would be misleading IMO.

We do have "field type definitions", they are array-based plugin definitions.
They are different from the FieldDefinition of each specific field ("how is 'title' different than 'body'")

What we are talking about here are "entity type definitions", much more akin to the former than to the latter. Those are not the definition of a specific entity ("how is entity 1 different from entity 2"). It's the definition of the entity type, i.e what all those entities have in common.

Yeah :)

Another thing that's a bit problematic is that we have config and content entities with different keys that are only relevant to them, keys that only make sense for a given situation, e.g. a lot of database related keys that only apply if you use the default storage controller (we discussed before that this should be configuration of the storage controller and therefore below or related to but that makes the annotation more complicated) and we have to support that contrib can add additional keys there as well IMHO.

So having methods for the common stuff is nice, but we need to support additional things there too and might not want to add method for anything that's there right now (e.g. things that are specific to a given type of entity types, like config stuff or database table...)

I'm working on this patch, I will address the recent flurry of discussion soon.

This still needs work (mostly docs), but should pass as of right now, no ArrayAccess needed.
I will explain more tomorrow.

+  $entities = array_filter(\Drupal::entityManager()->getDefinitions(), function($entity_info) use ($name) {
+    /** @var $entity_info \Drupal\Core\Entity\EntityTypeInterface */

Weird. Why didn't you typehint?

-  $entity_info = entity_get_info($entity_type);
+  $entity_info = \Drupal::entityManager()->getDefinition($entity_type);

Ouch but that's the D8 way, I guess. I would, however, really love (perhaps a followup) \Drupal::entityManager($entity_type)->getDefinition() -- consider that outside of the entity manager nothing at all is cross entity type, it's truly weird to have this entity manager thing which is "above all" of them.

+  protected $bundle_of;

Random nitpick: I thought we use camelCase ( I am not sure I like that convention but I think that's the coding standard ).

-      'route_name' => $entity_info['links']['drupal:content-translation-overview'],
+      'route_name' => $this->entity->entityInfo()->getLinkTemplate('drupal:content-translation-overview')

Very nice (as is the rest of the patch but this deserves mention)

I forgot you could typehint anonymous functions, duh. Will fix in the next patch.

See #2144677: Add per entity type managers to improve DX for the issue about entity managers

The $bundle_of is populated by an annotation, and we are still using lower_snake_case for those.

Thanks!

I fixed the typehinting, and a couple docblocks.

I added "@todo." to places that need docblocks, I don't have time today to fix it myself.

If someone chips in to help, *please* post an interdiff, as I will apply that to my sandbox for this.

101: entity-info-object-2005716-101.patch queued for re-testing.

101: entity-info-object-2005716-101.patch queued for re-testing.

Okay, this should be it.
I agree with @msonnabaum, we don't need to pass the string and the object around.
If others agree, I should fix that now, as we're breaking the interface of EntityControllerInterface already.

#2119905: Provide @ConfigEntityType and @ContentEntityType to have better defaults will still work fine after this.

The only remaining question is whether the \Drupal\Core\Entity\Annotation\EntityType class should continue to delegate to another class, or just merge the two and have it return $this. It wouldn't be a big change to fix now, but it might later.

Removing the needs profiling tag as that was when I was asking about leaving ArrayAccess in place, that's not happening anymore.

Went ahead and made the change to EntityControllerInterface, might as well make both API changes at once.

I'm going to punt on the combining of \Drupal\Core\Entity\EntityType and \Drupal\Core\Entity\Annotation\EntityType. That can be done later with no API change, and should probably wait for the @ContentEntityType issue.

This should still pass unless I made a silly mistake.

In which case, this is ready. It won't stay green for long...

It helps to run the unit tests locally.

No more patches after midnight (or after that many Christmas cookies).

Hopefully last one.

+1 on avoiding the duplicated variable for the entity type name. Given the following two examples, I still think EntityTypeDefinitionInterface $definition would be the best we can do now.

+  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_info) {

is it a type or info?

+ $entity_info = \Drupal::entityManager()->getDefinition($entity_type);

is it a definition or info?

There's also things like EntityInterface::entityInfo() and EntityStorageControllerInterface::entityInfo() to worry about. I really think that while switching to $entity_type_name for the string everywhere would be best, it'd make for a much bigger patch than we have already (almost 350K!).

I requeued the patch, it had only the same fail as when HEAD was broken, so we should be back to green now.

Updated the issue summary.

Here is my summary of the last few comments.

Historically, the results of entity_get_info() were stored in a variable called $entity_info. This ignored whether it was entity_get_info($entity_type) or entity_get_info(), because both were an array, even though one was an entity type's info, and the other was an array of all of the entity types and their info (yay for info being a fake word with no plural form).

Since we had no object representing an entity type, the variable $entity_type always referred to the name of the entity type, and should have been called $entity_type_name.
$entity_type should now refer only to the object, but that would make this patch *really* massive, and might be too late for D8.

Recently, because of entity_get_info() being split into ->getDefinitions() and ->getDefinition(), the variable $definition has cropped up.

So now after this patch, we have 3 different names commonly used to refer to the entity type object: $entity_info, $definition, and $info.
We would ideally like to call it $entity_type, but that is taken by what should be $entity_type_name.
A compromise was proposed, to call it $entity_type_definition, but not only is that a semantic cop-out, it is long, easy to misspell, and possibly confusing compared to FieldDefinition.

My proposal is to leave the naming as is, get this 350K API-breaking monster in, and consider the renaming in a follow-up.

My proposal is to leave the naming as is, get this 350K API-breaking monster in, and consider the renaming in a follow-up.

This seems sensible to me. One change at a time. We can evaluate how confusing the lack of standardization is once we can see the big picture and determine at that point whether that's a beta blocker, a beta target, or a "dang, D9" type of clean-up.

Added code example. This has no remaining tasks now that we're not renaming the variables here.

I did a final review myself, and noticed a couple docblock inconsistencies.

Reuploading because the bot won't queue the last one.

Any chance to have #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation committed before this goes in? ;)

That one is "normal" and not tagged "Avoid commit conflicts," this one is "critical" and is. Can you explain why we should postpone a critical task for a normal one?

This still leaves that code as a simple todo, I will personally reroll that after this goes in.

I wasn't worried about the reroll (it should be quite easy on both sides), but this issue is not RTBC yet and I tought the "Avoid commit conflicts" tag applied only to RTBC ones.

Personally I think the other one should be major: the more we wait for it to be committed, the higher are the chances that people will document entity type properties the wrong way.

#2067079: Remove the Field Language API removed a lot of code touched by this, hopefully it didn't add anything else important?
Rerolled.

A couple of use statements that are now unnecessary and node_test_entity_info_alter() had been changed in a way all the other implementations of hook_entity_info_alter() had not.

+++ b/core/modules/node/tests/modules/node_test/node_test.module
@@ -169,8 +169,9 @@ function node_test_node_insert(EntityInterface $node) {
-function node_test_entity_info_alter(&$entity_info) {
+function node_test_entity_info_alter(array $entity_info) {
+  /** @var $entity_info \Drupal\Core\Entity\EntityTypeInterface[] */
   if (\Drupal::state()->get('node_test.storage_controller')) {
-    $entity_info['node']['class'] = 'Drupal\node_test\NodeTest';
+    $entity_info['node']->setClass('Drupal\node_test\NodeTest');
   }
 }

This change is interesting because not passing in $entity_info by reference means that changing the class is not actually occurring. If you "fix" this then Drupal\node\Tests\NodeAdminTest fails.

Also as a result of the changes here we can remove two use statements.

diff --git a/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
index 411c091..d8d3297 100644
--- a/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
+++ b/core/modules/config_translation/lib/Drupal/config_translation/ConfigEntityMapper.php
@@ -7,7 +7,6 @@

 namespace Drupal\config_translation;

-use Drupal\Component\Utility\Unicode;
 use Drupal\Core\Config\ConfigFactory;
 use Drupal\Core\Entity\EntityInterface;
 use Drupal\Core\Entity\EntityManagerInterface;
diff --git a/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationOverviewTest.php b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationOverviewTest.php
index e5a80ef..a471ce9 100644
--- a/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationOverviewTest.php
+++ b/core/modules/config_translation/lib/Drupal/config_translation/Tests/ConfigTranslationOverviewTest.php
@@ -8,7 +8,6 @@
 namespace Drupal\config_translation\Tests;

 use Drupal\Component\Utility\String;
-use Drupal\Component\Utility\Unicode;
 use Drupal\Core\Language\Language;
 use Drupal\simpletest\WebTestBase;

I was going to submit a patch but since the change to node_test_entity_info_alter() breaks stuff I'll add the interdiff and sets to needs work.

I so wanted to rtbc this monster.

Discussed with @Berdir on IRC - he could not repeat the fail I'm experiencing with the Drupal\node\Tests\NodeAdminTest. So I'm uploading the patch I would have for #128 to see what testbot says.

However, @Berdir also mentioned that with #2145103: Provide non-configurable field types for entity created, changed and timestamp fields the test will not be possible so maybe in light of that we should just do what I've done on the #129 patch also attached :)

+1 for 129. The & is optional of course, but it is consistent. But I like the test cleanup more.

So considering my changes in#128 and #129 are so small let's get this done. RtbC for https://drupal.org/files/issues/2005716.129.patch

Dreditor crashed, so here is a short code-quote review.

+++ b/core/lib/Drupal/Component/Annotation/AnnotationBase.php
+++ b/core/lib/Drupal/Component/Annotation/AnnotationInterface.php

We cannot add plugin specific interface/code to the generic annotation namespace. If you look at AnnotationInterface it does not talk about plugins.

+++ b/core/lib/Drupal/Core/Entity/Annotation/EntityType.php
+    return new $class($values);
+  }

+1 for the separation.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.php

+  /**
+   * {@inheritdoc}
+   */
+  public function getDefinition($entity_type_name) {
+    return parent::getDefinition($entity_type_name);
+  }
+
+  /**
+   * {@inheritdoc}
+   */
+  public function getDefinitions() {
+    return parent::getDefinitions();
+  }

Leftover debugging code.

+  /**
+   * Indicates if the entity type is a subclass of the given class or interface.
+   *
+   * @param string $class
+   *   The class or interface to check.
+   *
+   * @return bool
+   *   TRUE if the entity type is a subclass of the class or interface.
+   */
+  public function isSubclassOf($class);

It is odd to wrap an existing php function or is this just for mocking?
For that maybe have a look at this pretty impressive trick: http://marcelog.github.io/articles/php_mock_global_functions_for_unit_te...

@@ -191,13 +191,11 @@ function content_translation_add_page(EntityInterface $entity, Language $source
   $target = !empty($target) ? $target : language(Language::TYPE_CONTENT);
   // @todo Exploit the upcoming hook_entity_prepare() when available.
   content_translation_prepare_translation($entity, $source, $target);
-  $info = $entity->entityInfo();
-  $operation = isset($info['default_operation']) ? $info['default_operation'] : 'default';
   $form_state['langcode'] = $target->id;
   $form_state['content_translation']['source'] = $source;
   $form_state['content_translation']['target'] = $target;
   $form_state['content_translation']['translation_form'] = !$entity->access('update');
-  return \Drupal::entityManager()->getForm($entity, $operation, $form_state);
+  return \Drupal::entityManager()->getForm($entity, 'default', $form_state);

 /**
@@ -216,11 +214,9 @@ function content_translation_add_page(EntityInterface $entity, Language $source
  */
 function content_translation_edit_page(EntityInterface $entity, Language $language = NULL) {
   $language = !empty($language) ? $language : language(Language::TYPE_CONTENT);
-  $info = $entity->entityInfo();
-  $operation = isset($info['default_operation']) ? $info['default_operation'] : 'default';
   $form_state['langcode'] = $language->id;
   $form_state['content_translation']['translation_form'] = TRUE;
-  return \Drupal::entityManager()->getForm($entity, $operation, $form_state);
+  return \Drupal::entityManager()->getForm($entity, 'default', $form_state);

This change looks a bit odd to be honest.

+  public function getLinkTemplates();

At least from the perspective of current HEAD this seems problematic
.

   /**
    * {@inheritdoc}
+   * @param \Symfony\Component\DependencyInjection\ContainerInterface $container
+   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_info
+   *
+   * @return static
    */
-  public static function createInstance(ContainerInterface $container, $entity_type, array $entity_info) {
+  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_info) {

Don't we just have an interface?

-    $this->assertTrue(TRUE == entity_get_info('entity_test'));
+    $this->assertTrue(TRUE == $entity_manager->getDefinition('entity_test'));

Man, drupal is quite awesome!

Fixed the docs in AnnotationInterface, good point.

Removed the redundant parent:: calls.

I added isSubclassOf because while most of the code used is_subclass_of(), some also used class_implements(), which was confusing. It also saves the direct access to getClass(), and is a nice and common need.

The default_operation stuff was snuck in by some other issue, it was supposed to be related to #2006348: Remove default/fallback entity form operation, but that never happened.

getLinkTemplates() maps to what is documented and done in HEAD. I know that's under discussion, but this is just a conversion.

Fixed the docs that were added by PHPStorm.

Because this was only docs changes, leaving at RTBC.

#2163035: Remove $user->theme and Drupal\user\Theme\UserNegotiator removed a file I changed. No changes to the patch otherwise.

I just want to mention that I'm generally really ++ to the plugin changes in this patch. Tim and I have discussed it pretty extensively, and I think there are some fairly minor follow ups to do, but I really like where this is going.

Eclipse

Said follow-up is #2164083: Improve AnnotationBase to be more useful and encapsulate best practices. Clarified the plugin parts @EclipseGC is talking about in the API changes section.

/me still don't get why an annotation always has an ID.

1. -class Translation implements AnnotationInterface {
   +class Translation extends AnnotationBase {
   

   This feels wrong as Translation is not being used for "top-level" annotations. A translation in an annotation does not have a provider itself or a class.

2. -      $type = $this->t('Simple configuration');
   +      $type = $this->t('simple configuration');
   

   I don't really understand this change. In any case it's completely unrelated as far as I see.

3. In my opinion the isSubclassOf() is a bit strange and the few keystrokes that you save vs. is_subclass_of() does not make it worth it. I don't feel strongly about that, though.
4. I think the generic get() and set() methods on EntityTypeInterface are strange, in my opinion. I'm not sure whether that's temporary and we want to move all the get() and set() calls to proper getFoo() and setFoo() methods in the long run or whether there is any specific need for get() and set().
5. From the discussion here in the issue I would have expected to find a EntityTypeDefinitionInterface instead of EntityTypeInterface. Can someone clarify the current thinking? Thanks.

6.  function content_translation_edit_page(EntityInterface $entity, Language $language = NULL) {
      $language = !empty($language) ? $language : language(Language::TYPE_CONTENT);
   -  $info = $entity->entityInfo();
   -  $operation = isset($info['default_operation']) ? $info['default_operation'] : 'default';
      $form_state['langcode'] = $language->id;
      $form_state['content_translation']['translation_form'] = TRUE;
   -  return \Drupal::entityManager()->getForm($entity, $operation, $form_state);
   +  return \Drupal::entityManager()->getForm($entity, 'default', $form_state);
    }
   

   This seems unrelated. #133 seems to hint that it should be or was removed from the patch but it's still in #135

7. +  protected function setUpEntityType($definition) {
   +    return new EntityType($definition);
   +  }
   

   This is incredibly minor, but I don't see the benefit of having that as a separate method.

8. I agree with #138 but maybe that can be re-hashed in #2164083: Improve AnnotationBase to be more useful and encapsulate best practices.

All of those are not very substantial so leaving at RTBC.

1) It's still an annotation, no problem here.
2) Check the surrounding context, I'm removing a strtolower later. We've done stuff like this in other issues.
3) isSubclassOf() replaces usages of both is_subclass_of and class_implements, and was confusing. It's just a nice to have.
4) We absolutely need this for non-standard properties. See #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation
5) This class represents the entity type. The actual real entity type, not the definition of one (that's the annotation itself). We agreed to use the correct naming.
6) My comment in #133 was to explain why its still desired. It should never have been added to core.
7) This is a phpunit test helper, it doesn't hurt anything, and it might get more extensive with more tests later.
8) I'm fine with discussing that more, but the AnnotatedClassDiscovery already assumes that it has an ID in HEAD, I'm just codifying that in a method.

EDIT: To clarify my last point, and to better answer @dawehner's question, https://github.com/drupal/drupal/blob/8.x/core/lib/Drupal/Component/Anno... is already happening, if we want to not do that in the future, it is unrelated to this issue.

Thanks for the quick answers. Here are some further replies:
1. It's not a problem that's true. I still think it's wrong, though. What is 'provider' of a @Translation annotation? Can you clarify?
2. I've looked at the context, and I see the strtolower(), but I don't see it being removed in the patch, maybe that got lost at some point? (I only looked at #135)
4. So does that mean after #1968970: Standardize module-provided entity info documentation and clean-up the @EntityType annotation this can be removed? Not really sure what "non-standard properties" are in a general sense.
6. OK, I see. Still a bit unfortunate to include the reverting of that in here, which is seemingly unrelated, but OK...

Still RTBC.

I really wanted to commit this, but unfortunately my entity/field API knowledge isn't up to snuff. Or at least, reading through EntityTypeInterface and EntityType, I end up with loads of questions like:

1. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -0,0 +1,537 @@
   +   * Indicates whether entities should be statically cached.
   ...
   +   * Indicates whether the rendered output of entities should be cached.
   ...
   +   * Indicates if the persistent cache of field data should be used.
   

   This would be a good idea to set when?

2. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -0,0 +1,537 @@
   +   * An array of entity keys.
   

   Yes, I can see that by the variable name. ;) What's an entity key?

3. +++ b/core/lib/Drupal/Core/Entity/EntityType.php
   @@ -0,0 +1,537 @@
   +   * The name of the provider of this entity type.
   

   What is a provider?

4. Ugh, bundles...

...etc. I don't actually know that this is a docs gate issue, per se, since I think the docs probably make sense to those who already know the entity/field/plugin system well. It's probably more follow-up material to expand the docs of these classes to be more explanatory of the entire concept of entities for newcomers because these are the #1 and #2 place, respectively, that new developers will look for that information, IMO.

Given that, I think I'd rather either leave this for one of the other core maintainers, or else postpone on an IRC session where I can get answers to my menial questions about this patch. I'll be around a lot tomorrow, so am happy to make the latter happen if anyone is willing to endure dumb questions. ;) Else, even though alexpott touched the patch slightly, he didn't do the bulk of authoring it and obviously knows it well, so I'd be totally comfortable with him committing it.

5) This class represents the entity type. The actual real entity type, not the definition of one (that's the annotation itself). We agreed to use the correct naming.

Did we? Not to my knowledge.

Definition or not, the current patch has unresolved terminology issues and makes the existing terminology issues ($info vs $definition) far worse by adding confusion about $entity_type vs $info/defintion. Therefore, I don't think it makes sense to move on with this as it is.

This is a 350K API-breaking patch. Changing the naming of local variables would make this patch insanely large, and is not API-breaking.
I ran this by the committers and @webchick agreed in #117.

Still there is no agreement on the naming to be used. Committing a patch that introduces *additional* naming confusion that late in the cycle is a nogo as we cannot risk ending up that way.

Yes the patch is enormous, but that's not a good reason to rush through and pretend decisions are made where they aren't. Usually, one would ensure terminology questions are sorted out *before* rolling such a monster.

pretend decisions are made where they aren't

Usually when I present both sides of a discussion to a committer, and then they make a decision, that is a decision that is made. I don't know what else to call that. Reread #116 and #117 again please.

The naming confusion exists in HEAD, we've just ignored it. $entity_info can mean two completely different things, and is interchangeably used with $info, and half of its meaning is also sometimes called $definition.

This patch does not change that.

This patch is more of a lift and shift and a conversion from an ArrayPI to an OOAPI :)

1. The existing docs have been maintained. I'm not going to argue that there is not massive scope for improvement - but this patch makes it a whole load easy to use entity types than before because of all the methods to access properties that previously were just array keys.

   In fact the existing docs have been improved:

     /**
      * Boolean indicating whether entities should be statically cached during a page request.
      *
      * @todo This is only used by \Drupal\Core\Entity\DatabaseStorageController.
      *
      * @var bool (optional)
      */
     public $static_cache = TRUE;
   

   Above code is current HEAD - the @todo here is not true and the text "during a page request" is also misleading since running an entity_load through the CLI would also use static caching if set to true.

2. Yep this description could and should be improved.
3. Provider is the thing that provides the entity type - this is not module because core can also provide entity types. We are standardising on provider in many places cf the routing system.
4. Ugh.

Looking deeper into @webchick's point 2 I think we've got an area of potential confusion in the patch:

  /**
   * Gets any arbitrary key.
   *
   * @param string $key
   *   The key to retrieve.
   *
   * @return mixed
   *   The value for that key, or NULL if the key did not exist.
   */
  public function get($key);

  /**
   * Sets a value to an arbitrary key.
   *
   * @param string $key
   *   The key to use for the value.
   * @param mixed $value
   *   The value to set.
   *
   * @return static
   */
  public function set($key, $value);

  /**
   * Indicates if a given entity key exists.
   *
   * @param string $key
   *   The name of the entity key to check.
   *
   * @return bool
   *   TRUE if a given entity key exists, FALSE otherwise.
   */
  public function hasKey($key);

  /**
   * Returns a specific entity key.
   *
   * @param string $key
   *   The name of the entity key to return.
   *
   * @return string|bool
   *   The entity key, or FALSE if it does not exist.
   *
   * @see self::getKeys()
   */
  public function getKey($key);

  /**
   * Returns an array of entity keys.
   *
   * @return array
   *   An array describing how the Field API can extract certain information
   *   from objects of this entity type:
   *   - id: The name of the property that contains the primary ID of the
   *     entity. Every entity object passed to the Field API must have this
   *     property and its value must be numeric.
   *   - revision: (optional) The name of the property that contains the
   *     revision ID of the entity. The Field API assumes that all revision IDs
   *     are unique across all entities of a type. This entry can be omitted if
   *     the entities of this type are not versionable.
   *   - bundle: (optional) The name of the property that contains the bundle
   *     name for the entity. The bundle name defines which set of fields are
   *     attached to the entity (e.g. what nodes call "content type"). This
   *     entry can be omitted if this entity type exposes a single bundle (such
   *     that all entities have the same collection of fields). The name of this
   *     single bundle will be the same as the entity type.
   *   - label: The name of the property that contains the entity label. For
   *     example, if the entity's label is located in $entity->subject, then
   *     'subject' should be specified here. If complex logic is required to
   *     build the label, a 'label_callback' should be defined instead (see the
   *     $label_callback block above for details).
   *   - uuid (optional): The name of the property that contains the universally
   *     unique identifier of the entity, which is used to distinctly identify
   *     an entity across different systems.
   */
  public function getKeys();

So get() and set() refer to arbitrary keys which are not actually keys at all but they are properties on the entity type object. And hasKey(), getKey() and getKeys() all refer to checking if the entityKeys property which is an array populated by the annotation. This array contains the keys used by the entity often for look ups eg id, uuid, bundle, revision.

The attached patch:

• Refactors the interface to make the order an bit simpler to understand. Putting getKeys() before hasKey() and getKey() gives a good explanation of what's going to be in the entityKeys property. Interestingly this is now the same order as in the EntityType class.
• Changed the $key parameter to be $property in get() and set() so that the word key is not overloaded in the interface.

With regards to the naming confusion, the result of entity_get_info() and entity_get_info($type) in the past was an array in both cases. The return value is often assigned to a variable called $entity_info. I'm in agreement with @tim.plunkett. This patch does not change the current situation - making it neither worse not better. Resolving this is, imo, beyond the scope of this patch.

As pointed out in #91 the additional confusion introduced by this patch is that it makes unclear what $entity_type is, so far $entity_type refers to the entity type name, but now we also have:
EntityType $entity_info

This breaks with $entity_type being a string, so e.g. $entity->entityType() does not return that class. That's the *new* confusion introduced by this patch given the current terminology.

Usually when I present both sides of a discussion to a committer, and then they make a decision, that is a decision that is made. I don't know what else to call that. Reread #116 and #117 again please.

As quoted in #143 you were pretending there is an agreement on the naming, but there isn't.

Ad #117: If decisions like that are made without consulting and/or discussing it with the respective sub-system maintainers I've nothing more to comment.

Yes, $entity_info/$info/$definition/getDefinition() and so on is already inconsistent. However, so far, the difference between those and $entity_type is fairly clear, the latter always being the entity type name and a string.

Now, after applying the patch, that is no longer the case. The following snippet from EntityStorageControllerBase shows that nicely:

  /**
   * Entity type for this controller instance.
   *
   * @var string
   */
  protected $entityType;

  /**
   * Array of information about the entity.
   *
   * @var \Drupal\Core\Entity\EntityTypeInterface
   */
  protected $entityInfo;

.....
  /**
   * Constructs an EntityStorageControllerBase instance.
   *
   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_info
   *   The entity info for the entity type.
   */
  public function __construct(EntityTypeInterface $entity_info) {
    $this->entityType = $entity_info->getId();
    $this->entityInfo = $entity_info;
    // Check if the entity type supports static caching of loaded entities.
    $this->cache = $this->entityInfo->isStaticallyCacheable();
  }

Apart from the obvious "Array of... " problem, this seems quite confusing. We have entityType and entityInfo and entityInfo is an EntityType object? I don't know how I'd could explain this to someone.

And yes, while we could relatively easily rename the class in PHPStorm in a follow-up, that will result in another huge patch and cleaning up the related docblocks and references will likely not be as easy.

On the other side, I'm not sure if we can find an agreement in the issue queue for a better name. Both @fago and myself are in favor of something with Definition to go along with FieldDefinition (and I think @yched too). Not sure if we still plan on combining the two EntityType classes together, that would be an argument against Definition as you don't want to write @EntityTypeDefinition

So yes, this has a committer decision in #117, but it also has a subsystem maintainer that is against it and another one that is unsure/not convinced. I could be convinced about the name I think (see above), but the current state of the patch with snippets like the one above is problematic.

Another thing that I'm not sure about is enforcing seldom used and storage/controller specific keys like the caching flags and table names in the interface, as mentioned before.

Here's a thought:

If we're going to commit this with a follow-up to discuss the name of this and will consider to rename/make it consistent, what about using EntityInfoInterface for the time being? While w're already quite sure that this is not want we want in the end, we would at least not introduce yet another new term for the same thing, the above snippet would make way more sense and it would be more consistent with 90% of the current code base, that *does* use entity info.

Personally, I'd much rather rename local variables than interfaces. Interfaces are the "gateway" into how a subsystem works, and when we rename one we a) force everyone to relearn a new place to look for that information, b) break existing URLs to relevant api.d.o pages, etc. c) break modules that are in the process of porting to D8. While we're not "API complete" yet in D8, we should not at this point be introducing *more* "beta blocker" issues we're forced to clean up in order to become API complete. (Versus local variables, which could be renamed at any time, since it's not an API break.)

My opinion is that yeah, that code snippet is confusing, but I think there are a lot of other aspects of local variable naming within the entity system that are also confusing, and so I'd prefer to handle those more holistically once we know what the API actually looks like.

This breaks with $entity_type being a string, so e.g. $entity->entityType() does not return that class. That's the *new* confusion introduced by this patch given the current terminology.

@fago this is a good point - the question in my mind is should we address this here or in a critical beta blocking follow up. Given @Berdir's comment #150 I think we need to step back here.

In my initial review of the patch I considered commenting on the entityType property on the Entity class being unnecessary since we have an entityType() method. Perhaps we could:

• Remove the entityInfo property
• Make the entityType property the actual entity type object
• Rename the entityType() method entityTypeId()
• Replace all usages of the entityType property with the new entityTypeId() method or $this->entityType->id()

Or we do what @Berdir suggests.

147: 2005716.143.patch queued for re-testing.

I've just had a look into the amount of work it'll take to replace all the entityType properties and methods that are assumed to be a string with something like what I suggest in #153. The following snippet from EntityInterface indicates the size of the challenge.

  /**
   * Returns the type of the entity.
   *
   * @return
   *   The type of the entity.
   */
  public function entityType();

  /**
   * Returns the bundle of the entity.
   *
   * @return
   *   The bundle of the entity. Defaults to the entity type if the entity type
   *   does not make use of different bundles.
   */
  public function bundle();

@fago and @tim.plunkett how do feel about @Berdir's suggestion in #151?

#151 works for me.

@msonnabaum I think we all in agreement with that a property called entityType that is just a string should not exist anymore. However, have you looked at the amount of code that will have to change to get that done?

I think the confusion is that we are assigning an EntityType object to a property called entityInfo and there is an entityType property which currently is just the string representing the ID. I agree with @fago and @Berdir - this is a new confusion brought about by this patch

Given that sorting out entityInfo and entityType properties and methods is going to add swathes of noise to this already large patch I think we need to accept that this patch represents a halfway house. The discussion should be what is the least confusing. EntityInfoInterface or EntityTypeInterface or something else?

I personally think we are likely to end up with an EntityTypeInterface which is pretty similar to what we can see in the patch #147. Therefore I think we should commit that patch and move forward with making the usage of entityType consistent in critical followups - i.e. always represent the entity type object. I agree with @webchick's argument in #152 adding yet another temporarily named interface is going to be more confusing in the long run.

Also fine with a critical follow-up for the local variable renaming fwiw.

I think the confusion is that we are assigning an EntityType object to a property called entityInfo and there is an entityType property which currently is just the string representing the ID. I agree with @fago and @Berdir - this is a new confusion brought about by this patch

Isn't that the same confusion HEAD already has with $node_type? E.g., comment_update_8006() uses $node_type as a string, whereas most other HEAD code uses $node_type as an instance of NodeType. Meanwhile, $node->getType() returns a string.

@catch the thing is the follow ups will not just be local variables it will also be changing the return value of the entityType() method and deprecating entityInfo() so there will be massive change.

To expand on #163, I think that EntityType is the correct class name, in part because we already have NodeType as a class name. I think $entity_info is a terrible name; would it help at all for this patch to rename that and all flavors of it to $entity_type_info (or if _info needs to be reserved to arrays only, then $entity_type_object or $entity_type_instance)? The followup could then deal with stripping that suffix away.

I agree with @webchick and others that misnaming the interface on purpose is the wrong decision.
I also recognize that these entityInfo/entityType methods do create additional confusion.

However, the only reason this issue has stayed green this long is because extremely few patches have been committed due to Christmas. I really don't think this will continue, and expanding it to all of those other calls will certainly not work as one whole patch.

I opened #2164827: Rename the entityInfo() and entityType() methods on EntityInterface and EntityStorageControllerInterface, and postponed it. I'd be happy to work on that once this is in...

I agree with @webchick's argument in #152 adding yet another temporarily named interface is going to be more confusing in the long run.

I can follow that reasoning, but that would require a discussion and agreement on that interfaces first for me. However that has been choked of here in favour of getting this big patch in asap :-(

ad #163: Meanwhile, $node->getType() returns a string.

I agree that it should return the object, not sure we have an issue for that? But having the problem elsewhere is not a good reason for repeating it everywhere imo.

Not sure if we still plan on combining the two EntityType classes together, that would be an argument against Definition as you don't want to write @EntityTypeDefinition

Having two EntityType classes is actually confusing, any chance to fix that?

Howsoever, I think having one EntityType and one EntityTypeDefinition class might be even more confusing as it would tell me that the definition object defines the non-definition object, but that's not the case here.

Taking the analogy to e.g. field formatters, we right now have:

/**
 * Plugin implementation of the 'text_default' formatter.
 *
 * @FieldFormatter(
 *   id = "text_default",
 *   label = @Translation("Default"),
 *   field_types = {
 *     "text",
 *     "text_long",
 *     "text_with_summary"
 *   },
 *   edit = {
 *     "editor" = "plain_text"
 *   }
 * )
 */
class TextDefaultFormatter extends FormatterBase {

In this example the definition cannot simply be "FieldFormatter", as the formatters is the actually instance of the TextDefaultFormatter class. It would have to be a FieldFormatterType or - as the plugin manager interface would suggest - some sort of definition, i.e. FieldFormatterTypeDefinition.
Imo it cannot be FieldFormatterDefinition as it's metadata about the class, not about the instance - this distinction becomes in particular important when take analogy to fields where we'd have FieldType(Definitions) describing the class and FieldDefinitions describing the instances.

Following the reasoning, it would have to be EntityTypeDefinition or EntityType. EntityTypeDefinition somehow suggests the object describes a different $entity_type object though, what's not the case - so I agree that we could save the "Definition" suffix and model it as the actual {foo}Type.

What remains awkward then though, is that the plugin system still forces the definition terminology on us - so EntityType, or FieldType becomes a definition via plugin interfaces still. :-/ Any thoughts on that?

I guess it's not on the table to apply the same change as to entity types to all plugin types, but is this at least supposed to become the new pattern for plugins then?

If so, that would work for me - but I'd like to hear at least berdir's thoughts on it before we move on.

I opened #2164827: Rename the entityInfo() and entityType() methods on EntityInterface and EntityStorageControllerInterface , and postponed it. I'd be happy to work on that once this is in...

Given the follow-up is critical beta blocker like this one, this would work for me. Since we've a new naming now, we should file another follow-up to rename the local EntityType $entity_info parameters to EntityType $entity_type then also.

Here a review of the actual patch:

1. +  /**
   +   * {@inheritdoc}
   +   */
   +  public function getId() {
   +    return $this->definition['id'];
   +  }

   We've $entity->id() else. As alex in #153 I'd have assumed $entityType->id(), can we align that?
   As this is part of the interface and new code, I guess it applies to the avoid-temporary interface changes rule and would need fixing before commit then.

2. @@ -115,12 +116,12 @@ public function getDefinitions() {
                  $finder = MockFileFinder::create($fileinfo->getPathName());
                  $parser = new StaticReflectionParser($class, $finder, TRUE);
   
   +              /** @var $annotation \Drupal\Component\Annotation\AnnotationInter
   face */

   Left over comment, or do we start keeping them in our codebase now?

3. -  $entity_info['action']['links']['edit-form'] = 'action.admin_configure';
   +  /** @var $entity_info \Drupal\Core\Entity\EntityTypeInterface[] */
   +  $entity_info['action']
   +    ->setForm('add', 'Drupal\action\ActionAddFormController')

   here as well

4.   * Implements hook_entity_info_alter().
     */
    function config_test_entity_info_alter(&$entity_info) {
   +  /** @var $entity_info \Drupal\Core\Entity\EntityTypeInterface[] */
   

   Here and in lots of other entity_info[_alter]() implementations again - I guess the hook should have a type-hint instead?

5. +   * The class used to represent the entity type.
       *
   -   * This is not provided manually, it will be added by the discovery mechanism
   .
   +   * It must implement \Drupal\Core\Entity\EntityTypeInterfaceInterface.

   interfaceinterface

so EntityType, or FieldType becomes a definition via plugin interfaces still. :-/ Any thoughts on that?

It doesn't bother me that $entity_manager->getDefinitions() returns an array of objects of EntityTypeInteface, no Definition suffix.

I guess it's not on the table to apply the same change as to entity types to all plugin types, but is this at least supposed to become the new pattern for plugins then?

I wasn't going to go around filing issues to rewrite how all core plugins work, but I definitely plan to write them this way in contrib.

Given the follow-up is critical beta blocker like this one, this would work for me

Yay! I will file the other follow-up.

For the code review:

1) Yes, @Berdir already pointed this out in my first follow-up, I'll just bite the bullet and switch to id() now.

2)
3)
4) We use this doc standard when typehinting is not available at the language level. The best we could do for our entity_info_alters is typehint with array...

5) Fixed here, and found another one.

Yes, I think I can live with this then as well with the mentioned follow-ups, which should also include documentation fixes for those methods and properties where it won't happen already in here (e.g. the Array of docblocks for entityInfo) and in general make sense in the general naming in the main interfaces (EntityInterface, EntityControllerInterface).

4) PhpStorm 7 or 7.1 (don't remember which) improved support for drupal/hooks, and is able to identify hook implementations, it doesn't support 8.x yet and it also doesn't understand hook argument types yet, but we could try to open a feature request for that, would be great as it's becoming more useful now. I don't like inline @var's either, they look ugly to me, but not a show-stopper ;)

Seems like we have consensus, anyone want to reRTBC?

Opened #2165155: Change $entity_type to $entity_type_id and $entity_info to $entity_type/$entity_types

Oops, bad search/replace earlier.

#2107531: Improve DX of local task YAML definitions and #2162271: Prefixes in config_get_storage_names_with_prefix broke this...

173: entity-info-object-2005716-173.patch queued for re-testing.

Ok, back to RTBC then, probably easier to fix the documentation problems in the follow-up. Going to be a lot of work to clean up after this :)

Committed 814aed2 and pushed to 8.x. Thanks!

Anyone can write this, I might have time soon but I don't want to discourage anyone.

For whoever writes this change notice, #2168333: Ensure custom EntityType controllers can be provided by modules changed some of the method names here (mostly adding "Class" as a suffix to methods like setForm()).

For whoever writes this change notice, #2168333: Ensure custom EntityType controllers can be provided by modules changed some of the method names here (mostly adding "Class" as a suffix to methods like setForm()).

I'm writing the change record for this.

Change notice is here.
https://drupal.org/node/2181667

Awesome, thanks!

Reviewed the change notice together with @michaellenahan, and other than a few minor touch-ups didn't have anything to complain. We can always improve on that, but this issue can be marked fixed, I think.

Looks great.

This is actually not just a 8.x before and 8.x now change, it also applies to 7.x => in almost the same way (there are different keys and so on, but it's the same concept).

Maybe we can slightly update it for that?

Updated over here: https://drupal.org/node/2181667