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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

EclipseGc’s picture

Status: Active » Needs review
FileSize
1.45 KB

and the patch.

Berdir’s picture

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

EclipseGc’s picture

Title: Move Drupal\Component\Reflection into Drupal\Component\Plugin » Make the interaction of the Annotation, Reflection & Plugin Discovery make sense
FileSize
2.32 KB

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

EclipseGc’s picture

FileSize
2.47 KB

re #2

Ok, fair enough.

Eclipse

Status: Needs review » Needs work

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

EclipseGc’s picture

Status: Needs work » Needs review
FileSize
2.78 KB

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.

Berdir’s picture

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

Berdir’s picture

Status: Needs work » Needs review

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

Berdir’s picture

Issue summary: View changes

fat fingers

dawehner’s picture

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!

tim.plunkett’s picture

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

EclipseGc’s picture

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

tim.plunkett’s picture

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.

EclipseGc’s picture

FileSize
2.78 KB

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

dawehner’s picture

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.

alexpott’s picture

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

Patch no longer applies.

twistor’s picture

Status: Needs work » Reviewed & tested by the community
Issue tags: -Needs reroll
FileSize
2.62 KB

3-way

webchick’s picture

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

Patch no longer applies.

EclipseGc’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.6 KB

Rerolled, should pass tests still.

Eclipse

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

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

EclipseGc’s picture

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

Not sure why that failed, retesting.

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

Eclipse

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Patch is green, so back.

alexpott’s picture

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

Patch no longer applies.

EclipseGc’s picture

FileSize
2.61 KB

rerolled

tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC

alexpott’s picture

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

Committed ddaeead and pushed to 8.x. Thanks!

EclipseGc’s picture

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

Eclipse

yched’s picture

Nice commit hash, bro.

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

Anonymous’s picture

Issue summary: View changes

Updated to include the revised scope.