Comments

Tagging.

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.

Assigned:Unassigned» claudiu.cristea
Status:Needs work» Needs review
Issue tags:-Needs profiling
StatusFileSize
new7.55 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

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.

StatusFileSize
new9.02 KB
PASSED: [[SimpleTest]]: [MySQL] 59,249 pass(es).
[ View ]

Ah! Forgot new test plugins :)

StatusFileSize
new2.73 KB
new10.06 KB
PASSED: [[SimpleTest]]: [MySQL] 59,937 pass(es).
[ View ]

Small doc fix.

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

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.

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

Issue summary:View changes
StatusFileSize
new1.55 KB
FAILED: [[SimpleTest]]: [MySQL] 58,945 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

OK. Here's without max depth.

Status:Needs review» Needs work

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

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.

Status:Needs work» Needs review
StatusFileSize
new3.83 KB
PASSED: [[SimpleTest]]: [MySQL] 58,989 pass(es).
[ View ]
new1.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.

Status:Needs review» Reviewed & tested by the community

Looks good, works like it says.

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.

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.