Issue #2504031: Fix __construct of plugin to be compatible with D8 Plugins #51

drupol · 2015-06-11T14:04:52Z

Update code to be compatible with D8.

LionsAd · 2015-06-11T14:18:36Z

src/DependencyInjection/Container.php

+      'arguments' => array()
+    );
+
+    array_unshift($service_definition['arguments'], $configuration);


Lets move all of this into the caller (ContainerAwarePluginManager::createInstance)

dawehner · 2015-06-11T14:19:21Z

Mh I think its odd to expose the plugin information on a container

LionsAd · 2015-06-11T14:20:42Z

src/Plugin/ContainerAwarePluginManager.php

@@ -61,8 +61,7 @@ public function hasDefinition($plugin_id) {
  public function createInstance($plugin_id, array $configuration = array()) {
    // @todo: Use ->expandArguments() when get() disallows getting private
    //        services.
-    $plugin = clone $this->container->get($this->servicePrefix . $plugin_id);
-    return $plugin;
+    return $this->container->createInstance($this->servicePrefix . $plugin_id, $configuration);


Call this with the $definition instead, get the definition via: $this->getDefinition($plugin_id);

drupol · 2015-06-11T16:03:31Z

Can't get this working.

LionsAd · 2015-06-11T16:52:23Z

src/DependencyInjection/Container.php

@@ -218,6 +218,8 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER) {
    $this->services[$id] = $service;
  }

+
+


white space change

LionsAd · 2015-06-11T16:56:06Z

@dawehner: It is, the definitions should be parameters per plugin manager. (and yes I want that info in the container for now for those plugin managers).

This is a first step into that direction, createInstance is only temporary. I think the real fix is to have get() also take a definition array.

LionsAd · 2015-06-11T16:56:34Z

@drupol The code is great. The unit tests fail, because of changed expectations. I will fix them - likely tomorrow.

drupol · 2015-06-11T17:06:17Z

Ok, I will look for it in the meantime ! Thanks for your patience :)

drupol · 2015-06-11T17:10:35Z

If it helps, when I try to run Openlayers with these changes, I get the error:

RuntimeException: The "plugin_openlayers.Map.internal.OLMap" service definition does not exist. in Drupal\service_container\DependencyInjection\Container->get() (line 88 of /home/pol/git/service_container_fork/src/DependencyInjection/Container.php).

LionsAd · 2015-06-11T17:13:01Z

src/Plugin/ContainerAwarePluginManager.php

+
+    array_unshift($plugin_definition['arguments'], $configuration);
+
+    return $this->container->createInstance($this->servicePrefix . $plugin_id, $configuration);


I see the bug now, need to pass $plugin_definition and not $configuration here ...

drupol · 2015-06-11T17:16:27Z

Now I don't have any errors and it seems that it has fixed a couple of things. BUT... I have now plenty of:

Warning: Missing argument 3 for Drupal\openlayers\Types\Object::__construct(), called in /home/pol/git/service_container_fork/src/DependencyInjection/Container.php on line 154 and defined in Drupal\openlayers\Types\Object->__construct() (line 88 of /home/pol/git/openlayers/src/Types/Object.php).
Notice: Undefined index: plugin module in Drupal\openlayers\Types\Object->__construct() (line 90 of /home/pol/git/openlayers/src/Types/Object.php).
Notice: Undefined index: plugin type in Drupal\openlayers\Types\Object->__construct() (line 90 of /home/pol/git/openlayers/src/Types/Object.php).
Notice: Undefined index: name in Drupal\openlayers\Types\Object->__construct() (line 90 of /home/pol/git/openlayers/src/Types/Object.php).
Warning: Missing argument 3 for Drupal\openlayers\Types\Object::__construct(), called in /home/pol/git/service_container_fork/src/DependencyInjection/Container.php on line 154 and defined in Drupal\openlayers\Types\Object->__construct() (line 88 of /home/pol/git/openlayers/src/Types/Object.php).

I will look for them at home.

LionsAd · 2015-06-11T17:42:44Z

@drupol Yes, that is the BC break I talked about.

We should break BC some more before you fix it ;).

drupol · 2015-06-11T17:43:17Z

No problem :)

LionsAd · 2015-06-11T17:46:12Z

To break BC some more we should use:

$configuration
$plugin_id
$plugin_definition (already in arguments as first part)
any further parameters

As the first parameters.

That means array_unshift the plugin_id first and then the configuration.

Great work!

drupol · 2015-06-11T18:00:45Z

It works if I do some change in the constructor of my OL objects:

  /**
   * Constructs a Drupal\Component\Plugin\PluginBase object.
   *
   * @param array $configuration
   *   A configuration array containing information about the plugin instance.
   */
  public function __construct(array $configuration, $plugin_id, array $plugin_definition) {
    $this->pluginDefinition = $plugin_definition;

    $this->pluginId = $plugin_definition['plugin module'] . '.' . $plugin_definition['plugin type'] . ':' . $plugin_definition['name'];
    $this->configuration = $configuration;
  }

But $configuration is always empty.

drupol · 2015-06-11T18:06:23Z

Actually, I can now remove the constructor method from Objects.php ! 👍
I don't need the pluginId property.

LionsAd · 2015-06-12T08:19:03Z

@drupol We will need a huge change notice both for service_container and openlayers.

I will also have to fix render_cache first.

drupol · 2015-06-12T08:21:01Z

Huge ?
I can take care of it if you give me more informations. Right now, things hasn't changed much in Openlayers, why would it be so huge ? I don't see what you mean.

LionsAd · 2015-06-12T14:40:31Z

Triggering a rebuild.

…lugins-to-be-compatible-with-Drupal-8 Issue #2504031: Fix __construct of plugins to be compatible with Drupal 8 and take the necessary configuration

LionsAd reviewed Jun 11, 2015
View reviewed changes

Pol Dellaiera added 3 commits June 11, 2015 19:12

Update code to be compatible with D8.

0b5f4b2

Update code according to the comments.

96bdc1b

Update code according to the comments.

2e81b37

LionsAd added the Needs rebase label Jun 11, 2015

LionsAd reviewed Jun 11, 2015
View reviewed changes

Update according to the comment.

af3d652

Update according to the comment.

b863b22

drupol added Needs review and removed Needs rebase labels Jun 11, 2015

LionsAd added the RTBM label Jun 12, 2015

LionsAd added Hold and removed Needs review labels Jun 12, 2015

drupol added this to the 1.0.0 milestone Jun 14, 2015

drupol merged commit d0472c4 into LionsAd:7.x-1.x Jun 16, 2015

drupol changed the title ~~Fix __construct of plugin to be compatible with D8~~ Issue #2504031: Fix __construct of plugin to be compatible with D8 Plugins Jun 16, 2015

drupol removed the Hold label Jun 16, 2015

drupol mentioned this pull request Jun 23, 2015

service_container 1.0-beta3 release note proposal. #70

Closed

drupol deleted the Issue-2504031-Fix-__construct-of-plugins-to-be-compatible-with-Drupal-8 branch June 25, 2015 07:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #2504031: Fix __construct of plugin to be compatible with D8 Plugins #51

Issue #2504031: Fix __construct of plugin to be compatible with D8 Plugins #51

drupol commented Jun 11, 2015

LionsAd Jun 11, 2015

dawehner commented Jun 11, 2015

LionsAd Jun 11, 2015

drupol commented Jun 11, 2015

LionsAd Jun 11, 2015

LionsAd commented Jun 11, 2015

LionsAd commented Jun 11, 2015

drupol commented Jun 11, 2015

drupol commented Jun 11, 2015

LionsAd Jun 11, 2015

LionsAd Jun 11, 2015

LionsAd Jun 11, 2015

drupol commented Jun 11, 2015

LionsAd commented Jun 11, 2015

drupol commented Jun 11, 2015

LionsAd commented Jun 11, 2015

drupol commented Jun 11, 2015

drupol commented Jun 11, 2015

LionsAd commented Jun 12, 2015

drupol commented Jun 12, 2015

LionsAd commented Jun 12, 2015

		@@ -218,6 +218,8 @@ public function set($id, $service, $scope = self::SCOPE_CONTAINER) {
		$this->services[$id] = $service;
		}


		array_unshift($plugin_definition['arguments'], $configuration);

		return $this->container->createInstance($this->servicePrefix . $plugin_id, $configuration);

Issue #2504031: Fix __construct of plugin to be compatible with D8 Plugins #51

Issue #2504031: Fix __construct of plugin to be compatible with D8 Plugins #51

Conversation

drupol commented Jun 11, 2015

LionsAd Jun 11, 2015

Choose a reason for hiding this comment

dawehner commented Jun 11, 2015

LionsAd Jun 11, 2015

Choose a reason for hiding this comment

drupol commented Jun 11, 2015

LionsAd Jun 11, 2015

Choose a reason for hiding this comment

LionsAd commented Jun 11, 2015

LionsAd commented Jun 11, 2015

drupol commented Jun 11, 2015

drupol commented Jun 11, 2015

LionsAd Jun 11, 2015

Choose a reason for hiding this comment

LionsAd Jun 11, 2015

Choose a reason for hiding this comment

LionsAd Jun 11, 2015

Choose a reason for hiding this comment

drupol commented Jun 11, 2015

LionsAd commented Jun 11, 2015

drupol commented Jun 11, 2015

LionsAd commented Jun 11, 2015

drupol commented Jun 11, 2015

drupol commented Jun 11, 2015

LionsAd commented Jun 12, 2015

drupol commented Jun 12, 2015

LionsAd commented Jun 12, 2015