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

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
CommentFileSizeAuthor
#218 drupal8.entity-info.218.patch8.62 KBsun
#203 drupal8.entity-info.203.patch6.9 KBneclimdul
#186 drupal8.entity-info.186.patch4.81 KBsun
#166 drupal-1763974-166.patch190.17 KBtim.plunkett
#161 drupal-1763974-160.patch191.01 KBtim.plunkett
#159 drupal-1763974-159.patch193.21 KBtim.plunkett
#157 drupal-1763974-157.patch193.11 KBtim.plunkett
#157 interdiff.txt7.62 KBtim.plunkett
#155 150-154.txt7.64 KBxjm
#154 drupal-1763974-154.patch193.64 KBtim.plunkett
#153 drupal-1763974-153.patch194.48 KBtim.plunkett
#153 interdiff.txt9.58 KBtim.plunkett
#149 interdiff.txt5.56 KBtim.plunkett
#149 drupal-1763974-150.patch193.74 KBtim.plunkett
#144 drupal-1763974-144.patch193.52 KBtim.plunkett
#138 interdiff.txt7.22 KBtim.plunkett
#137 drupal-1763974-137.patch193.56 KBtim.plunkett
#137 interdiff.txt4.2 KBtim.plunkett
#136 xhprof-1763974-diff.png52.73 KBFabianx
#126 entity-1763974-126.patch193.48 KBxjm
#126 interidff-99-126.txt21.77 KBxjm
#110 drupal-1763974-99.patch191.75 KBsun
#99 drupal-1763974-99.patch191.75 KBtim.plunkett
#96 drupal-1763974-96.patch191.65 KBtim.plunkett
#96 interdiff.txt4.2 KBtim.plunkett
#94 drupal-1763974-94.patch192.62 KBtim.plunkett
#94 interdiff.txt6.1 KBtim.plunkett
#88 drupal-1763974-88.patch188.6 KBtim.plunkett
#86 drupal-1763974-86.patch185.87 KBtim.plunkett
#85 drupal-1763974-85.patch348.16 KBtim.plunkett
#71 drupal-1763974-71.patch325.07 KBtim.plunkett
#67 drupal-1763974-67.patch324.33 KBtim.plunkett
#65 drupal-1763974-65-REVIEW-THIS-do-not-test.patch140.22 KBtim.plunkett
#64 drupal-1763974-64.patch323.37 KBtim.plunkett
#60 drupal-1763974-60.patch317.33 KBtim.plunkett
#57 drupal-1763974-57.patch313.25 KBtim.plunkett
#55 drupal-1763974-55.patch283.78 KBtim.plunkett
#55 interdiff.txt1.02 KBtim.plunkett
#53 drupal-1763974-53.patch283.78 KBtim.plunkett
#51 drupal-1763974-51.patch283.58 KBtim.plunkett
#49 drupal-1763974-49.patch250.46 KBtim.plunkett
#48 drupal-1763974-48.patch238.94 KBtim.plunkett
#46 drupal-1763974-46.patch60.44 KBtim.plunkett
#43 drupal-1763974-43.patch59.05 KBtim.plunkett
#41 drupal-1763974-41.patch51.52 KBtim.plunkett
#38 interdiff.txt8.69 KBtim.plunkett
#38 drupal-1763974-38.patch39.22 KBtim.plunkett
#36 drupal-1763974-36.patch30.54 KBtim.plunkett
#36 interdiff.txt776 bytestim.plunkett
#34 drupal-1763974-34.patch30.44 KBtim.plunkett
#32 drupal-1798944-14.patch9.35 KBtim.plunkett
#15 annotations-1763974-15.patch30.18 KBtim.plunkett
#12 annotations-1763974-12.patch30.08 KBtim.plunkett
#11 annotations-1763974-11.patch22.55 KBtim.plunkett
#9 annotations-1763974-9.patch16.86 KBtim.plunkett
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Issue tags: +VDC

Yes please.

catch’s picture

I'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?

tim.plunkett’s picture

sun’s picture

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

scor’s picture

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

yched’s picture

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

fago’s picture

Yes, #6 should work just fine I think.

tim.plunkett’s picture

Priority: Normal » Major

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

tim.plunkett’s picture

Assigned: Unassigned » tim.plunkett
Status: Active » Needs review
FileSize
16.86 KB

Here 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:

  • id
  • label
  • controller class
  • form controller class
  • base table
  • revision table
  • uri callback
  • fieldable
  • entity keys
  • static cache
  • field cache

"entity class" should be removed.

The following keys have been left in entity_get_info():

  • bundle keys
  • bundles
  • view modes
  • translation

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.

Status: Needs review » Needs work

The last submitted patch, annotations-1763974-9.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
22.55 KB

Whoops, I messed up the Node namespaces.
While the test ran I also converted User and Comment.

tim.plunkett’s picture

FileSize
30.08 KB

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

plach’s picture

+++ b/core/modules/book/book.pages.inc
@@ -5,7 +5,7 @@
-use Drupal\node\Node;
+use Drupal\node\Core\Entity\Node;

+++ b/core/modules/book/lib/Drupal/book/Tests/BookTest.php
@@ -7,7 +7,7 @@
-use Drupal\node\Node;
+use Drupal\node\Core\Entity\Node;

There are still some wrong namespaces here and there. A search/replace should do the trick :)

Status: Needs review » Needs work

The last submitted patch, annotations-1763974-12.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
30.18 KB

Argh! Thanks @plach.

plach’s picture

Status: Needs work » Needs review
Issue tags: -Needs issue summary update

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

tim.plunkett’s picture

Agreed, I didn't even notice how terse that was.

Status: Needs review » Needs work

The last submitted patch, annotations-1763974-15.patch, failed testing.

tim.plunkett’s picture

Status: Needs review » Postponed
Issue tags: +Needs issue summary update
rbayliss’s picture

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

tim.plunkett’s picture

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

pounard’s picture

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

tim.plunkett’s picture

@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

tstoeckler’s picture

Since 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 in Drupal\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.

pounard’s picture

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

tstoeckler’s picture

So I think basing this kind of decision on DX only probably isn't a good idea: being restrictive is by design against DX.

I 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 and Drupa\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".

pounard’s picture

Indeed 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?

tstoeckler’s picture

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

pounard’s picture

Yes, ideally I'd prefer to shorten the namespaces too. I'm not against refactoring the annotation discovery throught this issue thought.

EclipseGc’s picture

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

tim.plunkett’s picture

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

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
9.35 KB

#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

Status: Needs review » Needs work

The last submitted patch, drupal-1798944-14.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
30.44 KB

LOL wrong patch anyway

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-34.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
776 bytes
30.54 KB

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-36.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
39.22 KB
8.69 KB

Converted config_test and entity_test.

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-38.patch, failed testing.

tstoeckler’s picture

Sorry 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()?

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
51.52 KB

This addresses #40.

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-41.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
59.05 KB

Still known bugs, but let's see how much is fixed.

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-43.patch, failed testing.

yched’s picture

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
60.44 KB

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

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-46.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
238.94 KB

This needs clean-up, but I want to see what the bot thinks.

tim.plunkett’s picture

FileSize
250.46 KB

The breakpoint module has been since added to core.

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-49.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
283.58 KB

Typo in the breakpoint stuff, and missing the namespace for File

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-51.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
283.78 KB

Fixed some more of my own mistakes.

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-53.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
1.02 KB
283.78 KB

WOW. Note to self. Don't do that.

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-55.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
313.25 KB

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

tim.plunkett’s picture

Issue summary: View changes

added blocker

tim.plunkett’s picture

I updated the issue summary

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-57.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
317.33 KB

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

Status: Needs review » Needs work
Issue tags: -Entity system, -VDC, -Plugin system

The last submitted patch, drupal-1763974-60.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review

#60: drupal-1763974-60.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +Entity system, +VDC, +Plugin system

The last submitted patch, drupal-1763974-60.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
323.37 KB

Ah! I needed NestedArray::mergeDeep() to properly merge the defaults in.

Also, converted the new contact category.

tim.plunkett’s picture

Here is the above patch but without all of the field_test changes, it makes it much more reviewable.

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-64.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
324.33 KB

This includes a fix for the upgrade paths that should be split into a new issue. Just testing here first.

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-67.patch, failed testing.

Sylvain Lecoy’s picture

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

tim.plunkett’s picture

Please open a new issue, this is complex enough for now :)

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
325.07 KB
tim.plunkett’s picture

Issue summary: View changes

updated issue summary

tim.plunkett’s picture

Status: Needs review » Postponed

With the impending Views merge (#1805996: [META] Views in Drupal Core), I'm postponing this, since I'll have to convert that as well.

tstoeckler’s picture

Status: Postponed » Needs review
+++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.php
@@ -0,0 +1,163 @@
+    $this->discovery = new ModuleDecorator(new AlterDecorator(new DerivativeDiscoveryDecorator(new AnnotatedClassDiscovery('Core', 'Entity')), 'entity_info'));

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

+++ b/core/modules/book/book.module
@@ -253,7 +253,10 @@ function book_admin_paths() {
-  $info['node']['view modes'] += array(
+  if (!isset($info['node']['view_modes'])) {
+    $info['node']['view_modes'] = array();
+  }
+  $info['node']['view_modes'] += array(

Is there a reason this is needed? If so, it seems the defaults aren't being applied correctly?!

+++ b/core/modules/comment/comment.module
@@ -98,40 +98,18 @@ function comment_help($path, $arg) {
+  $info['comment']['view_modes'] = array(
+    'full' => array(
+      'label' => t('Full comment'),
+      'custom settings' => FALSE,
     ),

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.

tstoeckler’s picture

Status: Needs review » Postponed

Crosspost.

yched’s picture

I'd advocate to remove 'bundles' from hook_entity_info() as well, but that would probably be for a followup.

tim.plunkett’s picture

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

Sylvain Lecoy’s picture

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

webchick’s picture

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

catch’s picture

Status: Postponed » Needs work

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

tim.plunkett’s picture

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

catch’s picture

Hmm I need to actually review the patch properly before I can answer that question ;)

pounard’s picture

please never ever again describe any work that I am doing as a "golden hammer". This is twice now. You've been warned.

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

Sylvain Lecoy’s picture

@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

tim.plunkett’s picture

Status: Needs work » Postponed

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

tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
348.16 KB

Okay, working on ripping out the test_entity craziness, here's an update that includes porting Views over just to see if I missed anything.

tim.plunkett’s picture

FileSize
185.87 KB

Okay, here's a new approach for TestEntity.

Onto fixing CacheDecorator

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-86.patch, failed testing.

tim.plunkett’s picture

FileSize
188.6 KB

Running into some trouble, here's a patch to fix more of those fails.

andypost’s picture

Tim please provide a interdiff in newer patches to follow a changes

tim.plunkett’s picture

Will do, but hopefully I won't need newer patches ;)
This is ready for real review and hopefully commit.

tim.plunkett’s picture

FileSize
6.1 KB
192.62 KB

This still contains the fix from #1816916: Recursively merge in defaults

Changes include adding @todos for CacheDecorator, and porting ImageStyle.

tim.plunkett’s picture

Status: Needs review » Postponed
tim.plunkett’s picture

Status: Postponed » Needs review
FileSize
4.2 KB
191.65 KB

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

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-96.patch, failed testing.

tim.plunkett’s picture

Well it applied when I submitted it 5 hours ago. The drop is always moving :)
I'll reroll when I get home

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
191.75 KB

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

damiankloip’s picture

Status: Needs review » Postponed

Firstly, 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?

tim.plunkett’s picture

Status: Postponed » Needs review

dawehner 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?

sun’s picture

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

fubhy’s picture

I posted a suggestion for a possible follow-up: #1821746: Convert entity controllers into plugins

xjm’s picture

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

fubhy’s picture

+++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
@@ -0,0 +1,227 @@
+    $this->factory = new DefaultFactory($this);

I /think/ that should be $this->discovery.

+++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
@@ -0,0 +1,227 @@
+
+    $this->defaults = array(
+      'class' => 'Drupal\Core\Entity\Entity',
+      'controller_class' => 'Drupal\Core\Entity\DatabaseStorageController',
+      'list_controller_class' => 'Drupal\Core\Entity\EntityListController',
+      'render_controller_class' => 'Drupal\Core\Entity\EntityRenderController',
+      'form_controller_class' => array(
+        'default' => 'Drupal\Core\Entity\EntityFormController',
+      ),
+      'fieldable' => FALSE,
+      'static_cache' => TRUE,
+      'field_cache' => TRUE,
+      'bundles' => array(),
+      'view_modes' => array(),
+      'entity_keys' => array(
+        'revision' => '',
+        'bundle' => '',
+      ),
+      'translation' => array(),
+    );

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.

damiankloip’s picture

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

tim.plunkett’s picture

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

EclipseGc’s picture

@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

xjm’s picture

I'm doing an in-the-weeds review of this patch atm. About 10% through...

sun’s picture

FileSize
191.75 KB

In-patch renames for #102. Passed all tests.

xjm’s picture

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

  1. +++ b/core/includes/entity.api.phpundefined
    @@ -11,232 +11,65 @@
      * Alter the entity info.
    ...
    + * In addition, the following properties should be added here:
    

    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.

  2. +++ b/core/includes/entity.incundefined
    @@ -19,12 +19,10 @@
     function entity_get_info($entity_type = NULL) {
    -  $language_interface = language(LANGUAGE_TYPE_INTERFACE);
    -
       // Use the advanced drupal_static() pattern, since this is called very often.
       static $drupal_static_fast;
       if (!isset($drupal_static_fast)) {
    @@ -32,61 +30,8 @@ function entity_get_info($entity_type = NULL) {
    
    @@ -32,61 +30,8 @@ function entity_get_info($entity_type = NULL) {
       }
       $entity_info = &$drupal_static_fast['entity_info'];
     
    -  // hook_entity_info() includes translated strings, so each language is cached
    -  // separately.
    -  $langcode = $language_interface->langcode;
    -
       if (empty($entity_info)) {
    -    if ($cache = cache()->get("entity_info:$langcode")) {
    -      $entity_info = $cache->data;
    -    }
    -    else {
    -      $entity_info = module_invoke_all('entity_info');
    -      // Merge in default values.
    -      foreach ($entity_info as $name => $data) {
    -        $entity_info[$name] += array(
    -          'fieldable' => FALSE,
    -          'entity class' => 'Drupal\Core\Entity\Entity',
    -          'controller class' => 'Drupal\Core\Entity\DatabaseStorageController',
    -          'list controller class' => 'Drupal\Core\Entity\EntityListController',
    -          'render controller class' => 'Drupal\Core\Entity\EntityRenderController',
    -          'form controller class' => array(
    -            'default' => 'Drupal\Core\Entity\EntityFormController',
    -          ),
    -          'static cache' => TRUE,
    -          'field cache' => TRUE,
    -          'bundles' => array(),
    -          'view modes' => array(),
    -          'entity keys' => array(),
    -          'translation' => array(),
    -        );
    -        $entity_info[$name]['entity keys'] += array(
    -          'revision' => '',
    -          'bundle' => '',
    -        );
    -        foreach ($entity_info[$name]['view modes'] as $view_mode => $view_mode_info) {
    -          $entity_info[$name]['view modes'][$view_mode] += array(
    -            'custom settings' => FALSE,
    -          );
    -        }
    -        // If no bundle key is provided, assume a single bundle, named after
    -        // the entity type.
    -        if (empty($entity_info[$name]['entity keys']['bundle']) && empty($entity_info[$name]['bundles'])) {
    -          $entity_info[$name]['bundles'] = array($name => array('label' => $entity_info[$name]['label']));
    -        }
    -        // Prepare entity schema fields SQL info for
    -        // Drupal\Core\Entity\DatabaseStorageControllerInterface::buildQuery().
    -        if (isset($entity_info[$name]['base table'])) {
    -          $entity_info[$name]['schema_fields_sql']['base table'] = drupal_schema_fields_sql($entity_info[$name]['base table']);
    -          if (isset($entity_info[$name]['revision table'])) {
    -            $entity_info[$name]['schema_fields_sql']['revision table'] = drupal_schema_fields_sql($entity_info[$name]['revision table']);
    -          }
    -        }
    -      }
    -      // Let other modules alter the entity info.
    -      drupal_alter('entity_info', $entity_info);
    -      cache()->set("entity_info:$langcode", $entity_info, CacheBackendInterface::CACHE_PERMANENT, array('entity_info' => TRUE));
    -    }
    +    $entity_info = drupal_container()->get('plugin.manager.entity')->getDefinitions();
       }
    

    Sexy.

  3. +++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
    @@ -0,0 +1,227 @@
    + * Contains Drupal\Core\Plugin\Type\EntityManager.
    

    Okay, this guy seems to be the heart of the patch. I'll look at it more closely after core mentoring.

  4. +++ b/core/modules/contact/contact.moduleundefined
    @@ -201,35 +201,12 @@ function contact_config_import_delete($name, $new_config, $old_config) {
    diff --git a/core/modules/contact/lib/Drupal/contact/Category.php b/core/modules/contact/lib/Drupal/contact/Category.php
    
    diff --git a/core/modules/contact/lib/Drupal/contact/Category.php b/core/modules/contact/lib/Drupal/contact/Category.php
    deleted file mode 100644
    index 1a34479..0000000
    
    +++ /dev/nullundefined
    @@ -1,59 +0,0 @@
    diff --git a/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Category.php b/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Category.php
    
    diff --git a/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Category.php b/core/modules/contact/lib/Drupal/contact/Plugin/Core/Entity/Category.php
    new file mode 100644
    

    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.

  5. +++ b/core/modules/field/field.attach.incundefined
    @@ -60,19 +60,19 @@
      * The Field Attach API uses the concept of bundles: the set of fields for a
      * given entity is defined on a per-bundle basis. The collection of bundles for
    ...
    + * an entity type is defined its hook_entity_info_alter() implementation. For
      * instance, node_entity_info() exposes each node type as its own bundle. This
      * means that the set of fields of a node is determined by the node type. The
      * Field API reads the bundle name for a given entity from a particular property
    ...
    + * of the entity object, and hook_entity_info_alter() defines which property to
    + * use. For instance, node_entity_info_alter() specifies:
    + * @code $info['entity_keys']['bundle'] = 'type'@endcode
      * This indicates that for a particular node object, the bundle name can be
    

    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 references node_entity_info() in the next sentence.

  6. +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Core/Entity/BundleKeyTestEntity.phpundefined
    @@ -0,0 +1,35 @@
    + * Test entity class.
    
    +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Core/Entity/BundleTestEntity.phpundefined
    @@ -0,0 +1,35 @@
    + * Test entity class.
    
    +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Core/Entity/CacheableTestEntity.phpundefined
    @@ -0,0 +1,32 @@
    + * Test entity class.
    
    +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Core/Entity/LabelCallbackTestEntity.phpundefined
    @@ -0,0 +1,35 @@
    + * Test entity class.
    
    +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Core/Entity/LabelTestEntity.phpundefined
    @@ -0,0 +1,38 @@
    + * Test entity class.
    ...
    + *   id = "test_entity_label",
    + *   label = @Translation("Test Entity label"),
    ...
    +class LabelTestEntity extends TestEntity {
    
    +++ b/core/modules/field/tests/modules/field_test/lib/Drupal/field_test/Plugin/Core/Entity/NoLabelTestEntity.phpundefined
    @@ -0,0 +1,37 @@
    + *   id = "test_entity_no_label",
    + *   label = @Translation("Test Entity without label"),
    ...
    +class NoLabelTestEntity extends TestEntity {
    

    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?

  7. +++ b/core/modules/node/node.moduleundefined
    @@ -191,59 +191,36 @@ function node_cron() {
    +function node_entity_info_alter(&$info) {
    

    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.

  8. +++ b/core/modules/system/tests/modules/entity_cache_test_dependency/lib/Drupal/entity_cache_test_dependency/Plugin/Core/Entity/EntityCacheTest.phpundefined
    @@ -0,0 +1,23 @@
    + * Defines the node entity class.
    ...
    +class EntityCacheTest extends Entity {
    

    No it doesn't.

  9. +++ b/core/modules/system/tests/modules/entity_test/lib/Drupal/entity_test/Plugin/Core/Entity/EntityTest.phpundefined
    @@ -2,15 +2,34 @@
    + *   label = @Translation("Test entity"),
    

    I remember some issue with @Translation back when we first added annotations; was it resolved? Does anyone know what I am talking about?

  10. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Term.phpundefined
    @@ -2,16 +2,38 @@
      * Defines the taxonomy term entity.
    + *
    + * @Plugin(
    + *   id = "taxonomy_term",
    
    +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Vocabulary.phpundefined
    @@ -2,15 +2,32 @@
      * Defines the taxonomy vocabulary entity.
    + *
    + * @Plugin(
    + *   id = "taxonomy_vocabulary",
    
    +++ b/core/modules/taxonomy/taxonomy.moduleundefined
    @@ -106,42 +106,21 @@ function taxonomy_permission() {
    -function taxonomy_entity_info() {
    ...
    +function taxonomy_entity_info_alter(&$info) {
    

    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.

  11. +++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
    @@ -2,15 +2,37 @@
    + * @Plugin(
    + *   id = "user",
    + *   label = @Translation("User"),
    + *   module = "user",
    + *   controller_class = "Drupal\user\UserStorageController",
    + *   render_controller_class = "Drupal\user\UserRenderController",
    + *   form_controller_class = {
    + *     "profile" = "Drupal\user\ProfileFormController",
    + *     "register" = "Drupal\user\RegisterFormController"
    + *   },
    + *   base_table = "users",
    + *   uri_callback = "user_uri",
    + *   label_callback = "user_label",
    + *   fieldable = TRUE,
    + *   entity_keys = {
    + *     "id" = "uid",
    + *     "uuid" = "uuid"
    + *   }
    + * )
    
    +++ b/core/modules/user/user.moduleundefined
    @@ -139,42 +139,20 @@ function user_theme() {
    - * Implements hook_entity_info().
    + * Implements hook_entity_info_alter().
    ...
    -function user_entity_info() {
    ...
    +function user_entity_info_alter(&$info) {
    +  $info['user']['bundles']['user'] = array(
    +    'label' => t('User'),
    +    'admin' => array(
    +      'path' => 'admin/config/people/accounts',
    +      'access arguments' => array('administer users'),
    +    ),
    +  );
    +  $info['user']['view_modes'] = array(
    +    'full' => array(
    +      'label' => t('User account'),
    +      'custom settings' => FALSE,
         ),
    

    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?

  12. +++ b/core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.phpundefined
    @@ -2,18 +2,37 @@
    - * Defines a ViewStorage configuration entity class.
    + * Defines a View configuration entity class.
    + *
    + * @Plugin(
    + *   id = "view",
    + *   label = @Translation("View"),
    + *   module = "views",
    + *   controller_class = "Drupal\views\ViewStorageController",
    + *   list_controller_class = "Drupal\views_ui\ViewListController",
    + *   config_prefix = "views.view",
    + *   fieldable = FALSE,
    + *   entity_keys = {
    + *     "id" = "name",
    + *     "label" = "human_name",
    + *     "uuid" = "uuid"
    + *   }
    + * )
    
    +++ b/core/modules/views/views.moduleundefined
    @@ -20,33 +20,6 @@
    -function views_entity_info() {
    -  $return = array(
    -    'view' => array(
    -      'label' => t('View'),
    -      'entity class' => 'Drupal\views\ViewStorage',
    -      'controller class' => 'Drupal\views\ViewStorageController',
    -      'list controller class' => 'Drupal\views_ui\ViewListController',
    -      'list path' => 'admin/structure/views',
    -      'form controller class' => array(
    -        'default' => 'Drupal\node\NodeFormController',
    -      ),
    -      'config prefix' => 'views.view',
    -      'fieldable' => FALSE,
    -      'entity keys' => array(
    -        'id' => 'name',
    -        'label' => 'human_name',
    -        'uuid' => 'uuid',
    -      ),
    -    ),
    -  );
    

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

  13. +++ b/core/modules/views/lib/Drupal/views/Plugin/Core/Entity/View.phpundefined
    @@ -171,9 +190,8 @@ public function getModule() {
       public function uri() {
    -    $info = $this->entityInfo();
         return array(
    -      'path' => $info['list path'],
    +      'path' => 'admin/structure/views/view/' . $this->id(),
         );
    
    +++ b/core/modules/views/views_ui/views_ui.moduleundefined
    @@ -747,7 +747,7 @@ function views_ui_ajax_callback(ViewExecutable $view, $op) {
    -    drupal_goto($entity_info['list path']);
    +    drupal_goto('admin/structure/views');
    

    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?

  14. +++ b/core/modules/views/lib/Drupal/views/Plugin/views/wizard/WizardPluginBase.phpundefined
    @@ -119,7 +119,7 @@ public function __construct(array $configuration, $plugin_id, DiscoveryInterface
    -      if (isset($entity_info['base table']) && $this->base_table == $entity_info['base table']) {
    +      if (isset($entity_info['base_table']) && $this->base_table == $entity_info['base_table']) {
    
    @@ -742,7 +742,7 @@ protected function default_display_filters_user(array $form, array &$form_state)
    -      $bundle_key = $this->entity_info['bundle keys']['bundle'];
    +      $bundle_key = $this->entity_info['bundle_keys']['bundle'];
    

    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.

  15. +++ b/core/modules/views/lib/Drupal/views/Tests/ModuleTest.phpundefined
    @@ -196,7 +196,7 @@ function testStatusFunctions() {
    -   *   An array of Drupal\views\ViewStorage objects to create an options array
    +   *   An array of Drupal\views\Plugin\Core\Entity\View objects to create an options array
    

    Someone didn't rewrap after they did a search-and-replace. :)

  16. +++ b/core/modules/views/lib/Drupal/views/ViewStorageController.phpundefined
    @@ -12,7 +12,7 @@
    - * Defines the storage controller class for ViewStorage entities.
    + * Defines the storage controller class for View entities.
    
    +++ b/core/modules/views/views.moduleundefined
    @@ -1526,46 +1499,46 @@ function views_get_views_as_options($views_only = FALSE, $filter = 'all', $exclu
    -function views_disable_view(ViewStorage $view) {
    +function views_disable_view(View $view) {
    

    Okay, so around Paris we split the view object into ViewExecutable and ViewStorage; the latter is back to just View now? And it's defined with a plugin. Good golly.

    Edit: Got up to the ViewStorageController class description; that explains the rename for me.

  17. +++ b/core/modules/views/views_ui/lib/Drupal/views_ui/ViewListController.phpundefined
    @@ -83,7 +83,7 @@ public function buildHeader() {
    -    $path = $uri['path'] . '/view/' . $view->id();
    +    $path = $uri['path'];
    

    Whoa man. Whoa.

    Edit: Is this fixing #1783964: Allow entity types to provide menu items?

xjm’s picture

Issue tags: +Avoid commit conflicts

Also let's please please please protect this patch from reroll hell. :)

xjm’s picture

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

Sylvain Lecoy’s picture

Issue tags: -Avoid commit conflicts

I 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

Sylvain Lecoy’s picture

Issue tags: +Avoid commit conflicts

x-post

tim.plunkett’s picture

1/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

xjm’s picture

Also, since I didn't mention it earlier, I am +1 on the approach of this path overall.

xjm’s picture

Issue summary: View changes

Added blockers

tim.plunkett’s picture

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

fago’s picture

Status: Needs review » Needs work

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

+++ b/core/includes/entity.api.php
@@ -11,232 +11,65 @@
+ *
+ * In addition, the following properties should be added here:
+ * - bundles: An array describing all bundles for this object type. Keys are

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.

+++ b/core/lib/Drupal/Core/Entity/EntityFieldQuery.php
@@ -1126,7 +1126,7 @@ public function joinPropertyData(Select $select_query, $entity_type, $base_table
       $entity_info = entity_get_info($entity_type);
-      $id_key = $entity_info['entity keys']['id'];
+      $id_key = $entity_info['entity_keys']['id'];

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?

+++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.php
@@ -0,0 +1,227 @@
+ * Inform the base system and the Field API about one or more entity types.
+ *
+ * Inform the system about one or more entity types (i.e., object types that
+ * can be loaded via entity_load() and, optionally, to which fields can be
+ * attached).

The Plugin manager informs the base system? I guess this docs need an update.

+++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.php
@@ -0,0 +1,227 @@
+  public function getDefinitions() {
+    if ($cache = cache($this->cacheBin)->get($this->cacheKey)) {
+      return $cache->data;
+    }

This should use the CacheDecorator. Looks like it doesn't for a good reason, so we should do it in a follow-up then.

+++ b/core/modules/book/book.admin.inc
@@ -5,7 +5,7 @@
+use Drupal\node\Plugin\Entity\Type\Node;

I like this namespace as it provides for further Entity\* plugins

+++ b/core/modules/system/tests/modules/theme_page_test/theme_page_test.module
@@ -18,4 +18,4 @@ function theme_page_test_system_theme_info() {
-}
\ No newline at end of file
+}

easy fix.

+++ b/core/modules/views/lib/Drupal/views/Tests/ModuleTest.php
@@ -196,7 +196,7 @@ function testStatusFunctions() {
-   *   An array of Drupal\views\ViewStorage objects to create an options array
+   *   An array of Drupal\views\Plugin\Entity\Type\View objects to create an options array

Comment needs a new line break now.

+++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.php
@@ -0,0 +1,227 @@
+ * Contains Drupal\Core\Plugin\Type\EntityManager.

I'd have expected to find this class below Core\Entity as it's part of the entity system?

tim.plunkett’s picture

Status: Needs work » Needs review

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

yched’s picture

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.

Another 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?".

xjm’s picture

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

xjm’s picture

Review part 2.

  1. +++ b/core/includes/entity.api.phpundefined
    @@ -11,232 +11,65 @@
    + *     - path: the path of the bundle's main administration page, as defined
    + *       in hook_menu(). If the path includes a placeholder for the bundle,
    + *       the 'bundle argument', 'bundle helper' and 'real path' keys below
    + *       are required.
    + *     - bundle argument: The position of the placeholder in 'path', if any.
    + *     - real path: The actual path (no placeholder) of the bundle's main
    + *       administration page. This will be used to generate links.
    + *     - access callback: As in hook_menu(). 'user_access' will be assumed if
    + *       no value is provided.
    ...
    + *   - custom settings: A boolean specifying whether the view mode should by
    

    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.

  2. +++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
    @@ -0,0 +1,227 @@
    + * @return
    + *   An array whose keys are entity type names and whose values identify
    + *   properties of those types that the system needs to know about:
    ...
    +class EntityManager extends PluginManagerBase {
    

    Okay so I'm no expert on the plugin system but.... @return on a class? wha?

  3. +++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
    @@ -0,0 +1,227 @@
    + *     The class has to implement the
    + *     Drupal\Core\Entity\EntityStorageControllerInterface interface. Leave blank
    

    The interface interface! Also over 80 chars.

    +++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
    @@ -0,0 +1,227 @@
    + *     the entities. Deafaults to Drupal\Core\Entity\EntityRenderController.
    

    Typo: Deafaults.

  4. +++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
    @@ -0,0 +1,227 @@
    + *   - static_cache: (used by Drupal\Core\Entity\DatabaseStorageController)
    ...
    + *   - fieldable: Set to TRUE if you want your entity type to be fieldable.
    

    All of these need to be labeled as optional if they provide a default or are otherwise not required, I think?

  5. +++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
    @@ -0,0 +1,227 @@
    +    $this->discovery = new AlterDecorator(new AnnotatedClassDiscovery('Core', 'Entity'), 'entity_info');
    

    So this is where we tell it that hook_entity_info_alter adds to the entity info from the annotation?

  6. +++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
    @@ -0,0 +1,227 @@
    +      'bundles' => array(),
    +      'view_modes' => array(),
    

    So bundles and view modes are given empty defaults here even though they're always defined in the alter hook?

  7. +++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.phpundefined
    @@ -0,0 +1,227 @@
    +    // If no bundle key is provided, assume a single bundle, named after
    +    // the entity type.
    +    if (empty($definition['entity_keys']['bundle']) && empty($definition['bundles'])) {
    +      $definition['bundles'] = array($plugin_id => array('label' => $definition['label']));
    

    Does this make the user bundle definition in user_entity_info_alter() redundant?

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

xjm’s picture

Assigned: Unassigned » xjm

Working on the docs a bit.

xjm’s picture

Alright, the attached:

fago’s picture

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

+++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.php
@@ -0,0 +1,227 @@
+ * Contains Drupal\Core\Plugin\Type\EntityManager.

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.

xjm’s picture

Assigned: xjm » Unassigned

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

xjm’s picture

+++ b/core/includes/entity.api.phpundefined
@@ -11,232 +11,72 @@
- *   - entity class: The name of the entity class, defaults to
...
- *   - controller class: The name of the class that is used to load the objects.
...
- *   - render controller class: The name of the class that is used to render
...
- *   - form controller class: An associative array where the keys are the names
...
- *   - list controller class: The name of the class that is used to provide
...
- *   - base table: (used by Drupal\Core\Entity\DatabaseStorageController) The
...
- *   - static cache: (used by Drupal\Core\Entity\DatabaseStorageController)
...
- *   - field cache: (used by Field API loading and saving of field data) FALSE
...
- *   - uri callback: A function taking an entity as argument and returning the
...
- *   - label callback: (optional) A function taking an entity and optional langcode
...
- *   - entity keys: An array describing how the Field API can extract the
...
- *   - bundle keys: An array describing how the Field API can extract the
...
- *   - view modes: An array describing the view modes for the entity type. View

So these are all our key renames. Edit: I updated the summary with this information.

xjm’s picture

Issue summary: View changes

added follow-ups

xjm’s picture

Issue summary: View changes

Updated issue summary.

xjm’s picture

+++ b/core/lib/Drupal/Core/Plugin/Type/EntityManager.php
@@ -0,0 +1,227 @@
+  public function getDefinitions() {
+    if ($cache = cache($this->cacheBin)->get($this->cacheKey)) {
+      return $cache->data;
+    }

This should use the CacheDecorator. Looks like it doesn't for a good reason, so we should do it in a follow-up then.

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.

Another argh at #1764278: Replace PluginManagerBase::processDefinition() with a DefaultsDecorator :-(. 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?".

Can we add an inline comment explaining why the cache decorator can't be used?

tim.plunkett’s picture

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

pounard’s picture

A one liner comment won't hurt thought.

xjm’s picture

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

yched’s picture

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

Fabianx’s picture

Benchmark (42 nodes on /node) hitting /node:

=== core..core-1763974--core compared (5089824b9b7be..5089834c44cf8):

wt  : 298,812|298,950|138|0.0%
cpu : 300,018|300,018|0|0.0%
mu  : 11,074,648|11,074,648|0|0.0%
pmu : 11,375,424|11,375,424|0|0.0%


=== core..core-1763974--126 compared (5089824b9b7be..508985603d513):

wt  : 298,812|292,688|-6,124|-2.0%
cpu : 300,018|292,018|-8,000|-2.7%
mu  : 11,074,648|11,087,000|12,352|0.1%
pmu : 11,375,424|11,385,216|9,792|0.1%
Fabianx’s picture

FileSize
52.73 KB

To explain #135:

xhprof-1763974-diff.png

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.

tim.plunkett’s picture

FileSize
4.2 KB
193.56 KB

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

tim.plunkett’s picture

FileSize
7.22 KB

Wrong interdiff in #137, the patch is right.
Here's the right one.

xjm’s picture

Both changes in #137/#138 look good to me.

yched’s picture

From the interdiff :

- * @see Drupal\Core\Plugin\Type\EntityManager
+ * @see \Drupal\Core\Entity\EntityManager
  * @see Drupal\Core\Entity\EntityStorageControllerInterface

So what's our standard ? leading '\' or not ?

tim.plunkett’s picture

I 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

Fabianx’s picture

Just 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!

Fabianx’s picture

Status: Needs review » Reviewed & tested by the community

Tested and reviewed, improves performance, works fine => RTBC

Needs: drush rr or re-install, but that is okay in 8.x :-)

YES!

tim.plunkett’s picture

FileSize
193.52 KB

Rerolled after recent commits.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, drupal-1763974-144.patch, failed testing.

Anonymous’s picture

Issue summary: View changes

Updated issue summary.

Berdir’s picture

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

rmdir(/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/files/php/service_container/c99151a773f22affd23884450f2920f37ee5a1c709770bbc4c14c4569139e5feb.php): No such file or directory	Warning	file.inc	1782
drupal_rmdir()	
rmdir(/var/lib/drupaltestbot/sites/default/files/checkout/sites/default/files/php/service_container):

One thing about the view modes. Will apply and test that now to verify this.

+++ b/core/modules/book/book.moduleundefined
@@ -253,7 +253,10 @@ function book_admin_paths() {
   // Add the 'Print' view mode for nodes.
-  $info['node']['view modes'] += array(
+  if (!isset($info['node']['view_modes'])) {
+    $info['node']['view_modes'] = array();
+  }
+  $info['node']['view_modes'] += array(
     'print' => array(
       'label' => t('Print'),
       'custom settings' => FALSE,

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

+++ b/core/modules/node/node.moduleundefined
@@ -191,59 +191,36 @@ function node_cron() {
+  $info['node']['view_modes'] = array(
+    'full' => array(
+      'label' => t('Full content'),
+      'custom settings' => FALSE,
+    ),
+    'teaser' => array(
+      'label' => t('Teaser'),
+      'custom settings' => TRUE,
+    ),
+    'rss' => array(
+      'label' => t('RSS'),
+      'custom settings' => FALSE,

Ok, a bit confused by this then. Won't this override other view modes if they've been defined first?

Berdir’s picture

Status: Needs work » Needs review

#144: drupal-1763974-144.patch queued for re-testing.

Berdir’s picture

Status: Needs review » Needs work

Yes, I can confirm that the print view mode does not show up in the UI. I think that needs to be fixed...

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
193.74 KB
5.56 KB

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

sun’s picture

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

tim.plunkett’s picture

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

Berdir’s picture

The view mode related changes look ok to me, except of this:

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -112,12 +112,9 @@ function taxonomy_entity_info_alter(&$info) {
-  $info['taxonomy_term']['view_modes'] = array(
-    // @todo View mode for display as a field (when attached to nodes etc).
-    'full' => array(
-      'label' => t('Taxonomy term page'),
-      'custom settings' => FALSE,
-    ),
+  $info['taxonomy_term']['view_modes']['full'] = array(
+    'label' => t('Taxonomy term page'),
+    'custom settings' => FALSE,

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.

tim.plunkett’s picture

FileSize
9.58 KB
194.48 KB

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

tim.plunkett’s picture

FileSize
193.64 KB

After a discussion in IRC, I've reverted moving these alters:

+++ b/core/modules/language/language.moduleundefined
@@ -172,6 +172,16 @@ function language_permission() {
 /**
+ * Implements hook_entity_info_alter().
+ */
+function language_entity_info_alter(&$info) {
+  // Add a translation handler for fields if the node module is enabled.
+  if (module_exists('node')) {
+    $info['node']['translation']['node'] = TRUE;
+  }
+}

+++ b/core/modules/node/node.moduleundefined
@@ -191,75 +191,14 @@ function node_cron() {
-  // Add a translation handler for fields if the language module is enabled.
-  if (module_exists('language')) {
-    $return['node']['translation']['node'] = TRUE;
-  }
-
-  // Search integration is provided by node.module, so search-related
-  // view modes for nodes are defined here and not in search.module.
-  if (module_exists('search')) {
-    $return['node']['view modes'] += array(
-      'search_index' => array(
-        'label' => t('Search index'),
-        'custom settings' => FALSE,
-      ),
-      'search_result' => array(
-        'label' => t('Search result'),
-        'custom settings' => FALSE,
-      ),
-    );

+++ b/core/modules/search/search.moduleundefined
@@ -134,6 +134,22 @@ function search_permission() {
 /**
+ * Implements hook_entity_info_alter().
+ */
+function search_entity_info_alter(&$info) {
+  if (module_exists('node')) {
+    $info['node']['view_modes']['search_index'] = array(
+      'label' => t('Search index'),
+      'custom settings' => FALSE,
+    );
+    $info['node']['view_modes']['search_result'] = array(
+      'label' => t('Search result'),
+      'custom settings' => FALSE,
+    );
+  }
+}
xjm’s picture

FileSize
7.64 KB

The changes in #149 look correct.

Here's a full interdiff for #154 against that patch.

xjm’s picture

Status: Needs review » Needs work
+++ b/core/includes/entity.api.phpundefined
@@ -11,232 +11,72 @@
+ * In addition, the following properties should be added here:
+ * - bundles: An array describing all bundles for this object type. Keys are
+ *   bundle machine names, as found in the objects' 'bundle' property
+ *   (defined in the 'entity_keys' entry for the entity type in the
+ *   EntityManager). Elements:
+ *   - label: The human-readable name of the bundle.
+ *   - uri_callback: The same as the 'uri_callback' key defined for the entity
+ *     type in the EntityManager, but for the bundle only. When determining
+ *     the URI of an entity, if a 'uri_callback' is defined for both the
+ *     entity type and the bundle, the one for the bundle is used.
+ *   - admin: An array of information that allows Field UI pages to attach
+ *     themselves to the existing administration pages for the bundle.
+ *     Elements:
+ *     - path: the path of the bundle's main administration page, as defined
+ *       in hook_menu(). If the path includes a placeholder for the bundle,
+ *       the 'bundle argument', 'bundle helper' and 'real path' keys below
+ *       are required.
+ *     - bundle argument: The position of the placeholder in 'path', if any.
+ *     - real path: The actual path (no placeholder) of the bundle's main
+ *       administration page. This will be used to generate links.
+ *     - access callback: As in hook_menu(). 'user_access' will be assumed if
+ *       no value is provided.
+ *     - access arguments: As in hook_menu().
+ * - view_modes: An array describing the view modes for the entity type. View
+ *   modes let entities be displayed differently depending on the context.
+ *   For instance, a node can be displayed differently on its own page
+ *   ('full' mode), on the home page or taxonomy listings ('teaser' mode), or
+ *   in an RSS feed ('rss' mode). Modules taking part in the display of the
+ *   entity (notably the Field API) can adjust their behavior depending on
+ *   the requested view mode. An additional 'default' view mode is available
+ *   for all entity types. This view mode is not intended for actual entity
+ *   display, but holds default display settings. For each available view
+ *   mode, administrators can configure whether it should use its own set of
+ *   field display settings, or just replicate the settings of the 'default'
+ *   view mode, thus reducing the amount of display configurations to keep
+ *   track of. Keys of the array are view mode names. Each view mode is
+ *   described by an array with the following key/value pairs:
+ *   - label: The human-readable name of the view mode.
+ *   - custom settings: A boolean specifying whether the view mode should by
+ *     default use its own custom field display settings. If FALSE, entities
+ *     displayed in this view mode will reuse the 'default' display settings
+ *     by default (e.g. right after the module exposing the view mode is
+ *     enabled), but administrators can later use the Field UI to apply custom
+ *     display settings specific to the view mode.
+ *
+ * @param array $entity_info
+ *   An associative array of all entity type definitions, keyed by the entity
+ *   type name.
+ *
+ * @see \Drupal\Core\Entity\Entity
+ * @see \Drupal\Core\Entity\EntityManager
+ * @see entity_get_info()
+ *
+ * @todo Allow a module to add its bundles and view modes before other modules

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
7.62 KB
193.11 KB

Docs fixed.

Status: Needs review » Needs work

The last submitted patch, drupal-1763974-157.patch, failed testing.

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
193.21 KB

Forgot the underscores in "custom settings".

tim.plunkett’s picture

FileSize
191.01 KB

Rerolled for EFQv2. No interdiff really, just moving stuff.

xjm’s picture

xjm’s picture

+++ b/core/modules/user/lib/Drupal/user/Plugin/Core/Entity/User.phpundefined
@@ -2,15 +2,52 @@
+ * @Plugin(
...
+ *   label = @Translation("User"),
...
+ *   bundles = {
...
+ *       "label" = "User",
...
+ *   view_modes = {
...
+ *       "label" = "User account",

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

xjm’s picture

Status: Needs review » Reviewed & tested by the community

Alrighty, @tim.plunkett is filing a followup for #163. I reviewed:

  • All the removed hook_entity_info() implementations to verify that their keys were moved either to the annotation or to hook_entity_info_alter() as appropriate.
  • All the changed keynames (with 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.

webchick’s picture

Status: Reviewed & tested by the community » Needs work

So most of this patch replaces code like this:

 /**
- * Implements hook_entity_info().
- */
-function breakpoint_entity_info() {
-  // Breakpoint.
-  $types['breakpoint'] = array(
-    'label' => 'Breakpoint',
-    'entity class' => 'Drupal\breakpoint\Breakpoint',
-    'controller class' => 'Drupal\Core\Config\Entity\ConfigStorageController',
-    'config prefix' => 'breakpoint.breakpoint',
-    'entity keys' => array(
-      'id' => 'id',
-      'label' => 'label',
-      'uuid' => 'uuid',
-    ),
-  );
-
-  // Breakpoint group.
-  $types['breakpoint_group'] = array(
-    'label' => 'Breakpoint group',
-    'entity class' => 'Drupal\breakpoint\BreakpointGroup',
-    'controller class' => 'Drupal\Core\Config\Entity\ConfigStorageController',
-    'config prefix' => 'breakpoint.breakpoint_group',
-    'entity keys' => array(
-      'id' => 'id',
-      'label' => 'label',
-      'uuid' => 'uuid',
-    ),
-  );
-
-  return $types;
-}

With code like this:

 /**
  * Defines the Breakpoint entity.
+ *
+ * @Plugin(
+ *   id = "breakpoint",
+ *   label = @Translation("Breakpoint"),
+ *   module = "breakpoint",
+ *   controller_class = "Drupal\Core\Config\Entity\ConfigStorageController",
+ *   config_prefix = "breakpoint.breakpoint",
+ *   entity_keys = {
+ *     "id" = "id",
+ *     "label" = "label",
+ *     "uuid" = "uuid"
+ *   }
+ * )
  */
 class Breakpoint extends ConfigEntityBase {

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

-namespace Drupal\breakpoint;
+namespace Drupal\breakpoint\Plugin\Core\Entity;
 

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

tim.plunkett’s picture

Status: Needs work » Needs review
FileSize
190.17 KB

The conflicts weren't too bad, just one @param and the actual change to config_test_entity_info().

webchick’s picture

Title: Convert entity type info into plugins » Change notice: Convert entity type info into plugins
Status: Needs review » Fixed
Issue tags: +Needs change record

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

webchick’s picture

Priority: Major » Critical
Status: Fixed » Active

Oops. Almost did that right. :P

We need a change notice for this.

tim.plunkett’s picture

Status: Active » Needs review
Issue tags: -Avoid commit conflicts

Added a change notice here: http://drupal.org/node/1827470

tstoeckler’s picture

Priority: Critical » Major
Status: Needs review » Fixed

Looks good.
Still needs that follow-up for nested @Translation's. That's not introduced here, though, so marking fixed.

tim.plunkett’s picture

Title: Change notice: Convert entity type info into plugins » Convert entity type info into plugins
Issue tags: -Needs change record
tim.plunkett’s picture

Issue summary: View changes

Added more follow-ups

Gábor Hojtsy’s picture

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

neclimdul’s picture

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

Gábor Hojtsy’s picture

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

sdboyer’s picture

/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:

Entity classes are now plugins, and as such are moved into a sub-directory: Drupal\node\Node becomes Drupal\node\Plugin\Core\Entity\Node

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

sun’s picture

Status: Fixed » Active

TBH, 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:

@Entity(
)

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.

tim.plunkett’s picture

Status: Active » Fixed

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

tim.plunkett’s picture

Issue summary: View changes

added another follow-up

amateescu’s picture

I'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

tstoeckler’s picture

Status: Fixed » Active

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

tstoeckler’s picture

Status: Active » Fixed

Crosspost.

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.

Sylvain Lecoy’s picture

While we are at it, it is my personnal opinion, but if we are changing it, then I see it like that:

<?php
/**
 * @Entity
 */
class User {
  
  /**
   * The user ID.
   * @Id @PrimaryKey @Column
   * @var int
   */
  public $uid;

  /**
   * The user name.
   * @Column(type="string")
   * @var string
   */
   public $name;
}
?>
EclipseGc’s picture

Guess 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

catch’s picture

Status: Fixed » Active

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

tim.plunkett’s picture

The ECK guys were fine with this, and hook_entity_info_alter() still exists.

EclipseGc’s picture

I'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

sun’s picture

Status: Active » Needs review
FileSize
4.81 KB

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

yched’s picture

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

fmizzell’s picture

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

Berdir’s picture

About 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?

fago’s picture

About 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?

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

tim.plunkett’s picture

EclipseGc’s picture

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

yched’s picture

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.

Nononono ! 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().

sun’s picture

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

yched’s picture

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

tim.plunkett’s picture

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

yched’s picture

For an annotated class to be a true plugin, it needs to implement PluginInspectionInterface, which the entity types do not

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

tim.plunkett’s picture

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

plach’s picture

I 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:

@Berdir:

I think extending another hook/info/discovery thingy is a bit problematic, we can cheat and extend the original documentation, but contrib modules would not be able to do that. Don't have a better solution.

If I were to do this in contrib right now I'd just use hook_info_alter(), I guess. However since this would make providing defaults harder we should consider investigating a way to augment plugin annotations before altering them.

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 on hook_entity_info_alter() for this is a recipe for disaster.

tim.plunkett’s picture

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

+++ b/core/lib/Drupal/Core/Plugin/Discovery/AlterDecorator.phpundefined
@@ -19,6 +19,7 @@ class AlterDecorator implements DiscoveryInterface {
+  protected $invokeInfo;

@@ -56,6 +64,12 @@ public function getDefinition($plugin_id) {
+    if ($this->invokeInfo) {
+      foreach (module_implements($this->hook) as $module) {
+        $function = $module . '_' . $this->hook;
+        $function($definitions);
+      }

Can we get some docblocks here? Otherwise this is fine by me.

plach’s picture

Works for me too. Should we add back the docs for hook_entity_info()?

tim.plunkett’s picture

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

neclimdul’s picture

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

Fabianx’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityManager.phpundefined
@@ -222,7 +223,7 @@ class EntityManager extends PluginManagerBase {
-    $this->discovery = new AlterDecorator(new AnnotatedClassDiscovery('Core', 'Entity'), 'entity_info');
+    $this->discovery = new AlterDecorator(new InfoHackDecorator(new AnnotatedClassDiscovery('Core', 'Entity'), 'entity_info'), 'entity_info');

+++ b/core/lib/Drupal/Core/Plugin/Discovery/InfoHackDecorator.phpundefined
@@ -2,19 +2,22 @@
-class AlterDecorator implements DiscoveryInterface {
+class InfoHackDecorator implements DiscoveryInterface {

Doesn't this remove the AlterDecorator used above?

Berdir’s picture

diff --git a/core/lib/Drupal/Core/Plugin/Discovery/AlterDecorator.php b/core/lib/Drupal/Core/Plugin/Discovery/InfoHackDecorator.php
similarity index 63%
copy from core/lib/Drupal/Core/Plugin/Discovery/AlterDecorator.php
copy to core/lib/Drupal/Core/Plugin/Discovery/InfoHackDecorator.php
index d4ce40b..4857289 100644

@Fabianx: Thought that too at the first look but it's actually copied and then changed, see those lines.

tstoeckler’s picture

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

neclimdul’s picture

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

tstoeckler’s picture

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

sun’s picture

Status: Needs review » Reviewed & tested by the community

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

neclimdul’s picture

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

plach’s picture

IMO we still need to restore at very least a basic documentation stub for hook_entity_info(), given also that its signature has changed.

Berdir’s picture

In 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

Jose Reyero’s picture

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

tim.plunkett’s picture

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

mitchell’s picture

Jose Reyero’s picture

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

webchick’s picture

Status: Reviewed & tested by the community » Needs work

I 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?

sun’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
8.62 KB

Renamed to InfoHookDecorator. Added Entity API docs. No reason to hold this stop-gap fix up any further.

tim.plunkett’s picture

RTBC +1.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x.

Status: Fixed » Closed (fixed)

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

effulgentsia’s picture

Follow up to enable the rest of PluginManagerInterface, not just discovery: #1867228: Make EntityTypeManager provide an entity factory

effulgentsia’s picture

#218 was committed with the message:

Issue #1763974 follow-up by neclimdul, sun: Put in temporary entity info hook shiv until conversions are complete.

Yet the doc for hook_entity_info_alter() that was added with that same commit says:

Do not use this hook to add information to entity types. Use hook_entity_info() for that instead.

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.

plach’s picture

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()?

The latter please :)

Quoting from #1968970-21: Standardize module-provided entity info documentation and clean-up the @EntityType annotation:

[The Entity Translation module] uses hook_entity_info_alter() because it's just providing defaults for its own keys. Actual values are supposed to be provided by modules integrating with it (through annotations or hook_entity_info()).

tim.plunkett’s picture

Except by that reasoning, ET should use hook_entity_info() to add to defined entity types.

tim.plunkett’s picture

Issue summary: View changes

Added a followup issue.