Updated: Comment #45

Problem/Motivation

In #2043379: Allow plugins to be discovered in any directory we added the ability to have complete control over a plugin's namespace (and therefore, directory structure).

#2005716: Promote EntityType to a domain object attempted to rearchitect entity types, with one of the end goals being moving entity types to a new namespace pattern:

Before:
Drupal\node\Plugin\Core\Entity\Node
After:
Drupal\node\Node

This will hopefully reduce some of "Why is my entity type a Plugin? Why do I need three extra directories? Why is my Node class so far away from my Drupal\node\NodeInterface? What did you do?!?"

Proposed resolution

Write a script to do the directory move, and namespace/docblock changes

Remaining tasks

Write said script, and get it committed ASAP

User interface changes

N/A

API changes

All entity types will be moved.
However, since every entity type has an interface, no typehints should need to change.
Only modules providing custom entity types will be affected.

Original report by @tim.plunkett

#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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

Thanks Tim!

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
FileSize
9.99 KB
86.83 KB

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

I'm happy with this.

xjm’s picture

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.

Status: Needs review » Needs work

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
9.64 KB
93.12 KB

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.

neclimdul’s picture

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.

sun’s picture

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.

sun’s picture

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
tim.plunkett’s picture

@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.

sun’s picture

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.

fago’s picture

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.

tim.plunkett’s picture

Assigned: Unassigned » 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.

sun’s picture

Excellent, let's do that :)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
531 bytes
93.38 KB

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

sun’s picture

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.

tim.plunkett’s picture

Assigned: 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.

Berdir’s picture

This makes sense to me as well, +1.

tstoeckler’s picture

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'));

tim.plunkett’s picture

Assigned: 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.

tstoeckler’s picture

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...

tim.plunkett’s picture

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

tstoeckler’s picture

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.

effulgentsia’s picture

I just discovered this issue thanks to #1867228-4: Make EntityTypeManager provide an entity factory, 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: Make EntityTypeManager provide an entity factory, they need to be instantiated from a set of values (entity_create() = FactoryInterface::createInstance()) and and from an id (entity_load() = MapperInterface::getInstance()).

tim.plunkett’s picture

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: Make EntityTypeManager provide an entity factory

So parts of this issue is still valid, but should likely be postponed on those two issues that are still open.

tim.plunkett’s picture

Assigned: tim.plunkett » Unassigned
Status: Postponed » Closed (works as designed)

I think with the progress made in #1867228: Make EntityTypeManager provide an entity factory and the other issues above, this is no longer something we actually want.

(And if it is, lets get some discussion going again!)

sun’s picture

Status: Closed (works as designed) » Active
Issue tags: +API clean-up

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\FooListController
Drupal\foo\Entity\FooAccessController
Drupal\foo\Entity\FooStorageController
Drupal\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.php
Drupal\foo\Entity\FooListController.php
Drupal\foo\Entity\...
Drupal\foo\block\block\RecentFoo.php
Drupal\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.

fago’s picture

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 ?

sun’s picture

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.

fago’s picture

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.

Berdir’s picture

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: Make EntityTypeManager provide an entity factory 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.

fago’s picture

#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.

tim.plunkett’s picture

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.

sun’s picture

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.

effulgentsia’s picture

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.

sun’s picture

-    $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.

effulgentsia’s picture

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.

fago’s picture

Agreed with #34 and #35.

neclimdul’s picture

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.

tim.plunkett’s picture

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

sun’s picture

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.)

effulgentsia’s picture

Status: Active » Needs review
FileSize
20.21 KB

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.

Status: Needs review » Needs work

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

tim.plunkett’s picture

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
tim.plunkett’s picture

Title: Switch entity namespaces from \Plugin\Core\Entity to \Plugin\EntityType » Switch entity type namespaces from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider
Priority: Normal » Major
yched’s picture

Er - this issue has always been around some flavor of lib/Drupal/$provider/Entity, how come all of a sudden it's about having the entity type classes be directly at the root ?

Very much +1 for moving those out of Plugins, but -1 for losing structure (& encouraging contrib to put other "unclassified" stuff in there ?)
And where would the supporting classes go ?

tim.plunkett’s picture

Where they are right now:

Drupal\node\NodeInterface
Drupal\node\NodeStorageController
Drupal\node\NodeTranslationController
Drupal\node\NodeRenderController
etc

Unless we just decide to move all of that into Drupal\$provider\Entity, since we're renaming it all in #1976158: Rename entity storage/list/form/render "controllers" to handlers

tim.plunkett’s picture

Honestly I'm fine with Drupal\$provider\Entity for everything except the interfaces. Doesn't bother me.

Berdir’s picture

An Entity or EntityType directory/namespace would be my suggestion to, I think that having it in the top-level would be considerably slower as it would have to parse lot of unrelated classes.

Having the interface in the top-level directory makes sense to me, the controller could be in a separate namespace, that's what I'm already doing in a contrib module that has ~5 diffferent entity types. As suggested we could come up with a standard when renaming them?

yched’s picture

Sounds good (why leave interfaces out though ?)

Berdir’s picture

We can move them too, but to where? Same namespace as entity types means we're going to parse those too to look for annotations I think?

effulgentsia’s picture

Title: Switch entity type namespaces from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider » Move entity type namespaces from Drupal\$provider\Plugin\Core\Entity to somewhere shallower
Issue tags: +Approved API change

Discussed this with @alexpott and he approved moving them out of Plugin. Retitling issue to reflect what's approved at this point. Per recent comments, there's not consensus yet on where we're moving them to.

yched’s picture

We can move [interfaces] too, but to where? Same namespace as entity types means we're going to parse those too to look for annotations I think?

Yes, I'm not sure that's a problem ? There already are base classes next to plugin classes in a couple plugin directories.

msonnabaum’s picture

Status: Active » Needs review
FileSize
258.9 KB

Here's a patch that moves everything to /Entity.

It doesn't yet move any other files around.

Here's the script I use if anyone else needs to reroll:

https://gist.github.com/msonnabaum/6258265

tim.plunkett’s picture

I think that this is a well-scoped change. Move the controllers can just happen in #1976158: Rename entity storage/list/form/render "controllers" to handlers, no reason to move them as well.

It seems we have consensus here, if this passes I'd like to RTBC it.

yched’s picture

Yay, +1

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

I reviewed this with git diff --color-words and it checks out.

tim.plunkett’s picture

Title: Move entity type namespaces from Drupal\$provider\Plugin\Core\Entity to somewhere shallower » Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity
webchick’s picture

OMG YES. If there are no further objections to this by the time I'm online tomorrow, I'll be happy to commit it. :) (anyone else feel free to, too.)

webchick’s picture

Title: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity » Change notice: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity
Status: Reviewed & tested by the community » Active
Issue tags: +Needs change record

Committed and pushed to 8.x. YEAAAAH!! :D

Let's get a quick change notice out so that all developers everywhere can rejoice.

geerlingguy’s picture

I'm thinking #2068667: Error message when trying to access the configure page for text formats using ckeditor. may have been caused by this—as it's the only change to CKEditorPluginInterface.php that I can find with git log in the past month or so, and everything was working a couple days ago when I ported Wysiwyg Linebreaks to D8 at MWDS... can anyone figure out why this change might have broken the Linebreaks plugin in Wysiwyg Linebreaks?

[Edit: Looks like I just needed to update the path to the Editor class, like below:]

-use Drupal\editor\Plugin\Core\Entity\Editor;
+use Drupal\editor\Entity\Editor;
webchick’s picture

..Please?

Guys, I am going to be far less inclined to commit these types of module developer-facing API clean-ups post code-freeze if the change notices are sitting out there for longer than a day or two.

tim.plunkett’s picture

I added a quick and dirty one at https://drupal.org/node/2069199. We still need to hunt through and update the existing change notices.

effulgentsia’s picture

We still need to hunt through and update the existing change notices.

There might be others, but https://drupal.org/node/1400186 is probably the main one.

xjm’s picture

Priority: Major » Critical
Berdir’s picture

Title: Change notice: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity » Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity
Priority: Critical » Major
Status: Active » Fixed
Issue tags: -Needs change record

I updated https://drupal.org/node/1400186. Doesn't seem to be possible to search for "Plugin\Core" (no results even before I updated that issue and I wasn't able to find any other. So I think we can close this.

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

drumm’s picture

Issue summary: View changes

Updated