Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

Tagging.

EclipseGc’s picture

Should have written it this way myself the first time through. ++ and RTBC if it passes.

Eclipse

Status: Needs review » Needs work

The last submitted patch, acd.patch, failed testing.

claudiu.cristea’s picture

Assigned: Unassigned » claudiu.cristea
Status: Needs work » Needs review
Issue tags: -needs profiling
FileSize
7.55 KB

Here's a patch that introduces a max depth argument. We don't want to leave the discovery dig to much. Passing -1 as $max_depth still permits unlimited level descending (that is not recommended). The default is 0 meaning the actual behavior.

Introducing this argument we don't have to be afraid about performance, so I'm removing the tag.

I'm taking this issue just to keep track. Forgot the interdiff bit it's easy to read diffs.

claudiu.cristea’s picture

Ah! Forgot new test plugins :)

claudiu.cristea’s picture

claudiu.cristea’s picture

Small doc fix.

dawehner’s picture

claudiu.cristea’s picture

+++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -43,6 +43,13 @@ class AnnotatedClassDiscovery implements DiscoveryInterface {
   /**
+   * The maximum level of depth for recursion under the top discovery directory.
+   *
+   * @var int
+   */
+  protected $maxDepth;

The only thing I'm not very sure with this patch is if this protected property needs to be added also to DefaultPluginManager, as we pass the $max_depth in the constructor. I'm tempted to say "No", because we need the max depth only to be passed to discovery component.

chx’s picture

I do not think adding complexity in the form of maximum depth is necessary. Nor do I think any performance concerns can be possibly valid when the annotated discovery is insanely slow already. It's cached and if you remove the caching decorator ( I tried for debug ) you'll think HEAD is broken it takes so long to do anything that you'll think it's not happening.

claudiu.cristea’s picture

@chx,

Max depth is a functional add not necessary a performance improvement. It was only a side reply to #1. I think we need to limit descending level.

claudiu.cristea’s picture

OK. Here's without max depth.

Status: Needs review » Needs work

The last submitted patch, 12: plugin-discovery-recursion-2103155-12.patch, failed testing.

chx’s picture

So, to clairfy more, plugin directories will, in my opinion never be deep. In fact, most of the time there will be no subdirs even.

claudiu.cristea’s picture

Status: Needs work » Needs review
FileSize
3.83 KB
1.83 KB

The test had to be adjusted for descending in subdirectories. But this is good as we need to test this new feature too.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks good, works like it says.

chx’s picture

Priority: Normal » Major

This is a blocker for migrate so elevating to major. I am itching to elevate it to critical though... it's just unnatural not to pick up recursively.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

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