As discussed in #2051847: AnnotatedClassDiscovery::getDefinitions is critically slow when viewing nodes with comments, the fact that discovery has a getDefinition() method is misleading. When using the default plugin manager, it by-passes caching, processing and and possibly other logic defined in the plugin manager.

As discussed over there, we should remove that method. @catch agreed on doing so, and it's a fairly small API change. that only affects plugin managers that call it (and when they do, it's probably a bug anyway, at least those that use the now recommended default plugin manager) and implementations, but those wouldn't break if they would still implement the method.

Files: 
CommentFileSizeAuthor
#5 drupal-2052921-3.patch11.01 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] 54,867 pass(es), 468 fail(s), and 699 exception(s).
[ View ]
#5 interdiff.txt578 bytesdawehner
#1 drupal-2052921-1.patch10.45 KBdawehner
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new10.45 KB
FAILED: [[SimpleTest]]: [MySQL] Drupal installation failed.
[ View ]

Let's see whether this is already green.

Status:Needs review» Needs work

The last submitted patch, drupal-2052921-1.patch, failed testing.

I very much disagree with this. Because Drupal has api's that force it to cache everything and use this de'facto standard does not change the validity of the API. I'll leave this open because I am open to having my mind changed but I'd rather close this and just fix the miss-use of the API causing the performance problem in the other issue.

It's not just about caching. The discovery returns the raw, discovered values, they're also missing defaults and anything else that's applied in processDefinition().

Status:Needs work» Needs review
StatusFileSize
new578 bytes
new11.01 KB
FAILED: [[SimpleTest]]: [MySQL] 54,867 pass(es), 468 fail(s), and 699 exception(s).
[ View ]

Does someone know whether there is a plan for the entity manager to switch to the default plugin manager as long as it has not its own custom annotation reader system?

Status:Needs review» Needs work

The last submitted patch, drupal-2052921-3.patch, failed testing.

getDefinitions is no different in that regard so we have done nothing here to address the problem and have removed a useful API method we just happen to not be using in our implementations.

So, since this seems to be an issue about defending the DiscoveryInterface, let's focus on that for the time being.

Most of core's discovery classes do make use of the getDefinition() method by populating ALL definitions before hand making it seem useless at first. This is, however, not the case in the broader sense of the interface itself. The StaticDiscovery class can get individual definitions, and if you were to build (for example) a DBDiscovery, getting a single definition stored in the database would be quite simple too. This same thing is true of a CMI based solution (which there have been many people who've asked for, and though we've fought it in core, I expect to see it in contrib). In short, the two methods of DiscoveryInterface are QUITE defensible in my opinion, and any plugin manager code that is NOT giving you the processed/cached/altered/whatever'd definition, is a bug in the manager, not an indicator of some greater issue with the interfaces themselves.

I hope that's a clear answer. I'm very much on the "closed (won't fix)" train here myself.

Eclipse

Status:Needs work» Closed (works as designed)

I'm closing this. If there is a general sense that this needs to be talked out more, that's fine, but I think I made my point. Hopefully we're all on the same page now.

Eclipse

Status:Closed (works as designed)» Active

Even if we leave the method in, this needs comments to explain when it's OK and not OK to use it.

Title:Remove getDefinition() from Plugin discovery interfaceDocument the complexities of the DefaultPluginManager
Issue tags:-API change

I'm OK with documenting that there's complexity but I think that documentation belongs on the DefaultPluginManager where the complexity resides. Let me elaborate a little.

The problem we've built for ourselves is that in our efforts to make a one-size fits all implementations for plugins we've made a very complex interaction of components. The responsibilities of the interface is still as documented, to "Gets a specific plugin definition." https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Plug...

If we look at the Component plugin manager you'll see the counterpoint simplicity https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Component%21Plug...

The problem is that ->discovery is the base discovery on the core default manager and we've baked complexity into the manager itself meaning when changing the behavior of the base manager you inherit the need to understand that complexity. That is the nature of direct inheritance.

So, I guess what I'm saying is that we'll never completely document the complexity of any use of the interface in the interface itself and we should focus on people understanding what they're getting when they inherit from the default manager.

Yup, just to be clear, getDefinition() should always be safe to use. That is not what bit us in the other issue.

Eclipse

I feel the problem comes from the fact that we use the same interface for two different objects with two different purposes: the "discovery mechanism" (whose typical action is "discover the definitions") and the "definitions registry" (whose typical actions are "give me the final, official definitions in a performant way" or "only one of them")
Only the manager can be used as a registry, the discovery should not.

This being said, not sure what to do with this :-)

Again, we're talking about the DefaultPluginManager implementation not the interface. If Pressflow wanted to replace our annotation discovery with some prebuilt sqlite/APC cache optimized discovery object and gut the default manager that would be completely valid and your statements would be reversed.