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.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
FileSize
10.45 KB

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
FileSize
578 bytes
11.01 KB

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.

Eclipse

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.

Eclipse

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

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.

Eclipse

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.

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

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

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

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

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

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.6 was released on August 2, 2017 and is the final full bugfix release for the Drupal 8.3.x series. Drupal 8.3.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.4.0 on October 4, 2017. (Drupal 8.4.0-alpha1 is available for testing.)

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

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.4 was released on January 3, 2018 and is the final full bugfix release for the Drupal 8.4.x series. Drupal 8.4.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.5.0 on March 7, 2018. (Drupal 8.5.0-alpha1 is available for testing.)

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

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.6 was released on August 1, 2018 and is the final bugfix release for the Drupal 8.5.x series. Drupal 8.5.x will not receive any further development aside from security fixes. Sites should prepare to update to 8.6.0 on September 5, 2018. (Drupal 8.6.0-rc1 is available for testing.)

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

Version: 8.6.x-dev » 8.8.x-dev

Drupal 8.6.x will not receive any further development aside from security fixes. Bug reports should be targeted against the 8.8.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.9.x-dev branch. For more information see the Drupal 8 and 9 minor version schedule and the Allowed changes during the Drupal 8 and 9 release cycles.

Version: 8.8.x-dev » 8.9.x-dev

Drupal 8.8.7 was released on June 3, 2020 and is the final full bugfix release for the Drupal 8.8.x series. Drupal 8.8.x will not receive any further development aside from security fixes. Sites should prepare to update to Drupal 8.9.0 or Drupal 9.0.0 for ongoing support.

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

Version: 8.9.x-dev » 9.2.x-dev

Drupal 8 is end-of-life as of November 17, 2021. There will not be further changes made to Drupal 8. Bugfixes are now made to the 9.3.x and higher branches only. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.2.x-dev » 9.3.x-dev

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.15 was released on June 1st, 2022 and is the final full bugfix release for the Drupal 9.3.x series. Drupal 9.3.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.4.x-dev branch from now on, and new development or disruptive changes should be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.9 was released on December 7, 2022 and is the final full bugfix release for the Drupal 9.4.x series. Drupal 9.4.x will not receive any further development aside from security fixes. Drupal 9 bug reports should be targeted for the 9.5.x-dev branch from now on, and new development or disruptive changes should be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 11.x-dev

Drupal core is moving towards using a “main” branch. As an interim step, a new 11.x branch has been opened, as Drupal.org infrastructure cannot currently fully support a branch named main. New developments and disruptive changes should now be targeted for the 11.x branch. For more information, see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.