# Switch entity namespaces from \Plugin\Core\Entity to \Plugin\EntityType

 Project: Drupal core Version: 8.x-dev Component: entity system Category: task Priority: normal Assigned: Unassigned Status: active Issue tags: API clean-up

## Issue Summary

#1763974: Convert entity type info into plugins and #1828852: Update entity class names after the conversion to plugins saw a lot of push back on converting entity types into plugins, with good reason.

All we want from the plugin system so far is the discoverability.

This is a first attempt toward doing that, but entity specific.

Because plugins need to be in a separate directory from other non-plugins, I've changed the namespace from Plugin\Core\Entity to EntityType.

So, Node.php now lives at core/modules/node/lib/Drupal/node/EntityType/Node.php, and it is use Drupal\node\EntityType\Node; for using it.

A majority of this is renaming those namespaces, git diff --color-words will help here.

AttachmentSizeStatusTest resultOperations
decouple-entity-types-from-plugins.patch83.39 KBIdlePASSED: [[SimpleTest]]: [MySQL] 48,460 pass(es).View details | Re-test

Thanks Tim!

### #2

 Assigned to: tim.plunkett » Anonymous

After a short conversation with EclipseGc, it became clear that we could make the AnnotatedBaseClass more useful.

I'm happy with this.

AttachmentSizeStatusTest resultOperations
entity-types-1847002-2.patch86.83 KBIdleFAILED: [[SimpleTest]]: [MySQL] 48,547 pass(es), 0 fail(s), and 9 exception(s).View details | Re-test
interdiff.txt9.99 KBIgnored: Check issue status.NoneNone

### #3

Yay! I like this much better.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined@@ -7,17 +7,19 @@+use Drupal\Component\Plugin\Factory\FactoryInterface;+use Drupal\Component\Plugin\Mapper\MapperInterface;

I asked Tim why we were using the FactoryInterface, and he pointed out it was mostly just copied from plugins, so we could remove this dependency. Probably needs more discussion.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined@@ -160,45 +162,66 @@-class EntityManager extends PluginManagerBase {+class EntityManager implements DiscoveryInterface, FactoryInterface, MapperInterface {+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined@@ -7,18 +7,10 @@-class AnnotatedClassDiscovery implements DiscoveryInterface {+class AnnotatedClassDiscovery extends AnnotatedClassDiscoveryBase {+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryBase.phpundefined@@ -2,7 +2,7 @@- * Definition of Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery.+ * Definition of Drupal\Core\Plugin\Discovery\AnnotatedClassDiscoveryBase.

Note to other reviewers: The diff is doing some odd things, so I recommend reading the source for these classes as well.

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined@@ -160,45 +162,66 @@+   * The object that returns the preconfigured entity type instance appropriate for a particular runtime condition.

This is not close to under 80 characters unless we are using hex. ;)

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined@@ -29,72 +21,15 @@ function __construct($owner,$type) {-  public function getDefinitions() {-    $definitions = array();-$reader = new AnnotationReader();-    // Prevent @endlink from being parsed as an annotation.-    $reader->addGlobalIgnoredName('endlink');-- // Register the namespace of classes that can be used for annotations.- AnnotationRegistry::registerAutoloadNamespace('Drupal\Core\Annotation', array(DRUPAL_ROOT . '/core/lib'));- // Get all PSR-0 namespaces.-$namespaces = drupal_classloader()->getNamespaces();-    foreach ($namespaces as$ns => $namespace_dirs) {-- // OS-Safe directory separators.-$ns = str_replace('\\', DIRECTORY_SEPARATOR, $ns);-- foreach ($namespace_dirs as $dir) {- // Check for the pre-determined directory structure to find plugins.-$prefix = implode(DIRECTORY_SEPARATOR, array(-          $ns,- 'Plugin',-$this->owner,-          $this->type- ));-$dir .= DIRECTORY_SEPARATOR . $prefix;-- // If the directory structure exists, look for classes.- if (file_exists($dir)) {-          $directories = new DirectoryIterator($dir);-          foreach ($directories as$fileinfo) {-            // @todo Once core requires 5.3.6, use $fileinfo->getExtension().- if (pathinfo($fileinfo->getFilename(), PATHINFO_EXTENSION) == 'php') {-              $class = str_replace(- DIRECTORY_SEPARATOR,- '\\',-$prefix . DIRECTORY_SEPARATOR . $fileinfo->getBasename('.php')- );-- // The filename is already known, so there is no need to find the- // file. However, StaticReflectionParser needs a finder, so use a- // mock version.-$finder = MockFileFinder::create($fileinfo->getPathName());-$parser = new StaticReflectionParser($class,$finder);--              if ($annotation =$reader->getClassAnnotation($parser->getReflectionClass(), 'Drupal\Core\Annotation\Plugin')) {- // AnnotationInterface::get() returns the array definition- // instead of requiring us to work with the annotation object.-$definition = $annotation->get();-$definition['class'] = $class;-$definitions[$definition['id']] =$definition;-              }-            }-          }-        }-      }-    }

So this code is not actually being removed; the diff is just goofy. Apply the patch and see in AnnotatedClassDiscoveryBase.

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.phpundefined@@ -29,72 +21,15 @@ function __construct($owner,$type) {+  protected function getDirectoryStructure($namespace) {+ return array(+$namespace,+      'Plugin',+      $this->owner,+$this->type,+    );+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscoveryBase.phpundefined@@ -18,14 +18,21 @@-class AnnotatedClassDiscovery implements DiscoveryInterface {+abstract class AnnotatedClassDiscoveryBase implements DiscoveryInterface {...+  protected function getDirectoryStructure($namespace) {+ return array(+$namespace,+    );   }   /**@@ -54,14 +61,9 @@ public function getDefinitions() {@@ -54,14 +61,9 @@ public function getDefinitions() {       // OS-Safe directory separators.       $ns = str_replace('\\', DIRECTORY_SEPARATOR,$ns);+      $prefix = implode(DIRECTORY_SEPARATOR,$this->getDirectoryStructure($ns)); foreach ($namespace_dirs as $dir) { // Check for the pre-determined directory structure to find plugins.-$prefix = implode(DIRECTORY_SEPARATOR, array(-          $ns,- 'Plugin',-$this->owner,-          $this->type- )); This is a lot cleaner. Nice! +++ b/core/modules/views/lib/Drupal/views/Tests/ModuleTest.phpundefined@@ -196,7 +196,7 @@ function testStatusFunctions() { * @param array$views-   *   An array of Drupal\views\Plugin\Core\Entity\View objects for which to+   *   An array of Drupal\views\EntityType\View objects for which to    *   create an options array.

Some comment rewrapping will need to happen in places like this.

### #4

 Status: needs review » needs work

The last submitted patch, entity-types-1847002-2.patch, failed testing.

### #5

 Status: needs work » needs review

The Entity Access stuff went in in the meantime.
Also, I removed FactoryInterface and MapperInterface until we need them.

I didn't re-wrap the comments manually yet. Otherwise this should address everything in #3.

AttachmentSizeStatusTest resultOperations
entity-types-1847002-5.patch93.12 KBIdlePASSED: [[SimpleTest]]: [MySQL] 48,568 pass(es).View details | Re-test
interdiff.txt9.64 KBIgnored: Check issue status.NoneNone

### #6

I actually think entities make fine plugins per discussion had in IRC with EclipseGC and fmizzell_. I also don't have any problem with discovery being used separately from plugins though. Interested in where this goes.

### #7

I didn't fully think through this yet.

For now, I only have a single, major point to raise:

All of these modules are integrating with a component and subsystem that is called "Entity". The common, PHP-world-wide standard for denoting that is to structure and put the respective integration class files into a directory that uses the exact name of the component they are integrating with. You can see this pattern being followed to some good extent in Drupal\Core\* already. Even better examples can be found in the source code of Symfony components (which are each decoupled on their own, but optionally also integrate with each other).

Essentially, the entity type classes should be located in e.g., Drupal\node\Entity\Node.

It is perfectly possible that the new Entity Access and Entity Operations and Entity Actions and Entity Displays and Entity WTF APIs will demand for further Entity system specific classes though. In case this change here still depends on a filesystem scan in the designated directory to find entity type classes, and in case we want to avoid to parse the annotations of other potentially existing classes at all costs, then we should change the location into Drupal\node\Entity\Type\Node.

IMHO this is important, not only for properly structuring our source code, but especially for developers who are new to Drupal.

### #8

A small, but potentially significant amendment to #7:

I already questioned more than once but only for myself why we have the "/Plugin" directory prefix in the first place. Given #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory and related issues, I do wonder whether should not drop that extra "Plugin" parent directory entirely. The reason for doing so is that we'd effectively end up with the exact same as #7 for entity types:

  Drupal\node\Plugin\Core\Entity» Drupal\node\Plugin\Entity\Type [corrected owner + type]» Drupal\node\Entity\Type

### #9

@sun, I appreciate the feedback, but that seems rather non-actionable. If you have a specific suggestion for this issue, please mark the issue "needs work" with a suggestion/request, or at least a discussion point.

I picked EntityType because I thought it was rather uncontroversial. If you'd rather that be Entity\Type, it's a trivial change, and just one more directory to affect DX.

### #10

Sorry, yeah, the actionable part from my side would be to rename into .\Entity\.

I don't know whether we need or will need an additional separation into .\Entity\Type\, as we currently don't have it either, so I think that could be left for later. OTOH, one could reasonably argue that custom entity controllers should be moved into that namespace, too; e.g., Drupal\node\Entity\NodeStorageController, which would finally lead to some real and properly structured extension directories. I was already struggling with the arbitrarily mixed classes when working on the UserData service, which happens to be surrounded by a dozen of other classes in Drupal\user\. If we'd shift the entity classes off into a proper .\Entity\ subdir, then I guess .\Entity\Type\ might start to make some more sense.

So I think that .\Entity\Type\ would actually be better.

AFAICS, the additional \Type subdir is only needed for the annotated class discovery to not hit other classes — I don't know how fast it is able to skip over unrelated classes though - if that was fast enough, then we could consider to skip the additional \Type subdirectory.

### #11

OTOH, one could reasonably argue that custom entity controllers should be moved into that namespace, too; e.g., Drupal\node\Entity\NodeStorageController, which would finally lead to some real and properly structured extension directories.

I do argue so, yes. From a DX perspective I'd like to see all entity-related classes below \Entity, but given we've the plugin system people we'll have to deal with per-pllugin-type directories anyway - so Entity\Type would be fine also.

### #12

 Assigned to: Anonymous » tim.plunkett Status: needs review » needs work

Non-annotated classes cannot exist in the same directory as annotated classes.
But we can do Entity\Type here, and then move the controllers and related stuff to Entity\ in a follow-up.

### #13

Excellent, let's do that :)

### #14

 Status: needs work » needs review

Okay, here it is. The interdiff is the only change that wasn't just s/\\EntityType/\\Entity\\Type/

AttachmentSizeStatusTest resultOperations
entity-types-1847002-13.patch93.38 KBIdlePASSED: [[SimpleTest]]: [MySQL] 48,718 pass(es).View details | Re-test
interdiff.txt531 bytesIgnored: Check issue status.NoneNone

### #15

I hope this question doesn't come across in an offending way:

Is there anything that is blocked on this change? If not, then I think that many contributors (myself included) would highly appreciate it if we could postpone this adjustment until feature freeze, since I believe we have many entity-related issues in the queue right now which are trying hard to get their patches done in time. The namespace and filesystem location changes could be happily done "afterwards."

Of course, if something happens to be blocked on this, that would change the situation.

### #16

 Assigned to: tim.plunkett » catch

Nothing is blocked on this, and I take zero offense to that question :)

You, catch, and yched were those most vocally against the Plugin\Core\Entity switch, and its implications, but there was a good deal of unhappiness and confusion coming out of that issue.

There are no technical reasons for this to go in any time soon, and since it seems like everyone is pretty happy with this.

I'm assigning to catch for his opinion on postponing this or not.

### #17

This makes sense to me as well, +1.

### #18

 Status: needs review » needs work

If we are decoupling this from the plugin system, I don't think we should continue to use the "@Plugin" anntotation. Instead the Annotation to parse should be set in the child class. In this case I think "@EntityType" makes sense. We could also do "@Entity", but I think that would be a bit ambiguous. Marking "needs work" for that.
The line that references Plugin currently is:
if ($annotation =$reader->getClassAnnotation($parser->getReflectionClass(), 'Drupal\Core\Annotation\Plugin')) { For this to be usable in contrib, we also need to be able to set the namespace to use for all *possible* annotations (so people can provide their own things like the current "@Translation") The line that currently does this, is: AnnotationRegistry::registerAutoloadNamespace('Drupal\Core\Annotation', array(DRUPAL_ROOT . '/core/lib')); ### #19  Assigned to: catch » tim.plunkett #18 should definitely be a separate issue. Now that we have more time to feature freeze, I'm going to get this patch ready again. ### #20 Re #19: I'm not hung up on doing it in this issue per se, but can you elaborate on why replacing the "@Plugin" annotation by something more fitting does not belong into the issue about decoupling from the Plugin system? It also seems as though it wouldn't be too hard to do... ### #21 Because if we do it the easy way we'll have class EntityType extends Plugin, if we do it the right way we'll add a base class, and we'll have to much around in AnnotatedClassDiscovery when that's being changed in 3 different issues, and none of it is central to the main goal of changing the discovery path ### #22 Right, I had assumed we would go with the first version and then refactor later. But again, I'm totally not married to this, I just wanted to hear your thoughts. :-) Thanks for the quick reply. ### #23 I just discovered this issue thanks to #1867228-4: Enable EntityManager to create entities, and I read all the comments here (but not yet in #1763974: Convert entity type info into plugins), but I'm still left wondering why entity types are not good as plugins. Can someone summarize the main points for that, or do I need to read the entirety of #1763974: Convert entity type info into plugins? From my perspective, they make 100% sense as plugins: they need to be discovered, and per #1867228: Enable EntityManager to create entities, they need to be instantiated from a set of values (entity_create() = FactoryInterface::createInstance()) and and from an id (entity_load() = MapperInterface::getInstance()). ### #24  Status: needs work » postponed Mostly, this was an acknowledgement that The namespaces were verbose and uninformative and technically flawed (could be solved by #1849752: Abstract non-Drupal-specific parts of AnnotatedClassDiscovery into a Drupal\Component base class) Entity types had no owner and are floating around in Drupal\Core (can be solved by #1857074: Re-introduce entity.module as a configuration owner) Only the discovery aspect was being used, not instantiation/retrieval (to be changed in #1867228: Enable EntityManager to create entities So parts of this issue is still valid, but should likely be postponed on those two issues that are still open. ### #25  Assigned to: tim.plunkett » Anonymous Status: postponed » closed (works as designed) I think with the progress made in #1867228: Enable EntityManager to create entities and the other issues above, this is no longer something we actually want. (And if it is, lets get some discussion going again!) ### #26  Status: closed (works as designed) » active I still would like to see this. My main concern right now is that • Entity list + access + storage + form controllers, etc.pp. live in Drupal\foo\ • Entity type classes live in Drupal\foo\Plugin\Core\Entity\ i.e., completely detached from each other, and none of both make their exact type and relationship to the Entity system really clear. What I'd like to see is this: Drupal\foo\Entity\FooListControllerDrupal\foo\Entity\FooAccessControllerDrupal\foo\Entity\FooStorageControllerDrupal\foo\Entity\Type\Foo i.e.: 1. The top-level "Entity" sub-namespace/folder clarifies the overall relationship to the Entity system, and bundles all related classes together into one bag. 2. This layout is apparently a standard for PSR-0 components. The pattern can be re-used for all other classes that are integrating with a particular service/component. Looking beyond this particular change, TBH, with regard to plugin classes, I'd even like to consider to drop the additional .\Plugin\ sub-namespace/folder altogether, since it appears to be pretty clear that D8's entire architecture will revolve around plugins — there's hardly anything left that wouldn't be a plugin, so IMHO it's completely pointless to have them "separated" into an additional subdirectory. The directory structure for plugins (i.e., /[owner]/[type]) actually resembles aforementioned de-facto standard for PSR-0 component integration classes — all it really does is to "enhance" it by saying that every module is an owner and thus the name of another component/service we're integrating with. So in the end, it's more than foreseeable that we'll arrive at this: Drupal\foo\Entity\Type\Foo.phpDrupal\foo\Entity\FooListController.phpDrupal\foo\Entity\...Drupal\foo\block\block\RecentFoo.phpDrupal\foo\views\field\Foo.php We just don't see that as apparent just yet, since we're still in the process of converting everything. But once we're done, that will be the effective result, just with an obscure "Plugin" subdirectory in between. ### #27 Drupal\foo\Entity\Type\Foo.php Drupal\foo\Entity\FooListController.php Drupal\foo\Entity\... Drupal\foo\block\block\RecentFoo.php Drupal\foo\views\field\Foo.php So where would I put classes used by my own module? Below Drupal\foo\foo ? ### #28 Not sure I understand the question... but let me try: 1. If those classes are new services of their own, then Drupal\foo\FooHandler. 2. If Foo module is implementing its own APIs, then sure, Drupal\foo\foo\bar\FooPlugin. A typical example for 1) is Drupal\user\UserData. A typical example for 2) is Drupal\views\views\style\Grid. ### #29 I was referring to 1) - defining new classes, services, handlers whatever - anything custom. So this means modules do not own the namespaces below Drupal\foo anymore - the only place you may create a custom class is Drupal\foo - what is a bit weird. E.g., when Rules in 8.x defines quite some new classes and interfaces I want to structure it using directories, but I cannot? That said, I love the suggestion but I guess that's the issue why we have that "plugin" sub-directory in the first place. ### #30 Hm. I think the plugin directory is a discussion for a separate issue :) Anyway, my main problem with this is still terminology and and a bit how it matches conceptually: #1867228: Enable EntityManager to create entities shows the terminology problem nicely: "createInstance($plugin_id, array $configuration = array()". We have instance, plugin_id and$configuration. Anywhere else, we talk about an $entity,$entity_type and $values. And it's not just naming, it's also doesn't match conceptually. It's not configuration, it is data. Everywhere else, plugins contain either logic or wrap something and you possibly want to replace it with different logic or a different backend. That's just not true for entities. They are just data, the only "logic" they contain is a bunch of flags and shortcuts to certain data (e.g. to get the label, or the id). The logic and storage is in the controllers. We actually made *very* sure that you can not replace Node with MyNode unless MyNode extends from Node by adding a million type casts for Node. And now we make it pluggable? :) The controllers are the pluggable things here, and they can be pluggable without using the plugin system. ### #31 #1867228: Enable EntityManager to create and load entities shows the terminology problem nicely: "createInstance($plugin_id, array $configuration = array()". We have instance, plugin_id and$configuration. Anywhere else, we talk about an $entity,$entity_type and $values. And it's not just naming, it's also doesn't match conceptually. It's not configuration, it is data. Yes, but where's the problem with leaving$configuration out if we need it? That's how the typed data API already handles data values - they are not subject of the plugin or its configuration.

We actually made *very* sure that you can not replace Node with MyNode unless MyNode extends from Node by adding a million type casts for Node. And now we make it pluggable? :)

To me, using the plugin system does not mean that we make node classes truly pluggable. If you provide a "views field handler" plugin, e.g. a node edit link - it's not supposed to be pluggable either. You just provide a new plugin implementation: a views field, just as you can provide a new entity type.

Hm. I think the plugin directory is a discussion for a separate issue :)

Well, it has important DX implications for the entity system as long as they are plugins, so I think it's relevant.

### #32

 Title: Decouple annotated entity classes from the plugin system » Switch entity namespaces from \Plugin\Core\Entity to \Entity\Type

I'm actually convinced that all we're discussing here anymore is the namespace. We're already using the manager and discovery, and we have an issue to use the factory.

### #33

Agreed. Whether entity types are plugins or not is an architectural design decision that should be based on technical factors.

The reason for why I think we can safely skip the /Plugin directory is twofold:

1. The namespace + annotation discovered classes are living in a secondary subdirectory. Due to that, the chance for false-positives is small.
2. False-positive file matches do not actually break anything. If the discovery happens to find a /foo/bar/Baz.php class, then it will still check for the annotation first, before including it. (And if this doesn't work as safely as it should yet, then we need to fix it either way.)

As a result, the /Plugin directory is a needless subdirectory.

### #34

As a result, the /Plugin directory is a needless subdirectory.

Agreed for Entity\Type. And maybe for all plugin types defined by core subsystems as opposed to modules. I'm not convinced yet about dropping 'Plugin' from module-defined plugin types though.

How about this patch as a starting point? The @todo should be fixed, of course, as part of the scope of this issue.

AttachmentSizeStatusTest resultOperations
entity_plugin_locations.patch2.34 KBIdlePASSED: [[SimpleTest]]: [MySQL] 50,170 pass(es).View details | Re-test

### #35

-    $this->discovery = new AnnotatedClassDiscovery('Core', 'Entity');+$this->discovery = new AnnotatedClassDiscovery('Core', 'Entity', NULL, 'Plugin\Core\Entity');

Well, as also discussed elsewhere, I strongly believe that "Core" is not a valid owner, as it doesn't represent a singular service or extension, but instead, is only a bucket/container for multiple services/extensions.

Therefore, "Entity" is the actual $owner, and "Type" should be the plugin$type.

### #36

Let's leave #35 to #1821846: Consider better naming conventions for plugin types owned by includes. What we do here may help inform that issue too, but #34 intentionally steers clear of changing what's already in HEAD in that regard.

There's a further interesting question here as to what the purpose/meaning of $owner and$type are for managers that pass something for the new $sub_namespace parameter. These parameters currently have no purpose other than for constructing$this->subNamespace when one isn't passed, but I don't know if some other purpose will be found for them by the time D8 ships, or in contrib.

### #37

Agreed with #34 and #35.

### #38

It should be noted that the idea behind the Plugin directory is as I remember largely there for performance as fewer classes need to be scanned since we can match on the directory prior to parsing annotations. Fewer disk hits, less work processing annotations, etc. I think "Nothing breaks" might be an over-simplification and possibly wrong description of this change that should be considered more.

### #39

You cannot have a non plugin in a directory with plugins. I've tried, it blows up. So we'd need to have an explicit directory for only these files, which \Entity\Type still gives us. Only Entity plugins would be in there, the controllers would be in \Entity

### #40

1) Yes, as previously mentioned, the sub-sub-directory/namespace already implies a very very limited subset of potential matches.

2) If the annotation discovery blows up on false-positives (and whatever else), then that's definitely a major bug that needs to be fixed. Blowing up goes completely against the semantic/generic purpose of annotations and means that the implementation is utterly wrong.

(That said, at this point, given its incredibly slow performance, and all of the recent, negative DX feedback, I'm this ---> <--- close to completely rewrite the annotation parser with a custom implementation [oh, YAML?], since the current situation is just simply masochistic.)

### #41

 Status: active » needs review

Unless someone feels inspired to roll a patch that moves all entity types in one shot, we'll need to do it in steps, so here's a way to allow that, along with moving Node.

AttachmentSizeStatusTest resultOperations
entity_plugin_locations-41.patch20.21 KBIdlePASSED: [[SimpleTest]]: [MySQL] 49,919 pass(es).View details | Re-test

### #42

 Status: needs review » needs work

The last submitted patch, entity_plugin_locations-41.patch, failed testing.

### #43

 Title: Switch entity namespaces from \Plugin\Core\Entity to \Entity\Type » Switch entity namespaces from \Plugin\Core\Entity to \Plugin\EntityType Priority: major » normal Status: needs work » active

Repurposing this issue after #1987298: Change notice: Shorten directory structure and PSR-0 namespacing for plugins