Pass plugin definition to plugin instances instead of discovery object

I'm in favor of this.

+++ b/core/lib/Drupal/Component/Plugin/PluginBase.phpundefined
@@ -14,18 +14,18 @@
+  protected $pluginId;
...
-  protected $plugin_id;

@@ -41,28 +41,27 @@
-    $this->plugin_id = $plugin_id;
...
+    $this->pluginId = $plugin_id;

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/ResourceBase.phpundefined
@@ -34,7 +34,7 @@ public function permissions() {
-        $permissions["restful $lowered_method $this->plugin_id"] = array(
+        $permissions["restful $lowered_method $this->pluginId"] = array(

@@ -53,8 +53,8 @@ public function permissions() {
-    $path_prefix = strtr($this->plugin_id, ':', '/');
-    $route_name = strtr($this->plugin_id, ':', '.');
+    $path_prefix = strtr($this->pluginId, ':', '/');
+    $route_name = strtr($this->pluginId, ':', '.');

@@ -64,11 +64,11 @@ public function routes() {
-          '_plugin' => $this->plugin_id,
+          '_plugin' => $this->pluginId,
...
-          '_permission' => "restful $lower_method $this->plugin_id",
+          '_permission' => "restful $lower_method $this->pluginId",

+++ b/core/modules/rest/lib/Drupal/rest/Plugin/rest/resource/EntityResource.phpundefined
@@ -76,7 +76,7 @@ public function post($id, EntityInterface $entity) {
-      $url = url(strtr($this->plugin_id, ':', '/') . '/' . $entity->id(), array('absolute' => TRUE));
+      $url = url(strtr($this->pluginId, ':', '/') . '/' . $entity->id(), array('absolute' => TRUE));

Not in scope

However, we can likely remove all of the use Drupal\Component\Plugin\Discovery\DiscoveryInterface;

This totally makes sense. Another reason why I am in favor of this is if I do a debug($plugin) I totally don't want to see the innards of some discovery object (let alone the whole plugin manager) on my screen. This benefit is surely outweighed by the other factors mentioned above, but I thought I'd bring it up.

Regarding the patch: I'm not sure, but I think our coding standards would require $plugin->pluginID though, rather than $plugin->pluginId.

#11: pluginBase_copy_definition-1875182-11.patch queued for re-testing.

Failures aside, this doesn't look insane to me, and the sooner we settle this particular issue the better as new plugin types are being created. I'd really like if neclimdul could weigh in on this as I think I proposed essentially this when we started and he wanted the discovery injected and I don't recall why. But in general I'm ++ to this approach.

Eclipse

Still Looks good.

Quick pass, got some deeper thoughts I'm working on:

+++ b/core/lib/Drupal/Component/Plugin/Factory/DefaultFactory.phpundefined
@@ -41,23 +41,23 @@ public function __construct(DiscoveryInterface $discovery) {
-   *  @param array $plugin_id
+   *  @param string $plugin_id
    *    The id of a plugin.
-   * @param \Drupal\Component\Plugin\Discovery\DiscoveryInterface $discovery
-   *   The discovery object.
+   *  @param array $plugin_definition

Looks like the indenting is off on the @param lines?

+++ b/core/modules/tour/lib/Drupal/tour/TipPluginBase.phpundefined
@@ -41,16 +40,6 @@
-    $this->module = $this->definition['module'];

This change seems unrelated and possibly a regression.

+++ b/core/modules/views/lib/Drupal/views/Tests/PluginBaseUnitTest.phpundefined
@@ -250,8 +250,9 @@ public function testSetOptionDefault() {
     $discovery = new StaticDiscovery();
-    $discovery->setDefinition('default', array());
-    return new TestHelperPlugin(array(), 'default', $discovery);
+    $definition = array();
+    $discovery->setDefinition('default', $definition);
+    return new TestHelperPlugin(array(), 'default', $definition);

hugh? if we're not passing discovery why are we still bothering with it here?

Megh, I don't think its as big a deal. The lack of changes this caused to testing makes me very unhappy with the plugin tests but that's unrelated. I'm not sure we need to remove this from the reflection factory though.

+++ b/core/lib/Drupal/Component/Plugin/Factory/ReflectionFactory.phpundefined
@@ -46,28 +47,29 @@ public function createInstance($plugin_id, array $configuration) {
-      elseif ($param_class && $param_class->isInstance($this->discovery)) {
-        $arguments[] = $this->discovery;
-      }

#23: pluginBase_copy_definition-1875182-23.patch queued for re-testing.

After discussing this further, neclimdul and I are both ++ on the RTBC for this if 23 comes back green again.

Eclipse

Also, effulgentsia indicated support of the approach in general in #3, so I think we have all 3 of the plugin devs on board here.

Eclipse

This makes a lot more sense to me, and seems to simplify things quite a bit too. Yay!

Committed and pushed to 8.x. Thanks!