Spin-off from #1836008-65: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery.

CommentFileSizeAuthor
#28 plugin-annotatedclassdiscovery-followup-28.patch230.33 KBneclimdul
#26 plugin-annotatedclassdiscovery-followup-26.patch199.93 KBeffulgentsia
#24 plugin-annotatedclassdiscovery-followup-24.patch171.55 KBjibran
#20 plugin-annotatedclassdiscovery-followup-20.patch171.5 KBeffulgentsia
#18 plugin-annotatedclassdiscovery-18.patch14.26 KBeffulgentsia
#11 plugin-annotatedclassdiscovery-11.patch14.59 KBberdir
#11 plugin-annotatedclassdiscovery-11-interdiff.txt1.83 KBberdir
#8 plugin-annotatedclassdiscovery-8.patch14.6 KBeffulgentsia
#8 diff.txt727 byteseffulgentsia
#7 plugin-annotatedclassdiscovery-7.patch14.47 KBeffulgentsia
#6 plugin-annotatedclassdiscovery-6.patch179.95 KBeffulgentsia
#5 plugin-annotatedclassdiscovery-5.patch179.96 KBeffulgentsia
#3 plugin-annotatedclassdiscovery.patch179.1 KBeffulgentsia
#1 annotations_10.patch8.39 KBeffulgentsia

Comments

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new annotations_10.patch8.39 KB

EclipseGc's patch from #1836008-40: Remove drupal_classloader() use in Drupal\Core\AnnotatedClassDiscovery.

Status: Needs review » Needs work

The last submitted patch, annotations_10.patch, failed testing.

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Needs review
StatusFileSize
new plugin-annotatedclassdiscovery.patch179.1 KB

How about this? Don't let the size scare you. It's 99% global search/replace. Only the two AnnotatedClassDiscovery classes contain substantive changes.

Status: Needs review » Needs work

The last submitted patch, plugin-annotatedclassdiscovery.patch, failed testing.

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Needs review
StatusFileSize
new plugin-annotatedclassdiscovery-5.patch179.96 KB

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.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new plugin-annotatedclassdiscovery-6.patch179.95 KB

Chasing HEAD.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new plugin-annotatedclassdiscovery-7.patch14.47 KB

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.

effulgentsia’s picture

effulgentsia commented
StatusFileSize
new diff.txt727 bytes
new plugin-annotatedclassdiscovery-8.patch14.6 KB 
+++ b/core/lib/Drupal/Core/Plugin/Discovery/AnnotatedClassDiscovery.php
@@ -26,75 +20,29 @@ class AnnotatedClassDiscovery implements DiscoveryInterface {
+    foreach (drupal_classloader()->getNamespaces() as $namespace => $dirs) {
+      $plugin_namespaces["$namespace\\Plugin\\{$this->owner}\\{$this->type}"] = $dirs;
     }

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.

eclipsegc’s picture

eclipsegc commented

if that passes I'm 100% ++

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented

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

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new plugin-annotatedclassdiscovery-11-interdiff.txt1.83 KB
new plugin-annotatedclassdiscovery-11.patch14.59 KB

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

eclipsegc’s picture

eclipsegc commented

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.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ 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?

eclipsegc’s picture

eclipsegc commented

That's a fair point. Who wrote that todo? is it still valid? if so, why?

Eclipse

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Looks like the @todo needs clarification (I'm also confused by it).

effulgentsia’s picture

effulgentsia commented

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?

catch’s picture

catch
he/him
Primary language English
commented

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.

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new plugin-annotatedclassdiscovery-18.patch14.26 KB

Removed the two @todos related to that.

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Looks sensible. Committed/pushed to 8.x.

effulgentsia’s picture

effulgentsia commented
Status: Fixed » Needs review
StatusFileSize
new plugin-annotatedclassdiscovery-followup-20.patch171.5 KB

And here's the follow up per #16.

tstoeckler’s picture

tstoeckler
he/him
Primary language German
Location Essen, Germany
commented
Status: Needs review » Reviewed & tested by the community

Looks good.

catch’s picture

catch
he/him
Primary language English
commented

#20: plugin-annotatedclassdiscovery-followup-20.patch queued for re-testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, plugin-annotatedclassdiscovery-followup-20.patch, failed testing.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Toronto, Canada
commented
Status: Needs work » Needs review
StatusFileSize
new plugin-annotatedclassdiscovery-followup-24.patch171.55 KB

Reroll

Status: Needs review » Needs work

The last submitted patch, plugin-annotatedclassdiscovery-followup-24.patch, failed testing.

effulgentsia’s picture

effulgentsia commented
Status: Needs work » Reviewed & tested by the community
StatusFileSize
new plugin-annotatedclassdiscovery-followup-26.patch199.93 KB

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

Status: Reviewed & tested by the community » Needs work

The last submitted patch, plugin-annotatedclassdiscovery-followup-26.patch, failed testing.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Needs work » Needs review
StatusFileSize
new plugin-annotatedclassdiscovery-followup-28.patch230.33 KB

reroll

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Reviewed & tested by the community

Looks good. I scrolled through the complete patch, I think that gives me the right to RTBC this :)

robloach’s picture

robloach
he/him
commented

+1

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.x, thanks!

podarok’s picture

podarok
🇺🇦 he/him/his
Primary language Ukrainian
Location Ukraine
commented
Priority: Normal » Critical
Status: Fixed » Needs work
Issue tags: +Needs change record

this one needs change records due to
before

use Drupal\Core\Annotation\Plugin;

after

use Drupal\Component\Annotation\Plugin;

namespace resolving change

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Priority: Critical » Normal
Status: Needs work » Fixed
Issue tags: -Needs change record

Updated http://drupal.org/node/1704454. As this is a HEAD2HEAD only change, this doesn't need its own change notice.

Status: Fixed » Closed (fixed)

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