Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
How about this? Don't let the size scare you. It's 99% global search/replace. Only the two AnnotatedClassDiscovery classes contain substantive changes.
Added Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery::getPluginNamespaces(), because setting it only in the constructor was causing the test failures for reasons stated in the PHPDoc for it.
In case #6 is too big to get in quickly, here's a version that retains Drupal\Core\Annotation\Plugin with a @todo to remove it from the 300 classes that use it later.
I discussed #7 with EclipseGc, and he suggested making the drupal_classloader()->getNamespaces() above overridable with an optional constructor param, so here's that. Interdiff is failing me on this one, so here's a regular "diff" of the 2 patch files instead.
No I don't think we do, the whole point of having core/component here is so that we have a more injectable approach that still has core assumptions for when we're not injecting. This shouldn't require any sort of large scale updating of existing managers, and actually should give a few new abilities to people who've been asking for them.
+++ b/core/lib/Drupal/Core/Annotation/Plugin.phpundefined
@@ -7,68 +7,15 @@
+ *
+ * @todo Remove after globally replacing all plugin classes to "use" the
+ * Component one instead of this one: http://drupal.org/node/1849752.
*/
+class Plugin extends ComponentPlugin {
@EclipseGc: What you are saying probably makes perfect sense, I don't know that much about the plugin/discovery system yet.
But it does seem to conflict with this @todo? Do we maybe need some more documentation here then to tell people when they should use which annotation class?
There's no reason for D8 to ship with a Drupal\Core\Annotation\Plugin class that extends Drupal\Component\Annotation\Plugin, but does nothing extra or different. The only reason for it in #11 is to avoid people having to review and reroll a 200kb patch that's 90% global search/replace (see #6). My thought was we could commit #11, and then follow up with the global search/replace and removal of the stub Drupal\Core\Annotation\Plugin class right here in this issue within a day or so of when the initial patch is committed.
Note: this is not about AnnotatedClassDiscovery: it's about the class that defines the @Plugin annotation itself.
Given the above, any thoughts on how to reword the @todo to make that clear?
Oh I see. If that's the case, l think we could just add a follow up issue to remove it. No point fussing over a @todo that's supposed to only live for 2 days.
Comments
Comment #1
effulgentsia CreditAttribution: effulgentsia commentedEclipseGc's patch from #1836008-40: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery.
Comment #3
effulgentsia CreditAttribution: effulgentsia commentedHow about this? Don't let the size scare you. It's 99% global search/replace. Only the two AnnotatedClassDiscovery classes contain substantive changes.
Comment #5
effulgentsia CreditAttribution: effulgentsia commentedAdded Drupal\Core\Plugin\Discovery\AnnotatedClassDiscovery::getPluginNamespaces(), because setting it only in the constructor was causing the test failures for reasons stated in the PHPDoc for it.
Comment #6
effulgentsia CreditAttribution: effulgentsia commentedChasing HEAD.
Comment #7
effulgentsia CreditAttribution: effulgentsia commentedIn case #6 is too big to get in quickly, here's a version that retains Drupal\Core\Annotation\Plugin with a @todo to remove it from the 300 classes that use it later.
Comment #8
effulgentsia CreditAttribution: effulgentsia commentedI discussed #7 with EclipseGc, and he suggested making the
drupal_classloader()->getNamespaces()
above overridable with an optional constructor param, so here's that. Interdiff is failing me on this one, so here's a regular "diff" of the 2 patch files instead.Comment #9
EclipseGc CreditAttribution: EclipseGc commentedif that passes I'm 100% ++
Comment #10
tim.plunkettThis doesn't conflict at all with what I tried to do in #1847002: Move entity type classes from Drupal\$provider\Plugin\Core\Entity to Drupal\$provider\Entity (move the hardcoded Plugin\$owner\$type namespace into a method) since it still has getPluginNamespaces() to override, so I'm okay with this.
Comment #11
BerdirAttached re-roll just contains a bunch of Definition of vs. Contains fixes and one namespace reference fix.
This has the sign-off from EclipseGC and tim.plunkett and it looks great to me as well. RTBC.
Do we want to open a follow-up issue to convert the use definitions from Core to Component?
Comment #12
EclipseGc CreditAttribution: EclipseGc commentedNo I don't think we do, the whole point of having core/component here is so that we have a more injectable approach that still has core assumptions for when we're not injecting. This shouldn't require any sort of large scale updating of existing managers, and actually should give a few new abilities to people who've been asking for them.
Comment #13
Berdir@EclipseGc: What you are saying probably makes perfect sense, I don't know that much about the plugin/discovery system yet.
But it does seem to conflict with this @todo? Do we maybe need some more documentation here then to tell people when they should use which annotation class?
Comment #14
EclipseGc CreditAttribution: EclipseGc commentedThat's a fair point. Who wrote that todo? is it still valid? if so, why?
Eclipse
Comment #15
catchLooks like the @todo needs clarification (I'm also confused by it).
Comment #16
effulgentsia CreditAttribution: effulgentsia commentedThere's no reason for D8 to ship with a Drupal\Core\Annotation\Plugin class that extends Drupal\Component\Annotation\Plugin, but does nothing extra or different. The only reason for it in #11 is to avoid people having to review and reroll a 200kb patch that's 90% global search/replace (see #6). My thought was we could commit #11, and then follow up with the global search/replace and removal of the stub Drupal\Core\Annotation\Plugin class right here in this issue within a day or so of when the initial patch is committed.
Note: this is not about AnnotatedClassDiscovery: it's about the class that defines the @Plugin annotation itself.
Given the above, any thoughts on how to reword the @todo to make that clear?
Comment #17
catchOh I see. If that's the case, l think we could just add a follow up issue to remove it. No point fussing over a @todo that's supposed to only live for 2 days.
Comment #18
effulgentsia CreditAttribution: effulgentsia commentedRemoved the two @todos related to that.
Comment #19
catchLooks sensible. Committed/pushed to 8.x.
Comment #20
effulgentsia CreditAttribution: effulgentsia commentedAnd here's the follow up per #16.
Comment #21
tstoecklerLooks good.
Comment #22
catch#20: plugin-annotatedclassdiscovery-followup-20.patch queued for re-testing.
Comment #24
jibranReroll
Comment #26
effulgentsia CreditAttribution: effulgentsia commentedNew global search/replace. This was already RTBC'd in #21; this is just updated for the new plugins added since then, so straight to RTBC.
Comment #28
neclimdulreroll
Comment #29
BerdirLooks good. I scrolled through the complete patch, I think that gives me the right to RTBC this :)
Comment #30
RobLoach+1
Comment #31
catchCommitted/pushed to 8.x, thanks!
Comment #32
podarokthis one needs change records due to
before
after
namespace resolving change
Comment #33
tim.plunkettUpdated http://drupal.org/node/1704454. As this is a HEAD2HEAD only change, this doesn't need its own change notice.