The interplay between Drupal\Component\Annotation, Drupal\Component\Reflection & Drupal\Component\Plugin\Discovery\AnnotatedClassDiscovery is very scattered and difficult to follow. In terms of organizing this code properly, we should either include the Reflection & Annotation components as part of the Plugins component or we should separate the AnnotatedClassDiscovery from the Plugin Component and put it in the Component that's already supporting it. I've chosen the latter here as a.) it makes sense b.) more clearly defines dependencies for plugin & their discovery and c.) co-locates all the annotation stuff together in one place.

This should have no impact on core beyond the scope of the very very small patch I've already provided and should have 0 impact on contrib as none of these classes are likely to be used on their own. We have Drupal classes that stand in front of all of this stuff currently and have pointed contrib developers to it.

The "Reflection" component is really a subset of the AnnotatedClassDiscovery's needs. No other subsystem uses it and it's just one file. There's really no benefit to it being separated into another component. By contrast the "Annotation" component is heavily used all over core through subclasses of the Plugin annotation. This reorganization would start the ball rolling on getting Utility, Annotation & Plugin components all ready for Composer and this specific patch would bring about cohesion to the Annotation/Reflection & AnnotatedClassDiscovery by really associating these all together in a logical way.

Eclipse

Files: 
CommentFileSizeAuthor
#24 2065481-24.patch2.61 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 58,735 pass(es).
[ View ]
#19 2065481-19.patch2.6 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 58,857 pass(es).
[ View ]
#17 2065481-17.patch2.62 KBtwistor
PASSED: [[SimpleTest]]: [MySQL] 58,915 pass(es).
[ View ]
#14 2065481-6.patch2.78 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 58,123 pass(es).
[ View ]
#6 2065481-5.patch2.78 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 58,204 pass(es).
[ View ]
#4 2065481-4.patch2.47 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2065481-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#3 2065481-2.patch2.32 KBEclipseGc
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2065481-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 2065481-1.patch1.45 KBEclipseGc
PASSED: [[SimpleTest]]: [MySQL] 58,160 pass(es).
[ View ]

Comments

Status:Active» Needs review
StatusFileSize
new1.45 KB
PASSED: [[SimpleTest]]: [MySQL] 58,160 pass(es).
[ View ]

and the patch.

+++ b/core/lib/Drupal/Component/Plugin/Reflection/MockFileFinder.phpundefined
@@ -2,10 +2,10 @@
- * Definition of Drupal\Component\Reflection\MockFileFinder.
+ * Definition of Drupal\Component\Plugin\Reflection\MockFileFinder.

Change this to Contains \Drupal\... when you're touching it anyway?

Title:Move Drupal\Component\Reflection into Drupal\Component\PluginMake the interaction of the Annotation, Reflection & Plugin Discovery make sense
StatusFileSize
new2.32 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2065481-2.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Ok, after discussing this at length with others, I really really wanted Reflection & Annotation components to make sense as actual components, and not just files we didn't know where else to put. It's a good argument that moving the Annotation component around should not be tackled lightly considering the Plugin class in it is extended ALL OVER core (and probably beginning to be in contrib conversions). The Reflection component class is never used anywhere except in AnnotatedClassDiscovery in plugin components. Likewise the AnnotatedClassDiscovery in plugin component is only ever referenced by our Core AnnotatedClassDiscovery.

Logically, looking at Drupal's Plugins, we have a pseudo-dependency on Drupal\Component\Annotation. By virtue of that, this patch attempts to embrace that a bit more fully, and moves all the logically related code into Drupal\Component\Annotation. The Reflection class is moved there, the Component AnnotatedClassDiscovery is moved there. The patch is remarkably small and makes all if the interaction of these things make sense.

Eclipse

StatusFileSize
new2.47 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 2065481-4.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

re #2

Ok, fair enough.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 2065481-4.patch, failed testing.

Status:Needs work» Needs review
StatusFileSize
new2.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,204 pass(es).
[ View ]

Sorry about that, I've not worked with these -M diffs much.

Eclipse

Status:Needs review» Needs work

The last submitted patch, 2065481-5.patch, failed testing.

Raw "Your search used too many AND/OR expressions. Only the first 7 terms were included in this search." found Other SearchPageTextTest.php 73 Drupal\search\Tests\SearchPageTextTest->testSearchText()

No idea if this is random, but doesn't have anything to do with this? Re-testing..

Status:Needs work» Needs review

#6: 2065481-5.patch queued for re-testing.

Issue summary:View changes

fat fingers

It is a

+++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -2,14 +2,14 @@
+ * Contains Drupal\Component\Annotation\Plugin\Discovery\AnnotatedClassDiscovery.
+++ b/core/lib/Drupal/Component/Annotation/Reflection/MockFileFinder.php
@@ -2,10 +2,10 @@
+ * Contains Drupal\Component\Annotation\Reflection\MockFileFinder.
+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -2,12 +2,12 @@
+ * Contains Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery.

You could nitpick the missing starting slash ... Contains "\Drupal..."

Otherwise I really like the decoupling!

+++ b/core/lib/Drupal/Component/Annotation/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -2,14 +2,14 @@
+ * Contains Drupal\Component\Annotation\Plugin\Discovery\AnnotatedClassDiscovery.
+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -2,12 +2,12 @@
+ * Contains Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery.

Why is one moving into Annotation and the other is not?

Because the Core and Component classes don't NEED to mirror each other. Drupal\Core\Plugin\{Stuff} is good enough for anything Drupal specific about the plugin use case. The annotation component is purely support of that within core, and I don't see a reason to move that around really. I'm not opposed to it, but I don't see the benefit.

Eclipse

Issue tags:+API change

In that case, I don't really see the benefit of this entire issue. If the goal is to gather everything under one banner, oh except this thing, then this seems like a needless API break.

StatusFileSize
new2.78 KB
PASSED: [[SimpleTest]]: [MySQL] 58,123 pass(es).
[ View ]

The goal is to gather the COMPONENTS under their appropriate banners. The core implementation is dependent upon them, so any organization that makes sense there is ok. As I said, I'm not opposed to a move of some classes within core, but I don't see the need. Drupal\Core\Plugin is dependent upon anything in Drupal\Component and anything in vendor. If we want to logically re-organize Drupal\Core stuff, I'm ok with it, just trying to prevent unnecessary scope creep of the patch.

Fixed #10, no interdiff provided since this is so short.

Eclipse

Status:Needs review» Reviewed & tested by the community

Fixed #10, no interdiff provided since this is so short.

No interdiff, because you are lazy, let's be honest :)

In that case, I don't really see the benefit of this entire issue. If the goal is to gather everything under one banner, oh except this thing, then this seems like a needless API break.

As talked with EclipseGC in IRC, these are all classes which are wrapped by drupal core specific extensions, which will people use anyway.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Reviewed & tested by the community
Issue tags:-Needs reroll
StatusFileSize
new2.62 KB
PASSED: [[SimpleTest]]: [MySQL] 58,915 pass(es).
[ View ]

3-way

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Status:Needs work» Needs review
Issue tags:-Needs reroll
StatusFileSize
new2.6 KB
PASSED: [[SimpleTest]]: [MySQL] 58,857 pass(es).
[ View ]

Rerolled, should pass tests still.

Eclipse

Status:Needs review» Needs work
Issue tags:-API change

The last submitted patch, 2065481-19.patch, failed testing.

Status:Needs work» Needs review
Issue tags:+API change

Not sure why that failed, retesting.

#19: 2065481-19.patch queued for re-testing.

Eclipse

Status:Needs review» Reviewed & tested by the community

Patch is green, so back.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

StatusFileSize
new2.61 KB
PASSED: [[SimpleTest]]: [MySQL] 58,735 pass(es).
[ View ]

rerolled

Status:Needs work» Needs review
Issue tags:-Needs reroll

Status:Needs review» Reviewed & tested by the community

Back to RTBC

Status:Reviewed & tested by the community» Fixed
Issue tags:+Approved API change

Committed ddaeead and pushed to 8.x. Thanks!

No, THANK YOU! Having this in makes my life better already :-D

Eclipse

Nice commit hash, bro.

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

Issue summary:View changes

Updated to include the revised scope.