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
Comment | File | Size | Author |
---|---|---|---|
#24 | 2065481-24.patch | 2.61 KB | EclipseGc |
#19 | 2065481-19.patch | 2.6 KB | EclipseGc |
#17 | 2065481-17.patch | 2.62 KB | twistor |
#14 | 2065481-6.patch | 2.78 KB | EclipseGc |
#6 | 2065481-5.patch | 2.78 KB | EclipseGc |
Comments
Comment #1
EclipseGc CreditAttribution: EclipseGc commentedand the patch.
Comment #2
BerdirChange this to Contains \Drupal\... when you're touching it anyway?
Comment #3
EclipseGc CreditAttribution: EclipseGc commentedOk, 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
Comment #4
EclipseGc CreditAttribution: EclipseGc commentedre #2
Ok, fair enough.
Eclipse
Comment #6
EclipseGc CreditAttribution: EclipseGc commentedSorry about that, I've not worked with these -M diffs much.
Eclipse
Comment #8
BerdirRaw "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..
Comment #9
Berdir#6: 2065481-5.patch queued for re-testing.
Comment #9.0
Berdirfat fingers
Comment #10
dawehnerIt is a
You could nitpick the missing starting slash ... Contains "\Drupal..."
Otherwise I really like the decoupling!
Comment #11
tim.plunkettWhy is one moving into Annotation and the other is not?
Comment #12
EclipseGc CreditAttribution: EclipseGc commentedBecause 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
Comment #13
tim.plunkettIn 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.
Comment #14
EclipseGc CreditAttribution: EclipseGc commentedThe 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
Comment #15
dawehnerNo interdiff, because you are lazy, let's be honest :)
As talked with EclipseGC in IRC, these are all classes which are wrapped by drupal core specific extensions, which will people use anyway.
Comment #16
alexpottPatch no longer applies.
Comment #17
twistor CreditAttribution: twistor commented3-way
Comment #18
webchickPatch no longer applies.
Comment #19
EclipseGc CreditAttribution: EclipseGc commentedRerolled, should pass tests still.
Eclipse
Comment #21
EclipseGc CreditAttribution: EclipseGc commentedNot sure why that failed, retesting.
#19: 2065481-19.patch queued for re-testing.
Eclipse
Comment #22
dawehnerPatch is green, so back.
Comment #23
alexpottPatch no longer applies.
Comment #24
EclipseGc CreditAttribution: EclipseGc commentedrerolled
Comment #25
tim.plunkettComment #26
dawehnerBack to RTBC
Comment #27
alexpottCommitted ddaeead and pushed to 8.x. Thanks!
Comment #28
EclipseGc CreditAttribution: EclipseGc commentedNo, THANK YOU! Having this in makes my life better already :-D
Eclipse
Comment #29
yched CreditAttribution: yched commentedNice commit hash, bro.
Comment #30.0
(not verified) CreditAttribution: commentedUpdated to include the revised scope.