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.

#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


dawehner’s picture

Status: Active » Needs review
10.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.

neclimdul’s picture

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.

Berdir’s picture

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

dawehner’s picture

Status: Needs work » Needs review
578 bytes
11.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.

neclimdul’s picture

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.

EclipseGc’s picture

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.


EclipseGc’s picture

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.


catch’s picture

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.

neclimdul’s picture

Title: Remove getDefinition() from Plugin discovery interface » Document 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."

If we look at the Component plugin manager you'll see the counterpoint simplicity

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.

EclipseGc’s picture

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


yched’s picture

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

neclimdul’s picture

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.

joelpittet’s picture

Issue summary: View changes
Issue tags: +documentation

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.