Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
Problem/Motivation
Currently, in order to provide a new entity type, you must provide both a standalone class and implement a hook to declare static metadata. This is a perfect candidate for the Plugin system, and will help to resolve complexities about calling entity_get_info().
Proposed resolution
Convert entity classes into annotated plugins
Remaining tasks
Blockers
Follow-ups
- #1828852: Update entity class names after the conversion to plugins
- #1821846: Consider better naming conventions for plugin types owned by includes
- #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest
- #1822452: Convert all nested Entity type info keys to use underscores instead of spaces
- #1822458: Move dynamic parts (view modes, bundles) out of entity_info()
- #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator
- #1821846: Consider better naming conventions for plugin types owned by includes
- #1827506: Translate bundle labels and view mode labels
User interface changes
N/A
API changes
- Entity info is provided as an annotation on entity classes
- Entity classes are now plugins, and as such are moved into a sub-directory: Drupal\node\Node becomes Drupal\node\Plugin\Core\Entity\Node
- hook_entity_info() is removed
- The resulting keys from entity_get_info() now have underscores instead of spaces:
Old name New name entity class class controller class controller_class render controller class render_controller_class form controller class form_controller_class list controller class list_controller_class base table base_table static cache static_cache field cache field_cache uri callback urli_callback label callback label_callback entity keys entity_keys bundle keys bundke_keys view modes view_modes
Comment | File | Size | Author |
---|---|---|---|
#218 | drupal8.entity-info.218.patch | 8.62 KB | sun |
#203 | drupal8.entity-info.203.patch | 6.9 KB | neclimdul |
#186 | drupal8.entity-info.186.patch | 4.81 KB | sun |
#166 | drupal-1763974-166.patch | 190.17 KB | tim.plunkett |
#161 | drupal-1763974-160.patch | 191.01 KB | tim.plunkett |
Comments
Comment #1
tim.plunkettYes please.
Comment #2
catchI'd love to get rid of hook_entity_info(), but two questions:
- what happens to hook_entity_info_alter() - RDF module in core is using that.
- what about ECK - I could see possibly converting annotations into default configuration or something, but could we also just go straight to using configuration?
Comment #3
tim.plunketthook_entity_info_alter would be solved by #1705702: Provide a way to allow modules alter plugin definitions
Comment #4
sunDang, good points, @catch! :)
Indeed, there are quite some interesting implementations there:
http://api.drupal.org/api/search/8/entity_info_alter
#1705702: Provide a way to allow modules alter plugin definitions indeed seems to address most alter hook implementation needs.
on RDF: I'm actually not sure whether its use of hook_entity_info() is really legit -- I've the impression RDF abuses it as dumping ground for additional meta-data. rdf_entity_info_alter() already contains a @todo that seems to hint in that same direction.
on ECK: Mind-boggling. ;)
AFAIK, there once was a
ConfigPluginDiscoveryDecorator
in the plugins branch, which got ripped out since it didn't fully work or didn't make sense.As of now I think that entity types should be declarative, bound to code, and are not really configuration. Otherwise, you'd be able to stage entity types between servers, and the expectation would be that putting a new
entity.type.event.yml
file in your configuration would automagically create an {event} table and possibly even {event_revision} table and so on..... and some thing would have to handle that, so errr, entity types would be Configurables (OMG :P)...It's very hard to wrap one's brain around that right now. I'm inclined to punt on that entirely and leave it for ECK in contrib to figure out.
Comment #5
scor CreditAttribution: scor commented@sun: re RDF's use of entity_info, afaik this is what the entity info is for, to store metadata about your entities. If I recall properly, it's actually catch who suggested this approach, but maybe a better solution exist now? The @todo you see there is actually going away in #1092352: Improve documentation of rdf_entity_info_alter(), see the proposed new documentation (includes @todo removal).
Note that book and forum also alter entity_info to store view modes and do some path magic...
Comment #6
yched CreditAttribution: yched commentedIf Entity types moved to plugins, them something like ECK would typically use derivatives. I.e. one single class that is seen as a series of actual implementations, read from config.
Comment #7
fagoYes, #6 should work just fine I think.
Comment #8
tim.plunkettOnly parts of hook_entity_info() would move. The 'bundles' key would not. But anything static, definitely.
@catch has pointed out that ConfigEntity objects are abusing entity_get_info(), this would solve that. So I'm bumping priority.
Comment #9
tim.plunkettHere is a first stab at this.
The only downside to Entities as Plugins is that their namespace is dictated to them, and the existing ones will all have to move.
Instead of
Drupal\node\Node
,it will be
Drupal\node\Plugin\Core\Entity\Node
The naming of "Core" and "Entity" is up to us, but those make sense to me.
I've started work on this in http://drupalcode.org/sandbox/tim.plunkett/1698392.git/shortlog/refs/hea....
The following keys should be moved to annotations:
"entity class" should be removed.
The following keys have been left in entity_get_info():
When converting to annotations, all spaces in keys must be moved to underscores. I added a compatibility layer for now.
I moved Node as an example, to see what happens. I updated all of its
use
statements and type hinting, but not in all docblocks yet.Comment #11
tim.plunkettWhoops, I messed up the Node namespaces.
While the test ran I also converted User and Comment.
Comment #12
tim.plunkettOkay, and here's Term and File.
I'm skipping Vocabulary, because I'm not sure what's going to happen to that, and I didn't get to the test entities yet.
Comment #13
plachThere are still some wrong namespaces here and there. A search/replace should do the trick :)
Comment #15
tim.plunkettArgh! Thanks @plach.
Comment #16
plachJust stumbled upon this and while on one hand I find its goal sensible, OTOH I think the issue summary really lacks some explanations about why we need/want this. From just skimming through the code it seems we are only changing the way entity info are defined/discovered: is the goal of this issue to leverage annotations to declare entity info? What does relying on plugins buy us here? Perhaps moving to a unified way to provide swappable entity implementations instead of our current one-off solution? Is this paving the way for the implementation of configurable entities and/or VDC?
Sorry if this sounds dumb but I missed most of the Plugin discussion and I'm looking to this issue with fresh eyes.
Comment #17
tim.plunkettAgreed, I didn't even notice how terse that was.
Comment #19
tim.plunkettAs I feared, this is blocked on #1708692: Bundle services aren't available in the request that enables the module. Postponing for now.
Comment #20
rbayliss CreditAttribution: rbayliss commentedLooks great, but do we really need to put all entity classes under the Plugin\Core\Entity namespace? Given how critical entities are to the system, it seems like a developer experience headache to have to deal with names like "Drupal\node\Plugin\Core\Entity\Node."
Comment #21
tim.plunkettYes, it is essential. I had the same thought as you and tried to come up with a solution. But, in D8 plugins will be so common that this won't seem out of place.
Comment #22
pounardEven if the original namespace in core is "Drupal\Plugin\Core\Entity", node module could use "Drupal\node\Node" instead, it is not illegal nor illogical. In the core context, "Entity" might be something related to plugins mainly for technical reasons; On the opposite side of things in the node module context "Node" is a business object being part of the "node API" hence the "Drupal\node\Node" class name, where "Node" is the central piece of the node module business API.
Comment #23
tim.plunkett@pounard, I wish it worked that way, but if you look at how AnnotatedClassDiscovery builds its prefix: http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/lib/D...
It enforces Plugin\$owner\$type, where $owner and $type have to be the same for all entities, which is why I chose Plugin\Core\Entity.
Unless we make AnnotatedClassDiscovery less strict, it's a requirement.
Also, it's not Drupal\Plugin\Core\Entity, it'd be Drupal\node\Plugin\Core\Entity\Node
Comment #24
tstoecklerSince this will set the tone for various "foo-to-plugins" conversions, I think we need to decide carefully what to do here. As @tim.plunkett already mentions, it would be possible to allow for a different namespace scheme, if we wanted to alter the Plugin system as to allow for it. On the other hand #21 brings up an important point: Even though
Drupal\node\Plugin\Core\Entity\Node
is a bit verbose, it might actually turn into a DX improvement in D8, because as a D8 developer you immediately know what you're dealing with (a plugin!), you know how to extend it (with a similar class inDrupal\mymodule\Plugin\Core\Entity\MyEntity
), etc., etc. So I'm not really arguing in any specific direction, I just think we should make this decision based (more-or-less) purely on DX and not on current "restrictions" of the Plugin system.Comment #25
pounardHum DX can unserve the purpose sometime, considering that depending on the API you are developing orientation, you may want to inverse name logic for business purposes, because knowing that it's a plugin is not that important, but knowing that it's the center of the API is. So I think basing this kind of decision on DX only probably isn't a good idea: being restrictive is by design against DX. A good DX is probably a good API, but a good DX is not forcing developers how to organise their own packages: this might make some of them very angry.
That said, if this is a pure technical restriction due to the way annotation discovery works, then there is no questions at all. The real question is do we want to refactor the annotation discovery or not? This is a question for another new issue or follow-up may be.
EDIT: Thanks tim for the link and explaination.
Comment #26
tstoecklerI don't agree. It probably wasn't clear from my point above, but I'm not arguing in any direction here. Personally, I also think we should probably adapt the Plugin system / Annotations discovery to allow for more flexible namespace, but I'm not really sure. What I'm trying to say is that both sides, i.e. both
Drupal\node\Entity\Node
andDrupa\node\Plugin\Core\Entity\Node
could be considered as better DX, depending on which argument(s) you consider more important. We could also consider (again, not saying this is better, but it's a possibility) stripping the plugin $owner from the namespace, i.e.Drupal\node\Plugin\Entity\Node
. That would be sort of a "compromise".Comment #27
pounardIndeed you are right it's a question of point of view, anyway I think that annotation discovery belongs to another issue, as of now, due to this technical restriction, keeping the extremely long namespace for node is the only thing we can do; Am I wrong?
Comment #28
tstoecklerWell, technically it is a separate issue, yes. But:
1. This issue would set a precedent, so while we could definitely revert this decision later that would mean more work.
2. The technical change is not *that* large. The only "real" change would be to remove this line from AnnotatedClassDiscovery.php. Of course, a bit of plumbing would need to be done to actually make that work and pass tests.
That said, I have no problem simply going with this for now. I'm still leaning slightly against the verbose namespaces, but can absolutely live with that.
Comment #29
pounardYes, ideally I'd prefer to shorten the namespaces too. I'm not against refactoring the annotation discovery throught this issue thought.
Comment #30
EclipseGc CreditAttribution: EclipseGc commentedSo before we start making plans to just change AnnotatedClassDiscovery, we should all get on the same page about a few things.
1.) Core, Modules, Etc can declare multiple Plugin Types.
2.) Core, Modules, Etc can declare conflicting Plugin Type namespaces
Example:
Core needs caching.
Views needs caching.
Modules X, Y and Z need caching.
Core already took the "cache" namespace, so, if we force a one to one, then views has to declare views_cache instead of just cache. The same is true of X, Y and Z. Now the module developer has to know that the cache plugin type name is already taken, otherwise he blows up the whole system. Understanding we can protect him from himself here, we did.
In the same way, we've chosen to use PSR-0 in Drupal for the D8 cycle. This was the right choice, and it results in some rather nasty and verbose class names... but that's the nature of the beast, and developers constantly fighting that nature is getting to be a bit of a broken record. PSR-0 plus my previous few details here means that we:
1.) need a directory in which we know plugins will reside, thus the "Plugin" dir.
2.) We should automatically namespace things, I've been involved in plugin conflicts that failed to namespace appropriately, and they are some of the hardest bugs to track down if you don't know what you're looking for.
Thus, sites/all/modules/my_module/lib/Drupal/my_module/Plugin/[owner]/[type]/Class.php is the appropriate way to solve this problem, and while I'm sorry that it results in these sorts of directory structures, that's pretty much the intent of PSR-0, so...
With regard to this issue, yes I agree it should go in as is, but please don't file an issue to change how the AnnotatedClassDiscovery does its actual discovery, because that will only result in a long drawn out bikeshed that I've done my best to avoid by engaging in IRC and F2F discussion on the topic. If you want to discuss this topic that way, I'm in IRC at virtually all times (as close as possible) and I'm happy to organize a google hangout on the topic just to avoid this bikeshed. Also, since the Plugin system is a component, we'd like to get annotated class discovery out into the component area eventually as well, and in the greater component library world, beyond the borders of what is Drupal, this issue will be even more pronounced. A lot of thought and effort has gone into this issue, and many years of experience inform it.
Thanks for your consideration,
Eclipse
PS: If I've come across as being "short" here, that is certainly not my intention, we are just very short on time, and I'd prefer to not lose more of it to this particular issue. As always, I'm very happy to discuss this in an IRC or F2F, Skype/G+ format. Just find me and I'll be happy to address your comments or concerns.
Comment #31
tim.plunkettAnother benefit to this: entity types wouldn't need to be tied to a module. See #1043198: Convert view modes to ConfigEntity, where the entity system needs to provide an entity type.
Comment #32
tim.plunkett#1786990: [Module]Bundle is registered too late in WebTestBase::setUp() may help us here, just trying this out again.
EDIT: Turns out this wasn't actually committed when I posted this, when it fails I'll retest
Comment #34
tim.plunkettLOL wrong patch anyway
Comment #36
tim.plunkett#1778942: Discovery::getDefinition() / getDefinitions() : inconsistent return values for "no result" means that you can't assume that getDefinitions returns an array.
Comment #38
tim.plunkettConverted config_test and entity_test.
Comment #40
tstoecklerSorry if this was asked above, or if it is obvius, but is there any specific reason, that you're leaving part of the info ('bundles' and 'view modes', I think) in hook_entity_info()?
Comment #41
tim.plunkettThis addresses #40.
Comment #43
tim.plunkettStill known bugs, but let's see how much is fixed.
Comment #45
yched CreditAttribution: yched commentedI'm currently touring Quebec :-), unable to provide a real review, but two thoughts :
- given this use of processDefinition(), we're really gonna need a fix for #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator
- maybe this would deserve discussion in a followup, but we might want to split highly dynamic stuff like bundles and view modes into separate (static ?) methods, and out of entiyy info. Having to expose them in alter is cumbersome, and it's also where nasty cross-dependencies happens currently in D7 (list of bundles depending on, e.g., existing fields of a given type --> hell rebuild dependency chain)
Comment #46
tim.plunkettView modes are being tackled in #1043198: Convert view modes to ConfigEntity.
Here's a reroll after #1026616: Implement an entity render controller.
Working on converting 'test_entity', which constitues a large number of the remaining failures.
Comment #48
tim.plunkettThis needs clean-up, but I want to see what the bot thinks.
Comment #49
tim.plunkettThe breakpoint module has been since added to core.
Comment #51
tim.plunkettTypo in the breakpoint stuff, and missing the namespace for File
Comment #53
tim.plunkettFixed some more of my own mistakes.
Comment #55
tim.plunkettWOW. Note to self. Don't do that.
Comment #57
tim.plunkettLast patch for the night. The BC layer I included was making things too messy, so I removed it.
If only the test_entity wasn't so messy, 50% of this patch is dealing with that.
Comment #57.0
tim.plunkettadded blocker
Comment #58
tim.plunkettI updated the issue summary
Comment #60
tim.plunkettMissed config prefix, uri callback, and label callback.
I'm going to post a patch tomorrow without any of the test_entity fixes, to make it more reviewable.
Comment #62
tim.plunkett#60: drupal-1763974-60.patch queued for re-testing.
Comment #64
tim.plunkettAh! I needed NestedArray::mergeDeep() to properly merge the defaults in.
Also, converted the new contact category.
Comment #65
tim.plunkettHere is the above patch but without all of the field_test changes, it makes it much more reviewable.
Comment #67
tim.plunkettThis includes a fix for the upgrade paths that should be split into a new issue. Just testing here first.
Comment #69
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commented@nnotations on Entities !! Woot, we are doing Plain Old Java Object (POJO) !! Good direction, I remember wanted so much this feature back in the times where I started learning Drupal #1048838: Class Mapping from Drupal Entities to Flash Object. This is exciting :)
Just, why you guys didn't looked at doctrine ? see this page on annotations: http://docs.doctrine-project.org/projects/doctrine-common/en/latest/refe....
We started by hook_entity_info() which was nice to add metadata about the entity class (no more stdClass object!), we want now to switch to plugins (no more info array!), which is a blatant converting of _entity_info in annotations. All of these moves was improvements, so why not jump ahead and adopt doctrine ORM ? I bet if we are not doing this now, we will eventually do it in Drupal 9.
Comment #70
tim.plunkettPlease open a new issue, this is complex enough for now :)
Comment #71
tim.plunkettSo this now includes the fixes from:
Comment #71.0
tim.plunkettupdated issue summary
Comment #72
tim.plunkettWith the impending Views merge (#1805996: [META] Views in Drupal Core), I'm postponing this, since I'll have to convert that as well.
Comment #73
tstoecklerSince this patch is already involved enough (*awesome* job!!!, btw) I would recommend doing this in a follow-up, but we should open an issue to document the "Derivative" feature of entity types.
Is there a reason this is needed? If so, it seems the defaults aren't being applied correctly?!
It seems static properties, such as view modes, could just as well be provided directly in the Annotation, no? Since view modes are being removed from the info in #1043198: Convert view modes to ConfigEntity, it's probably not worth re-rolling for that.
So all in all, nothing that should hold up this patch, given that it's a bit of a pain to review, except that it now depends on two other issues. Once those are in, this should be RTBC.
Comment #74
tstoecklerCrosspost.
Comment #75
yched CreditAttribution: yched commentedI'd advocate to remove 'bundles' from hook_entity_info() as well, but that would probably be for a followup.
Comment #76
tim.plunkettOkay, so I'm going to reroll this each time a new config entity is added to core, but those are far enough along that its not fair to pull the rug out from under them. If they're not all done by the end of BADCamp, then too bad :)
Also, I need to rip out all of the derivative code from test_entity, it's just not worth it. The patch will get so much smaller...
Comment #77
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commented@tim: let's not make the plugin system the new golden hammer of Drupal.
Solutions are existing to remove Arrays of Doom. It is called Object Relational Maper.
I will open a new issue if you really wants to.
Comment #78
webchickIntroducing an ORM concept to Drupal is definitely a separate issue that needs its own separate discussion, and should not be tacked onto a fairly routine refactoring patch. So, yes.
Comment #79
catchThis shouldn't be postponed on the Views patch. It's OK for tim.plunkett to not work on it for a week, but it's not actually a dependency.
Also just to agree with webchick, please keep any ORM discussions in a separate issue.
Comment #80
tim.plunkett@Sylvain Lecoy, please never ever again describe any work that I am doing as a "golden hammer". This is twice now. You've been warned.
@catch, I could easily reroll Views for this in 5 minutes, but after further reflection I was more worried about pulling a fast one on all of the ConfigEntity conversion issues like image styles and vocabulary. If you think it's reasonable to just get this in and force them to convert themselves, I can wrap this patch up rather quickly.
Comment #81
catchHmm I need to actually review the patch properly before I can answer that question ;)
Comment #82
pounardI definitely understand you on that one. Nevertheless, and that's nothing against your own work (I won't engage in that path, I do respect what you do) I think the config is starting to be used a golden hammer pretty much everywhere in core at this point. I'm afraid that once all those patches will land, Drupal 8 will be slower than ever.
Comment #83
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commented@tim I actually do like you, and I wasn't even aware you written this patch so really don't take it personally. I am absolutely speaking about code, and that's it, nothing more, nothing less.
I am following the evolution of the plugin system and just noticed that I think it is conceptually wrong to make everything a plugin without worrying about the impact; Doing this will set the tone for the whole community and we have to consider the consequences.
I also understand the needing, and this issue is about to make a home made ORM, that's why I asked if we checked the possible use of Doctrine ORM before getting into the code. I created an issue as suggested: #1817778: Consider using Doctrine ORM for Entities.
Sylvain
Comment #84
tim.plunkettLooking at http://drupal.org/project/issues/search/drupal?text=convert&status[]=8&i..., image styles, vocabularies, and view modes are so close that I'd much prefer to reroll this than break those. There's a chance that might happen this weekend.
That will also give me a chance to fix the test_entity stuff.
So with that said, re-postponing with the intention of rerolling this by Tuesday at the latest.
Comment #85
tim.plunkettOkay, working on ripping out the test_entity craziness, here's an update that includes porting Views over just to see if I missed anything.
Comment #86
tim.plunkettOkay, here's a new approach for TestEntity.
Onto fixing CacheDecorator
Comment #88
tim.plunkettRunning into some trouble, here's a patch to fix more of those fails.
Comment #91
tim.plunkettFollow-ups:
Comment #92
andypostTim please provide a interdiff in newer patches to follow a changes
Comment #93
tim.plunkettWill do, but hopefully I won't need newer patches ;)
This is ready for real review and hopefully commit.
Comment #94
tim.plunkettThis still contains the fix from #1816916: Recursively merge in defaults
Changes include adding @todos for CacheDecorator, and porting ImageStyle.
Comment #95
tim.plunkettPostponing on #1816916: Recursively merge in defaults
Comment #96
tim.plunkettYay, all blockers are in!
And with help from neclimdul and EclipseGC, I resolved the caching parts too.
This contains a workaround for #1780396: Namespaces of disabled modules are registered during test runs, which is causing problems for several users. There is an @todo and the workaround is sane, so this should not be blocked on it.
Comment #98
tim.plunkettWell it applied when I submitted it 5 hours ago. The drop is always moving :)
I'll reroll when I get home
Comment #99
tim.plunkettIt was broken by #1812822: field_test_entity_info does not set a 'label' on the Test Entity
But since that code was deleted anyway, no problem.
Comment #100
damiankloip CreditAttribution: damiankloip commentedFirstly, this (massive) patch looks good to me. Nice work Tim!
Regarding the Views changes: Do we want to be changing ViewStorage to View? I think this might make things confusing for people. If someone, for example, tries to load views (not using views_get_view) they would then have an object called 'View'. ViewStorage is a lot more descriptive.
But then currently our entity type is 'view' and the entity class is ViewStorage. Which is probably also not ideal :)
I would maybe propose calling it ViewStorage and changing the entity type to view_storage. Thoughts?
Comment #101
tim.plunkettdawehner and I talked about this the other day.
If Views planned on providing more than one entity type, then the naming split would make sense.
But node.module's entity type is Node and user.module's entity type is User, why not call it View?
Comment #102
sun1) I agree that "ViewStorage" doesn't make much sense. "View" sounds fine to me.
2) I realize and apologize this is coming late, but I was not able to review this earlier.
Core\Entity
actually doesn't look right to me. The owner is not "core", but the Entity system. And the plugin type is not "Entity", but an entity "Type". Given where we're heading with the entity system, it looks like there's a very good chance that we'll see the need for other plugin types within the entity system. Thus, I think the plugin classes should all be:Entity\Type
e.g.,
Drupal\node\Plugin\Entity\Type\Node
Possibly laters...
Drupal\node\Plugin\Entity\Access\Node
, etc.pp. - hope that's sufficient to make sense.Comment #103
fubhy CreditAttribution: fubhy commentedI posted a suggestion for a possible follow-up: #1821746: Convert entity controllers into plugins
Comment #104
xjmsun's point 2 in #102 is worth discussing, but I'd suggest discussing it in a followup. @tim.plunkett says he'll file an issue for it in a bit.
Comment #105
fubhy CreditAttribution: fubhy commentedI /think/ that should be $this->discovery.
This doesn't have to be in the constructor and could instead be moved to the property itself. It's just an array with fixed, predefined values.
Comment #106
damiankloip CreditAttribution: damiankloip commented#105, I think this is fine, isn't $this->discovery just above? This is just the same pattern used on lots of other plugin managers. @tim.plunkett can confirm this.
Comment #107
tim.plunkettIt definitely shouldn't be $this->discovery, that bit us really hard in Views.
We could move that into a property above the constructor, but I don't think that's really necessary.
And yes, I'd REALLY prefer to leave that renaming bikeshed to a follow-up, I opened #1821846: Consider better naming conventions for plugin types owned by includes.
This is a big patch and getting it in sooner rather than later would cause less suffering for further conversions to entities.
Comment #108
EclipseGc CreditAttribution: EclipseGc commented@fubhy
The PluginManagerInterface implements DiscoveryInterface, FactoryInterface and MapperInterface specifically for the purposes of being passed around as $this. And in fact it's really important that you pass it as $this instead because the Manager proxies it's calls to the other classes, and this can get skipped if $this->discovery (or factory or mapper) are passed instead.
Hope that makes sense.
Eclipse
Comment #109
xjmI'm doing an in-the-weeds review of this patch atm. About 10% through...
Comment #110
sunIn-patch renames for #102. Passed all tests.
Comment #111
xjm@tim.plunkett asked me to take a close look at this. I managed to get through it in a little over an hour by giving myself several followup review tasks. ;) The following is completely unfiltered stream-of-xjm-consciousness, without having read the issue, because I don't have time to pare it down before core mentoring today. More later!
And here's the answer to my earlier question. (Can't update the previous snippet because dreditor is wigging out.) +1 for how clearly this is documented. I'll read over these docs closely on the next pass.
Sexy.
Okay, this guy seems to be the heart of the patch. I'll look at it more closely after core mentoring.
Aw boo, it doesn't detect the rename, I guess because the annotation is so large compared to the original class. Another thing to check closely on the next review.
Not quite English. Also the idea of an alter hook with no info hook, or an alter hook canonically defining something, is messing my brain. Can we at least clarify this to say something like "The collection of bundles for an entity type is added to the entity definition with
hook_entity_info_alter()
?" Finally, it still referencesnode_entity_info()
in the next sentence.This wasn't introduced here, I don't think, but it's as confusing as political comment deleted. Can we file a followup to properly document these classes with some explanation and @see and whatnot, and fix the labels and assertions so they freaking make sense?
Looks like this one is pretty big, I guess because node has a lot of conditional view modes based on the presence of other modules.
No it doesn't.
I remember some issue with @Translation back when we first added annotations; was it resolved? Does anyone know what I am talking about?
So one important thing to check in reviewing this patch is that all the keys from the old
hook_entity_info()
are being moved into either the appropriate annotation or the alter hook. I did for User and View (that rename seems natural when I type it in this sentence) but I'll skip it for the rest so I can get through this first pass before New Year's.Whoa man. WHOA.
So we can use
hook_entity_info_alter()
to alter on additional keys that aren't defined in the annotation? And bundles and view modes aren't defined in the annotation?Lovely. What happened to the form controller class key? Edit: Wait what? Form controller on a view? Looking around for the TARDIS or Delorean... Edit 2: lolwhat? Views used NodeFormController? Copypasta I guess. Who do they have working on that module, anyway? ;)
So, the list path was always a hacky workaround, but now we're killing it completely? Edit: Wait. We already completely killed it, no? We spun off http://drupal.org/node/1798540 as a followup. Is this just something we forgot to clean up in Views?
I am going crazy trying to see what the difference between these two lines is.
Edit: oh. space became and underscore. Good, but why?
Edit: Okay, looks like it's a thing. Do annotations not support spaces in the keys? Must not. I think these changes would be best reviewed with a local
git diff --staged --color-words
.Someone didn't rewrap after they did a search-and-replace. :)
Okay, so around Paris we split the view object into
ViewExecutable
andViewStorage
; the latter is back to justView
now? And it's defined with a plugin. Good golly.Edit: Got up to the ViewStorageController class description; that explains the rename for me.
Whoa man. Whoa.
Edit: Is this fixing #1783964: Allow entity types to provide menu items?
Comment #112
xjmAlso let's please please please protect this patch from reroll hell. :)
Comment #113
xjm@sun, the reason I suggested a followup issue on the renaming is because @tim.plunkett does not agree with the rename, and I'm not sure I do either. Rerolling to add a change you want that there isn't consensus on is kinda meh. =/
Comment #114
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedI am very sorry but I still feel uncomfortable with the use of the plugin system as a base for entities. I mean, entities are data objects, can't we just use annotations did right (@see #1801570-95: DX: Replace hook_route_info() with YAML files and RouteBuildEvent) instead of plugins ? They are pretty different in my head as plugins are primarily used to be swapped, like an JSON, AMF, or ATOM plugin for rendering for instance. But in any way for data objects.
Maybe an entity is now more than a DTO (Data Transfer Object), or VO (Value Object), or whatever ? I am very confused of the primary goal of an entity now. Sorry being arguing again but it is something that I don't understand, if someone can explain me in very simple sentences what are the goal of entities and plugins so I'll be able to embrace your vision and do proper review ?
Thanks
Comment #115
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedx-post
Comment #116
tim.plunkett1/11. I've chosen to not move view modes or bundles to the annotation at all, given how dynamic they can be, and issues like #1043198: Convert view modes to ConfigEntity #1782460: Allow to reference another entity type to define an entity type's bundles
5. Info-based alter hooks can now be paired with annotations, like hook_views_plugins_alter(), this isn't new.
6. Those Test Entity classes were hellish, and I didn't not spend time fixing the docs, agreed on the follow-up.
9. There was an @Translation bug with caching that has since been fixed I believe.
12. Yes, somehow Views ended up with Drupal\node\NodeFormController as it's form controller :D I just didn't move it over.
13/17 Additionally, Views still had the 'list path' key, which worked just fine, but is not agreed on yet. This conforms with how other things do it. I don't think that needs a separate issue.
14. And yes, annotations do not allow spaces in key names. That plus the docblocks account for the majority of the patch size.
---
And, there is no consensus on #102, so #99 is still the patch in question. See #1821846: Consider better naming conventions for plugin types owned by includes
Comment #117
xjmAlso, since I didn't mention it earlier, I am +1 on the approach of this path overall.
Comment #117.0
xjmAdded blockers
Comment #118
tim.plunkettOpened #1822000: Remove Drupal\field_test\Plugin\Entity\Type\TestEntity in favor of EntityTest as a follow-up and added a section to the issue summary
Comment #119
fagoI like where this is heading.
ad #114: This is not about making entity types swappable. You provide a new entity type by providing a new plugin, you implement an "entity plugin" if you want. That does not mean your entity type can be swapped out, but yes - the entity types are interchangable (to entity-generic code) as all implementations correspond to the EntityInterface.
That's problematic as it does away with the possibility to alter them. Imo both should be moved out of hook_entity_info anyway, so I'd suggest just adding a todo for that there.
This conversion is a bit awkward. It's even difficult to spot the difference when looking at the patch and will most likely result in a DX pain when switching betweeen d7 and d8: Wait, is it with or without underscore this time?
Is there a way to keep the old version even though we have annotations?
The Plugin manager informs the base system? I guess this docs need an update.
This should use the CacheDecorator. Looks like it doesn't for a good reason, so we should do it in a follow-up then.
I like this namespace as it provides for further Entity\* plugins
easy fix.
Comment needs a new line break now.
I'd have expected to find this class below Core\Entity as it's part of the entity system?
Comment #120
tim.plunkettI'm pretending your points are numbered. Not sure which one caused it to be CNW, since most will be in follow-ups.
Still could use more visibility and xjm isn't done her review, so marking back for the time being.
1) Yes, bundles will have to change. We can add a todo.
2) We are required to use underscores. It cannot really be helped.
3/6/7) Yes, docs need an update as per xjm's review
4) The absolutely cannot use the cache decorator, as it won't cache any of the work done in processDefinition(). I spent a good deal of time on that, and neclimdul and EclipseGC helped.
5/8) See the follow-up.
Comment #121
yched CreditAttribution: yched commentedAnother argh at #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator :-(. Have we lost all hope of fixing this ? processDefinition() being uncached is just useless and actually harmful.
Totally not for this patch to fix of course, but there should really be a comment about this on EntityManager::getDefinitions(), cause this cache code will definitely make people go "wtf?".
Comment #122
xjmI think documenting the space/underscore switch in the change notification will be sufficient for that concern. We've already started to move in the direction of "always underscores" in some other issues I remember from.... uh. summer 2011? Anyways I think we can standardize on "always underscores" more during feature freeze (but before code freeze) throughout core.
Also re: the newline, @tim.plunkett's patch is adding the newline that was previously missing. :)
Comment #123
xjmReview part 2.
Okay @fago is right; we need to make these key names use underscores to not confuse everyone to death. That can be a followup, but let's make sure we do.
Okay so I'm no expert on the plugin system but.... @return on a class? wha?
The interface interface! Also over 80 chars.
Typo: Deafaults.
All of these need to be labeled as optional if they provide a default or are otherwise not required, I think?
So this is where we tell it that
hook_entity_info_alter
adds to the entity info from the annotation?So bundles and view modes are given empty defaults here even though they're always defined in the alter hook?
Does this make the user bundle definition in
user_entity_info_alter()
redundant?This is a good point; we want the module's own "alters" to run before other, actual alters. I agree a @todo + followup issue might be a good way to address this.
Comment #124
xjmWorking on the docs a bit.
Comment #125
xjmFollowups:
Comment #126
xjmAlright, the attached:
EntityManager
andhook_entity_info_alter()
.Comment #127
fagoImprovements look good.
Imho the underscore issue is still problematic, but with everything being force to underscores it will finally become consistent for d8. So that's a plus that probably outweighs the d7 <-> d8 differences DX pain.
a Core owner should immediately imply core\lib\Drupal\Core, and in the case of Core Entity it should imply the directory core\lib\Drupal\Core\Plugin\Core\Entity. As a developer, when I come across node module's lib\Drupal\node\Plugin\Core\Entity, I should be able to deduce the above directory structure immediately.
Still, I'm concerned by this. I should find Entity* related code below Core\Entity. I've replied over at #1821846: Consider better naming conventions for plugin types owned by includes. Right now that location isn't picked up automatically anyway - so can we just move this somewhere over below Core\Entity for now? We do not have Core plugin implementations (e.g. see \Entity\Field\Type) lying below Core\Plugin either.
Comment #128
xjmI forgot to mention this earlier, and I hate to say it (Tim is going to be upset with me), but I really think some performance benchmarks for this would be good. It's quite the rearchitecture. I recognize that there are a lot of optimizations that can be done going forward, and I'm not saying we should say "no" to this patch based on a performance regression, but it would be good to know empirically what the effect is, if any.
Comment #129
xjmSo these are all our key renames. Edit: I updated the summary with this information.
Comment #129.0
xjmadded follow-ups
Comment #129.1
xjmUpdated issue summary.
Comment #130
xjmCan we add an inline comment explaining why the cache decorator can't be used?
Comment #131
tim.plunkettI don't think it needs to be documented, but maybe that's just me.
Paraphrasing neclimdul, the stock decorators are great for contrib and slapping one on if it works, but it shouldn't stop you from writing a more specialized manager.
To have been able to use CacheDecorator properly, we would have needed to completely override all of the methods from the base class, and add a second (currently hacky and controversial) "defaults" decorator.
Comment #132
pounardA one liner comment won't hurt thought.
Comment #133
xjmIMO anything that makes three different developers stop and go "oh but is this not like so?" deserves an inline comment. Could someone render #131 into something that works as a code comment? I don't entirely understand why that's the explanation for why the cache decorator is not used.
Comment #134
yched CreditAttribution: yched commentedFrom widgets / formatters as plugins, the typical overhead of the Plugins API is our inefficient class loader. Agreed that benchmarks would be a good thing, though.
Also, strongly disagree with "stock decorators are good enough for contrib".
Core is not the only place that needs to massage plugin definitions and cache them, and "Uncached processDefinition() / Factory($this) / Factory($this->discovery)" is currently a mess that fools every new person attempting to "move X as plugin". That's for #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator to solve, but as far as this issue is concerned, please let's add a comment on this cache() code.
Comment #135
Fabianx CreditAttribution: Fabianx commentedBenchmark (42 nodes on /node) hitting /node:
Comment #136
Fabianx CreditAttribution: Fabianx commentedTo explain #135:
There are regressions and improvements.
- == good, improvement, less time spent
positve == "bad", regression, more time spent
The same is true for memory.
The above second snapshot is the exact same as the image above.
The first part is just comparing core with core to make sure the chosen "base line" is sane.
Comment #137
tim.plunkett#119 suggested moving Drupal\Core\Plugin\Type\EntityManager to Drupal\Core\Entity\EntityManager, and I agree with that.
I also added a comment about the caching.
Comment #138
tim.plunkettWrong interdiff in #137, the patch is right.
Here's the right one.
Comment #139
xjmBoth changes in #137/#138 look good to me.
Comment #140
yched CreditAttribution: yched commentedFrom the interdiff :
So what's our standard ? leading '\' or not ?
Comment #141
tim.plunkettI only changed it in the lines I was adjusting, the rest is out of scope and can be a follow-up.
http://drupal.org/coding-standards/docs#namespaces
Comment #142
Fabianx CreditAttribution: Fabianx commentedJust for the record: I tested another scenario (more content types, more fields, some blocks) and had the same benchmark results.
The speedup comes mostly from the removing of calling "language" repeatedly from entity_get_info() in both cases.
Also entity_get_info gets in itself faster and more efficient.
Nice patch!
Comment #143
Fabianx CreditAttribution: Fabianx commentedTested and reviewed, improves performance, works fine => RTBC
Needs: drush rr or re-install, but that is okay in 8.x :-)
YES!
Comment #144
tim.plunkettRerolled after recent commits.
Comment #145.0
(not verified) CreditAttribution: commentedUpdated issue summary.
Comment #146
BerdirLooks generally OK to me. Haven't read through the patch in every detail but looked at the plugin implementation and a few examples. A few unfortunate things but most things that I noticed already have follow-up issues or are otherwise discussed and agreed upon.
Added these two as follow-ups:
- #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator
- #1821846: Consider better naming conventions for plugin types owned by includes
While not specifically relevant to entity types, both could possibly help to simplify and improve them.
Will trigger a re-test, testbot has thrown an exception like this in one test, looks like a fluke but should probably be investigated.
One thing about the view modes. Will apply and test that now to verify this.
Can totally be a follow-up, but there's no reason we can't just do a $info['node']['view_modes']['print'] ... here and then we won't need to initialize it.
Also, wondering if this has any effect on the order of view modes in the UI. E.g., book will now add it's print view mode before node adds the default and teaser view modes...
Ok, a bit confused by this then. Won't this override other view modes if they've been defined first?
Comment #147
Berdir#144: drupal-1763974-144.patch queued for re-testing.
Comment #148
BerdirYes, I can confirm that the print view mode does not show up in the UI. I think that needs to be fixed...
Comment #149
tim.plunkett#146 is why we have #1822458: Move dynamic parts (view modes, bundles) out of entity_info(), and will ideally be cleaned up completely by #1043198: Convert view modes to ConfigEntity
But, this is a perfectly reasonable thing to fix now.
Comment #150
sunIMHO, we need to at least resolve the basics of #1822458: Move dynamic parts (view modes, bundles) out of entity_info() here.
Otherwise, this patch presents a rather large regression for all modules that are extending other entity type modules in some way (and there's no guarantee that #1822458 will happen).
The standard way is to prepend a build operation before the alter operation; i.e.:
hook_entity_info_build()
hook_entity_info_alter()
The _build suffix is not necessarily required, we could as well just leave it out, and have hook_entity_info(&$types), or alternatively, hook_entity_info() + array merge deep $definitions + $return.
Comment #151
tim.plunkettI really would like to solve that problem in that issue. Otherwise, we have hook_module_implements_alter().
Considering that it's just view modes and bundles, and both are slated for drastic changes, I don't think that's necessary to hold up this patch.
Comment #152
BerdirThe view mode related changes look ok to me, except of this:
Looks like we lost that @todo here, was that intentational?
Right now, if you have Book module enabled, then "Print" will show up first in the list, before the default view modes. IMHO, that is good enough that we can live with it. We could use some array_unshift tricks or so to prepend the default view mode, but it's really not that important IMHO when we know that we already have a follow-up issue to deal with this. And as @timplunkett said in IRC, Field UI should probably sort that list alphabetically anyway.
Comment #153
tim.plunkettHere is a compromise for #150, in that it moves non-dynamic view modes and bundles into the annotation, and reserves hook_entity_info_alter() only for the dynamic ones.
The follow-up should be enough.
And yes, #152 is follow-up worth as well.
Comment #154
tim.plunkettAfter a discussion in IRC, I've reverted moving these alters:
Comment #155
xjmThe changes in #149 look correct.
Here's a full interdiff for #154 against that patch.
Comment #156
xjmThis needs to be updated now (moving docs for view modes and bundles back to the EntityManager, and then adding a note here about when bundles and view modes should be added here instead of in the annotation).
Comment #157
tim.plunkettDocs fixed.
Comment #159
tim.plunkettForgot the underscores in "custom settings".
Comment #161
tim.plunkettRerolled for EFQv2. No interdiff really, just moving stuff.
Comment #162
xjmFollowup issue:
#1827256: Document data_table entity info key
Comment #163
xjmI noticed that the main label for the entity type is translated, but the view mode and bundle labels are not. @tim.plunkett tested and this appears to be an annotations bug (the @Translation is not parsed properly in the nested arrays, and if we try to add
@Translation
on the bundle or view mode labels a notice is thrown). Let's file a followup issue for this.Comment #164
xjmAlrighty, @tim.plunkett is filing a followup for #163. I reviewed:
hook_entity_info()
implementations to verify that their keys were moved either to the annotation or tohook_entity_info_alter()
as appropriate.git diff --color-words
) to ensure they were all changed correctly.All the other points have either been addressed or have followup issues. I think this is RTBC.
Comment #165
webchickSo most of this patch replaces code like this:
With code like this:
...plus removing spaces in various array key names in favour of underscores instead, since this is required by the plugin system.
I'm not a huge fan of annotations, and don't see a tremendous DX improvement between the old code and new code since either way you have to copy/paste a bunch of crap and then read docs (this again is inherited from the plugin system). But what I do like is code like this:
...which seems like it will centralize and clean up things a heck of a lot better. Plus, as noted above, it does speed things up to change the discovery mechanism to plugin-based annotations stuff, and it also consolidates the code better, too.
One question I had was whether or not it made sense to have 500 comments above the EntityManager class definition, when you also have 500 comments down below above each of the individual properties. Seems like those could get out of sync pretty fast. xjm responded that this is needed because the code below will only define defaults, and not all of the possible properties. That's fair enough.
I also was wondering about
plugin.manager.entity
and whether that was consistent with what we named "plugin groups" elsewhere, but in grepping I see like "plugin.manager.views" already, so seems like this namespace is established.I did not have a chance to painstakingly check each _info() hook and make sure its properties were all carried over to plugin annotations, but I see xjm already did that (HUGELY appreciated, btw... tests would probably show any regressions here, but not necessarily, especially on the more "exotic" properties).
So, with that said... this looks great to me, and happy to commit it.... once it applies. D'oh.
Comment #166
tim.plunkettThe conflicts weren't too bad, just one @param and the actual change to config_test_entity_info().
Comment #167
webchickOK! Committed and pushed to 8.x!
I'm really sorry, too, because basically every patch in the RTBC queue atm is going to break because of this. :( But this lays really important groundwork for Views, CMI, and several other initiatives.
Comment #168
webchickOops. Almost did that right. :P
We need a change notice for this.
Comment #169
tim.plunkettAdded a change notice here: http://drupal.org/node/1827470
Comment #170
tstoecklerLooks good.
Still needs that follow-up for nested @Translation's. That's not introduced here, though, so marking fixed.
Comment #171
tim.plunkett#1827506: Translate bundle labels and view mode labels is the follow-up
Comment #171.0
tim.plunkettAdded more follow-ups
Comment #172
Gábor HojtsyI have several modules using config entities in the queue so just stumbled into this straight on. When converting my hook_entity_info() code, I stumbled into about 10 different annotation parse errors due to how quotes and non-quoted strings should be used on annotations, or how the class file need to use annotation classes for it to even work (which was not on the change notice, added a comment there). All-in-all the whole annotation structure is an obscure comment block in my IDE, it does not conform to PHP syntax at all obviously. My IDE naturally could not work out any "parse errors" ahead of time, requiring me to run my test suite over and over and over and over and over and over again to get the latest obscure error messages like 'constant id not defined' (without line number information), to track down the source of the error manually and go back to trial and error).
I think this is a significant setback in terms of DX unless there are a couple IDEs that support syntax colouring/checking in annotation comments and D8 wants to sponsor them so the developers need to buy/install those tools. (I searched the change records for mentions of annotations and have not found one specific for annotations to help me figure out best tools for working with them.)
Comment #173
neclimdulI think the "significant setback in terms of DX" is pretty subjective as through years of maintaining giant hooks if metadata I find them about the lowest DX possible.
I was able to find this about highlighting though:
http://docs.doctrine-project.org/projects/doctrine-common/en/latest/refe...
Comment #174
Gábor Hojtsy@neclimdul: I think IDEs have a good history of checking code, coloring syntax, etc. *outside* of comments. Whether the metadata is using PHP syntax like the rest of your code or in a different format/syntax in a comment that your IDE will not autocomplete or highlight or check syntax errors for, I see a significant step back in terms of developer experience. As per your link, the only documented editor to support even syntax highlight (not yet error checking or autocomplete I assume) is Eclipse then. Good to know.
Comment #175
sdboyer CreditAttribution: sdboyer commented/rant
stumbled across this today, too. it gets a big -1 from me. @Gábor Hojtsy's reasons in #172 bother me, but i *really* hate it from a namespace sanity perspective. that is, this, from the issue summary:
i mean, come on. this is ridiculous. we're gonna need to write a dictionary just so people can learn our namespace patterns. namespaces are supposed to have some internal sanity and pattern to them, and things like this make me relinquish that control. and the problem is getting worse, since we're sticking systems together like this now. ConfigEntities are now entities AND plugins AND config. terrifying.
@tim.plunkett has pointed me to #1821846: Consider better naming conventions for plugin types owned by includes as the place where we can fight out the namespacing issue, but really...we stuck two giant independent systems together in order to get reusability on one teensie part - annotations? really?
/rantoff
Comment #176
sunTBH, I'm not really fond of this change either, especially after experiencing the consequences in actual code.
We could have as well done
Drupal\node\Entity\Node
with a custom annotation:The concept also doesn't really seem to map to plugins. The multidimensional extension part is missing. What we're facing here is just a core application service that happens to allow implementations to register themselves. A simple, one-dimensional master-slave relation.
On that front, it doesn't even have to be annotations-based.
Perhaps the necessary (and possibly also overdue) conclusion to draw is that info hooks for core services should not be plugins. Core services have all the tools at hand that are necessary to build a custom solution without any overhead that is architecturally sound.
Comment #177
tim.plunkettI am moving this back to "fixed", because I do not think that rolling this patch back is the best solution.
----
If we decide to introduce non-"Plugin system" provided annotations, like @Entity, I think that is a welcome direction to take.
Likewise, if we decide to somehow loosen/remove the namespace restrictions introduced, I'd love to see some code to that effect.
As it stands, many info hooks are moving to annotations. Annotations are not a custom Drupalism, and IDEs will catch up sooner rather than later. And if desired, we could improve the error handling for improper annotation syntax, to aid in DX.
But none of those indicate a good reason to roll this back. That just sounds like three possible follow-ups.
Comment #177.0
tim.plunkettadded another follow-up
Comment #178
amateescu CreditAttribution: amateescu commentedI've created a followup to cleanup all references to the old class names. Most of the things in that patch are just nice to have (doxygen comments) but others are actually bugs.
#1828852: Update entity class names after the conversion to plugins
Comment #179
tstoecklerPlease take into account comments #24 through #30 of this issue, where this was already discussed to some extent.
DX-wise there are really two takes on this:
A: The DX overhead of the Plugin system can be avoided (see e.g. #176), and thus should be.
B: Because so many things in D8 will be Plugins the short-term DX overhead is actually a DX win in the long run because you only have to ever learn one thing.
As can be seen in #24 I was strongly in favor of A above but have since come to terms with this a bit and am torn between the two now. Just wanted to point out the two viewpoints and the previous discussion.
Comment #180
tstoecklerCrosspost.
While I'm at it, also referencing #1828616: AnnotatedClassDiscovery is not finding plugins in Drupal\(Core|Component)\COMPONENT_NAME\Plugin directory, which also related in terms of DX.
Comment #181
Sylvain Lecoy CreditAttribution: Sylvain Lecoy commentedWhile we are at it, it is my personnal opinion, but if we are changing it, then I see it like that:
Comment #182
EclipseGc CreditAttribution: EclipseGc commentedGuess I get to weigh in on this now, yay.
The plugin system maps to entities so precisely that it's not even funny. Comment to the contrary may have a point, but only in regard to this current patch (which was done on a smaller scale so that follow ups could be files without expanding the influence and size of this patch. That has proven to be a good approach to take since this actually got committed. Moving onward, and identifying areas of the existing annotations that could be slimmed down or removed in favor of asking the class (when appropriate) getting a standardized derivative handler to support entity bundles in place, and the various other minor issues here should all be follow ups.
I do NOT think the namespacing is likely to be anything but an education problem, and what's more on that topic, it's consistent, which is why the problem will ultimately be distilled down to relatively quick answers in the issue queue and irc. Will people immediately understand it? considering how often I've explained it, I sort of doubt that, but that doesn't change the fact that consistency in approach is probably a better plan than making entity a special snowflake. I've argued against this before, and will continue to do so for forever. In fact I made that argument on this very issue, please see #30.
Custom @Entity... I fail to see what that buys us if anything. A more robust argument needs to be made there before the topic can even be considered. @Sylvain, I don't believe our existing annotation parsing supports what you outlined, let's not argue over the benefits or detriments of such a system since we don't support it.
Eclipse
Comment #183
catchSorry I'm re-opening this.
I think it could've used more discussion before it went in, and I still don't feel like #2 was addressed.
Comment #184
tim.plunkettThe ECK guys were fine with this, and hook_entity_info_alter() still exists.
Comment #185
EclipseGc CreditAttribution: EclipseGc commentedI'm not sure why ECK is even a question here. Having a derivative handler that is two levels deep is actually very very simple and solves both configurable entity types as well as the potential bundles that reside on them. I actually have a mockup of this code existing in one of my sandboxes on d.o using ctools plugins instead.
http://drupal.org/sandbox/eclipsegc/1326258
My children (derivative) handler only attempts to support individual entity_types but it's just two nested foreach loops instead of one foreach loop in order to accomodate it. Quite straight forward really.
Eclipse
Comment #186
sunThe critical problem I have with this is still #150:
As long as we need to invoke hooks to add properties to the entity info,
hook_entity_info_alter()
is a clear, architectural abuse. Alter hooks are for altering previously collected data, not for adding.The moment you start adding things in alter hooks, you break the alter hook for other modules and introduce a huge interdependency and hook invocation order nightmare.
It's add vs. edit. That simple.
Comment #187
yched CreditAttribution: yched commentedThe current situation where bundles and view modes are added in hook_entity_info_alter is only temporary, as been said in #146 - #149.
Bundles and view modes *need* to get out of entity_info() to begin with, and so should any other dynamic part if any - see #1822458-6: Move dynamic parts (view modes, bundles) out of entity_info() for why.
This being said, and in retrospect, I can't really say I'm fully convinced moving entity types to the Plugin API was a good idea.
The net effect of this move is : "a $node is a plugin instance of the Entity plugin type, with plugin_id 'node'".
I can make sense of "this $image_effect is a plugin instance of the ImageEffect plugin type, with plugin_id 'resize'".
Same with input filters, field types, views row handlers...
But "a node is a plugin" ? Now I find it much harder to explain what the Plugin API should be used for or not.
Significantly enough, after turning Entity types into plugins, the patch makes a very limited use of the Plugin API itself :
- the only actual call is PluginTypeManager::getDefinitions(), in entity_get_info()
- that is, no use of createInstance() / getInstance() to create plugin instances, EntityManager declares a DefaultFactory but does not use it - $entity objects are created through
new $class
in EntityStorageController::create()So, it seems we only had a case to use the nifty *discovery* mechanism (and associated annotations reader) that's included in the plugin API, not really the plugin API itself.
Comment #188
fmizzell CreditAttribution: fmizzell commented@EclipseGc: Funny! You ask why ECK is relevant and then you solve the problem that this patch has regarding ECK in your answer.. so I guess you do agree that it is relevant after all!
I am currently working on a little code that allows for dynamic entity type declarations using the derivative decorator, and saving the dynamic entity type definition in config (I believe that is the exact approach used in eclipse's sandbox). So in relation to ECK all that it is needed on top of this patch is derivatives discovery in the Entity manager.
Comment #189
BerdirAbout the create() part. Dries disliked the fact that we do have a *Storage*Controller::create() method quite a bit in the entity uuid issue, where we extended it to generate the Uuid. So we can move that part to the Plugin system maybe?
The problem is that the create() method is *not* the same thing as just instantiating a class. It's creating a new entity that will be permamently saved, defining default values (like a UUD) and so on. When loading an existing entity, we just instantiate the class. We do't want to generate a new UUID in that case :)
So I'm not sure how that would map to plugins?
Comment #190
fagoI'd like to add method that allows fields to define a default value, or something similar. Then, the UUID generation can be just the default value. (Relevant issue: #1777956: Provide a way to define default values for entity fields).
Comment #191
tim.plunkett#189, #190: That would make #1831264: Use the Entity manager to create new controller instances even better.
Comment #192
EclipseGc CreditAttribution: EclipseGc commented@fmizzell,
I wasn't really trying to imply that ECK was irrelevant, just that the plugin system makes doing it very easy, so we shouldn't worry about it. :-)
@sun,
Views modes should arguably move off the entity info anyway (perhaps in favor of a simple, repeatable CMI prefix?) and bundles should be handled once we add derivative processing to the discovery.
Comment #193
yched CreditAttribution: yched commentedNononono ! As stated a couple comments above, bundles, or any other dynamic (= depending on config) parts need to get out of entity_info - see #1822458: Move dynamic parts (view modes, bundles) out of entity_info().
Comment #194
sunPlease understand that, with regard to #186, I don't really care what's in or out of entity info.
My point is that we can happily refactor stuff, and happily refactor stuff temporarily, but when doing so, we need to retain fundamental architectural concepts (build vs. alter). Otherwise, we're causing major breakage down the line, especially considering that contrib is slowly starting to get into porting business. The fundamental concept of build/alter separations is what enables contrib — breaking it breaks contrib.
Comment #195
yched CreditAttribution: yched commentedregarding my #187 above :
I was wrong, nodes are not plugins with the current code.
The current code actually does not deal with plugins at all, but only uses the discovery mechanism included in Plugin API, no actual plugin type is defined and no actual plugin is ever instantiated (that part of my comment was correct).
#1831264: Use the Entity manager to create new controller instances shows what could be an actual use of Plugins on top of those discovered annotation - the plugins being the different 'entity controllers' (form controller, render controller...). As I try to point in comment #7 over there, that's an uncommon pattern for the Plugin API, but as far as I'm concerned, dunno, why not.
Comment #196
tim.plunkettAFAIK, using "the Plugin system" is currently the only way to use annotated classes. Really all I'm after is AnnotatedClassDiscovery.
For an annotated class to be a true plugin, it needs to implement PluginInspectionInterface, which the entity types do not.
I'm not really concerned about that disconnect, but if others are, then we need a standalone "AnnotatedClassInterface" and means of instantiating them in the exact same way.
Comment #197
yched CreditAttribution: yched commentedNot AFAIK, Plugin API itself provides no constraints on plugin classes, and there is no PluginInterface . I'd say the use of plugin API is qualified by the use of a PluginManagerInterface object. This is the interface that exposes a model between definitions and plugin classes / instantiated plugins, which is different from the one used here.
It seems the current code could very well use a [annotations + alter + cache] discovery stack on its own, placed in the DIC, without a PluginManager whose methods are actually not used. Well, at least that could be done once #1764278: Run PluginManagerBase::processDefinition() in a ProcessDecorator is fixed.
Or, if we go for #1831264-7: Use the Entity manager to create new controller instances, with a "manager" object that explicitly does not implement PluginManagerInterface, which is useless and misleading here. That is, something, conceptually close to Plugin API, but not Plugin API because it is not Plugin API that is being used here - or at least not PluginManagerInterface. If properly explained in EntityManager ("this class uses concepts similar to the Plugin API, but does not implement Plugin API's PluginManagerInterface because our use case differs in this and that"), that would make total sense and be totally valid IMO, and much less confusing / misleading than the current state.
Comment #198
tim.plunkettNo, there are no constraints, but these are the only plugins to not extend PluginBase, which implements PluginInspectionInterface.
That said, I'd be okay with something like #197.
Comment #199
plachI think @sun's point in #186 (reiterated in #194) is not receiving the proper consideration. Let me cite a snippet of a discussion @Berdir and I had on #1188388-258: Entity translation UI in core:
Basically we identified exactly what @sun's talking about and came to the same conclusions. For this specific issue the matter is not whether we should rollback or not entity types as plugins or whether they make sense. The point is that currently we have a serious regression in our capability of providing new features to entities. In D7 Entity Translation is able to support core entity types because it implements
hook_entity_info()
on their behalf, now this is not possible anymore since relying onhook_entity_info_alter()
for this is a recipe for disaster.Comment #200
tim.plunkettI think we should go with the patch in #186 as a stopgap, and then open a follow-up to decouple the ability to provide annotated classes from the Plugin system.
Can we get some docblocks here? Otherwise this is fine by me.
Comment #201
plachWorks for me too. Should we add back the docs for
hook_entity_info()
?Comment #202
tim.plunkettI think it should mostly point at EntityManager, and clarify that it should only be used for dynamic data, nothing static (usually wrapped in module_exists()).
Comment #203
neclimdulDiscussing changing the plugin system in an issue about converting entity info sounds like the wrong thing. It sounds like sun has some real concern with something in the AnnotationDiscovery but I'm having trouble separating 200 comments worth of conversion from the plugin issue. Can we open an issue for it with a summary of the problem?
Also, lets not pollute a working system with work arounds to support one of things. Write a InfoHookHack decorator, document it, grep for it and fix until its gone. In fact, here you go.
Comment #204
Fabianx CreditAttribution: Fabianx commentedDoesn't this remove the AlterDecorator used above?
Comment #205
Berdir@Fabianx: Thought that too at the first look but it's actually copied and then changed, see those lines.
Comment #206
tstoecklerIf you look closely, you'll see that it is actually copied, and not renamed, hence, the original AlterDecorator is left in tact. I stumbled upon that too, at first.
I don't know whether it makes sense to discuss implementation details as this is going to be a temporary measure anyway, but I think maybe turning the existing
HookDiscovery
into a decorator might be less code.Either way, we should retain the usual expectation of an info hook that you *return* the info you're building instead of altering it in by reference. Given that the idea is to define view modes and such for existing, i.e. already defined entity types, we should use NestedArray::mergeDeep() for the merging of the two sources of info.
Comment #207
neclimdulNo I don't think it makes sense to discuss that. Its temporary and specifically designed like that. I'm curious why you're suggesting tweaking it since it passes tests and this is a temporary measure like you said. I think we should go ahead and move on to fixing the actual problem you guys are having.
Comment #208
tstoecklerWell at least the second point mentioned in #206 needs fixing IMO. An info hook that takes $info by reference is no info hook in my book. As said, about the implementation I don't really care.
Comment #209
sun@tstoeckler: You need a new book then. ;)
Passing thus far collected data to subsequent info hooks is a pattern that is implemented by more sophisticated info hooks in core; for example, hook_theme(). Whether you add to a passed in variable or return something that will be merged deeply afterwards doesn't really matter.
The only aspect that is important is that info hooks add to something, and only alter hooks manipulate what has been added.
Since this appears to be a temporary decorator, we're good to go here.
That said, I don't really understand how exactly we're going to deal with these kind of plugin/metadata dependency problems later on.
Comment #210
neclimdul@sun I hear your concern. I don't understand the problem so lets open a follow up with a summary of the plugin problem you ran into here and we'll work on it.
Comment #211
plachIMO we still need to restore at very least a basic documentation stub for
hook_entity_info()
, given also that its signature has changed.Comment #212
BerdirIn regards to the IDE discussion: Looks like Netbeans 7.3 will have support for annotations, composer *and* twig syntax :)
https://blogs.oracle.com/netbeansphp/entry/netbeans_7_3_beta2_is
Comment #213
Jose Reyero CreditAttribution: Jose Reyero commentedHi, I've just stumbled into this whole thread, late as usual, following the WTF - Change record - Issue tracker path.
I don't know whether there's an IDE or not for this, but this just doesn't compile in my mind.
So we don't have code and comments anymore, it's all the same thing, right?
(Now the stupid question: Can I add comments into the Annotations?)
I thought we were up to cleaner code and better standards, really. The fact that PHP or Symphony or (I don't know where annotations come from) has this awful feature doesn't mean we need to use it.
+1 for full rollback of this awful thing.
Comment #214
tim.plunkett#213, this is not the first issue that uses annotations. Rolling back this issue will not remove them. And being rude doesn't accomplish much either.
Comment #215
mitchell CreditAttribution: mitchell commentedConfig follow-up (#2 ~ #8): #1838676: Support dynamic entity type management.
Comment #216
Jose Reyero CreditAttribution: Jose Reyero commented@tim.plunkett,
Sorry if that looks rude to you, I'll look for the right issue... (If we've ever properly discussed that)... or create a new one.
Comment #217
webchickI think it's worth a separate issue to discuss the DX of annotations and how we might improve it. My observation from this issue (which is the first time annotations were really "in your face" for the "90%" developers) is that everyone who was NOT involved in the original annotations/plugins work but has tried to use annotations as a developer has found the experience extremely unpleasant, due to a number of factors (lack of syntax highlighting, lack of syntax checking, ambiguity around how to comment things within annotations, etc), all of which seem valid to me.
At BADCamp when I raised this with Tim, Kris, etc. they mentioned some possible mitigation strategies, such as trying to reduce the amount of info stored in annotations, and having those basically link off to classes/functions/whatever that are proper PHP that store the remaining metadata (this of course has other issues, like having to look 15 different places for all the info about your entity). So let's have a proper discussion about it elsewhere and see what (if anything) we can do.
Regarding the patch itself, I'm not sure how I can commit something with "Hack" in the name, cos it's totally not clear to a casual reader what that is doing. Seems like it should be InfoBCWrapperDecorator or something?
Comment #218
sunRenamed to InfoHookDecorator. Added Entity API docs. No reason to hold this stop-gap fix up any further.
Comment #219
tim.plunkettRTBC +1.
Comment #220
webchickCommitted and pushed to 8.x.
Comment #222
effulgentsia CreditAttribution: effulgentsia commentedFollow up to enable the rest of PluginManagerInterface, not just discovery: #1867228: Make EntityTypeManager provide an entity factory
Comment #223
effulgentsia CreditAttribution: effulgentsia commented#218 was committed with the message:
Yet the doc for hook_entity_info_alter() that was added with that same commit says:
And currently, in HEAD, we have no non-test modules using hook_entity_info(), but custom_block.module and menu.module both using hook_entity_info_alter() to do what the doc above says should be done with hook_entity_info().
So, do we believe the commit message, remove hook_entity_info(), and change the hook_entity_info_alter() doc? Or do we ignore the commit message, believe the doc, and change custom_block.module and menu.module to use hook_entity_info()?
Asking because of #1970360-39: Entities should define URI templates and standard links.
Comment #224
plachThe latter please :)
Quoting from #1968970-21: Standardize module-provided entity info documentation and clean-up the @EntityType annotation:
Comment #225
tim.plunkettExcept by that reasoning, ET should use hook_entity_info() to add to defined entity types.
Comment #225.0
tim.plunkettAdded a followup issue.