Posted by tim.plunkett on November 21, 2012 at 5:46pm
21 followers
| 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.
| Attachment | Size | Status | Test result | Operations |
|---|---|---|---|---|
| decouple-entity-types-from-plugins.patch | 83.39 KB | Idle | PASSED: [[SimpleTest]]: [MySQL] 48,460 pass(es). | View details | Re-test |
Comments
#1
Thanks Tim!
#2
After a short conversation with EclipseGc, it became clear that we could make the AnnotatedBaseClass more useful.
I'm happy with this.
#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
The last submitted patch, entity-types-1847002-2.patch, failed testing.
#5
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.
#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 inDrupal\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
\Typesubdir 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\Typesubdirectory.#11
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
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
Okay, here it is. The interdiff is the only change that wasn't just s/\\EntityType/\\Entity\\Type/
#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
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
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
#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
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
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
I still would like to see this.
My main concern right now is that
Drupal\foo\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\FooAccessController
Drupal\foo\Entity\FooStorageController
Drupal\foo\Entity\Type\Foo
i.e.:
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.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.
#27
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:
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
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.
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.
Well, it has important DX implications for the entity system as long as they are plugins, so I think it's relevant.
#32
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:
As a result, the /Plugin directory is a needless subdirectory.
#34
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.
#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
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.
#42
The last submitted patch, entity_plugin_locations-41.patch, failed testing.
#43
Repurposing this issue after #1987298: Change notice: Shorten directory structure and PSR-0 namespacing for plugins