Issue #2817751 by bojanz: Create an API for bundle plugins #49

bojanz · 2016-10-13T12:08:51Z

No description provided.

klausi · 2016-10-13T16:43:41Z

entity.module

+function entity_entity_type_build(array &$entity_types) {
+  foreach ($entity_types as $entity_type) {
+    if ($entity_type->get('bundle_plugin_type')) {
+      $entity_type->setHandlerClass('bundle_plugin', 'Drupal\entity\BundlePlugin\BundlePluginHandler');


should use BundlePluginHandler::class instead of the string

Hm, are we fine with the fact that doing so will cause BundlePluginHandler to be autoloaded on every request?

autoloaded just because you use ::class???? why would you need to load the class for that? Please dear PHP god no ...

Phew, I just tested this and of course the ::class syntax does not invoke the autoloader. You almost got me there!

Nice! I learned something new. I assumed the use statement would be enough to trigger it.

Yeah, I remember this discussion. We used to use FQDN in install files instead of use statements before 8.0.x release. Then we cleared the idea that a use statement is not equal to include/include_once.

Berdir · 2016-10-13T16:51:56Z

So this is for the case where the you directly set the plugin on the content entity and that defines the bundle, correct? So like payment.module for example handles the payment type. And not like media_entity.module, which has bundles and then the bundle has a plugin, but you could have multiple bundles using the same plugin?

bojanz · 2016-10-13T16:58:00Z

The plugin is the bundle. $entity->type stores the plugin ID. Instead of saying "this entity type uses config bundles, discover them and use them" we say "this entity type uses a plugin bundle, discover them and use them".

So code instead of config driven. And you can always have an $entity->getBundlePlugin() method that gives you the plugin on which you can call other methods.

larowlan

Love this. Awesome work @bojanz

larowlan · 2016-10-13T20:47:50Z

src/BundlePlugin/BundlePluginInstaller.php

+  public function installBundles(EntityTypeInterface $entity_type, array $modules) {
+    $bundle_handler = $this->entityTypeManager->getHandler($entity_type->id(), 'bundle_plugin');
+    $bundles = array_filter($bundle_handler->getBundleInfo(), function ($bundle_info) use ($modules) {
+      return in_array($bundle_info['provider'], $modules);


nit, should pass TRUE as third param

Have we started enforcing this in core? I'm fine with adding it, just don't see it usually (the array is full of strings here, and strict handling is usually important only for numbers)

larowlan · 2016-10-13T20:48:36Z

src/BundlePlugin/BundlePluginInstaller.php

+  public function uninstallBundles(EntityTypeInterface $entity_type, array $modules) {
+    $bundle_handler = $this->entityTypeManager->getHandler($entity_type->id(), 'bundle_plugin');
+    $bundles = array_filter($bundle_handler->getBundleInfo(), function ($bundle_info) use ($modules) {
+      return in_array($bundle_info['provider'], $modules);


larowlan · 2016-10-13T20:50:19Z

...s/entity_module_bundle_plugin_test/src/Plugin/BundlePluginTest/BundlePluginTestInterface.php

+
+use Drupal\entity\BundlePlugin\BundlePluginInterface;
+
+interface BundlePluginTestInterface extends BundlePluginInterface {


nit: missing docblock. Do we need this or could the manager just look for the parent interface?

Added it as an example (cause you'd usually have other methods in there), but I agree that it contributes nothing in this particular case, I'll remove it.

D'oh, Plugin API actually requires an interface to exist.

larowlan · 2016-10-13T20:52:14Z

tests/src/Kernel/BundlePluginTest.php

+  public function testPluginBundles() {
+    $bundled_entity_types = entity_get_bundle_plugin_entity_types();
+    /** @var \Drupal\Core\Entity\EntityTypeInterface $entity_type */
+    $entity_type = reset($bundled_entity_types);


could this lead to a fragile test? The results are keyed by ID, we could access it direct, without the reset()

Yeah, there's no reason for that reset.

larowlan · 2016-10-13T20:54:33Z

I assume when combined with the plugin module, this gives you the ability to have a field type of 'plugin' that refers back to the instance of the bundle. So you could put additional logic in your interface/on the bundle. $entity->bundle_plugin->instance->doSomething() which is sweet.

bojanz · 2016-10-13T21:09:37Z

@larowlan
Sure. I do this in Commerce, no need for Plugin module:

  public function getType() {
    $payment_type_manager = \Drupal::service('plugin.manager.commerce_payment_type');
    return $payment_type_manager->createInstance($this->bundle());
  }

Then $payment->getType()->doSomething().

kimpepper · 2016-10-13T21:17:22Z

src/BundleFieldDefinition.php

+ * example uses BaseFieldDefinition, which is wrong. Tracked in #2346347.
+ *
+ * Note that this class implements both FieldStorageDefinitionInterface and
+ * FieldStorageDefinitionInterface. This is a simplification for DX reasons,


Does it implement the same FieldStorageDefinitionInterface twice?

Good catch!

kimpepper · 2016-10-13T21:18:42Z

src/BundlePlugin/BundlePluginHandler.php

+use Drupal\Core\Entity\EntityTypeInterface;
+use Symfony\Component\DependencyInjection\ContainerInterface;
+
+class BundlePluginHandler implements BundlePluginHandlerInterface {


Class comment.

I wasn't planning on adding one. I don't see a point in a comment that doesn't add any new information, such as "Default implementation of the BundlePluginHandlerInterface."

klausi · 2016-10-13T21:51:27Z

tests/src/Kernel/BundlePluginTest.php

+    $bundle_info = $entity_type_bundle_info->getBundleInfo('entity_test_bundle_plugin');
+    $this->assertEquals(2, count($bundle_info));
+    $this->assertTrue(isset($bundle_info['first']));
+    $this->assertTrue(isset($bundle_info['second']));


could use ->assertArrayHasKey() for better readability and verbosity should the test ever fail.

Thanks, addressed.

jibran · 2016-10-13T22:05:18Z

To summarize this we can say, a content entity can have different ~~base~~fields based on different bundles(instances of bundle plugin type).
Edit: These are not basefields

 class BundleFieldDefinition extends BaseFieldDefinition {

   /**
    * {@inheritdoc}
    */
   public function isBaseField() {
     return FALSE;
   }

 }

jibran · 2016-10-13T22:10:53Z

entity.module

+    return $entity_type->hasHandlerClass('bundle_plugin');
+  });
+
+  return $entity_types;


I think we should static cache this.

Why? It's used when rebuilding the cached bundle or field data, it's not in any hot path. Plus, the function is not doing any I/O, it's just filtering an array.

It was just and uber nit pick. I thought hook_modules_installed and hook_entity_bundle_info can be called on same request.

Okay, let's skip that then.

jibran · 2016-10-13T22:22:16Z

tests/src/Kernel/BundlePluginTest.php

+  /**
+   * Tests the bundle plugins.
+   */
+  public function testPluginBundles() {


Can we add some EFQ asserts here as well? Just to make sure everything is dandy.

janstoeckler · 2016-11-22T12:33:09Z

src/BundlePlugin/BundlePluginHandler.php

+    $bundles = [];
+    foreach ($this->pluginManager->getDefinitions() as $plugin_id => $definition) {
+      $bundles[$plugin_id] = [
+        'label' => $definition['label'],


Would it be feasible to add something like 'description' => $definition['description'], here? This would enable us to add a bundle description & make the add-page considerably nicer looking.

This sounds like a nice idea!

dawehner · 2017-09-12T12:01:29Z

src/BundlePlugin/BundlePluginHandler.php

+  public static function createInstance(ContainerInterface $container, EntityTypeInterface $entity_type) {
+    return new static(
+      $entity_type,
+      $container->get('plugin.manager.' . $entity_type->get('bundle_plugin_type'))


Should we catch the exception here and maybe provide a helpful error message "you need to implement a plugin manager for your bundle type called '....'"

I think this is overengineering. The backtrace will show what is going on

dawehner · 2017-09-12T12:02:30Z

src/BundlePlugin/BundlePluginHandler.php

+    $bundles = [];
+    foreach ($this->pluginManager->getDefinitions() as $plugin_id => $definition) {
+      $bundles[$plugin_id] = [
+        'label' => $definition['label'],


This sounds like a nice idea!

dawehner · 2017-09-12T12:04:44Z

src/BundlePlugin/BundlePluginHandlerInterface.php

+  public function getBundleInfo();
+
+  /**
+   * Gets the field storage definitions.


nit: Let's add documentation about the return type.

dawehner · 2017-09-18T10:16:54Z

I think we are good with this PR now. 🎆

klausi reviewed Oct 13, 2016

View reviewed changes

larowlan reviewed Oct 13, 2016

View reviewed changes

kimpepper reviewed Oct 13, 2016

View reviewed changes

klausi reviewed Oct 13, 2016

View reviewed changes

jibran suggested changes Oct 13, 2016

View reviewed changes

janstoeckler reviewed Nov 22, 2016

View reviewed changes

dawehner reviewed Sep 12, 2017

View reviewed changes

bojanz and others added 5 commits September 12, 2017 14:25

Issue #2817751 by bojanz: Create an API for bundle plugins

459c33b

Address feedback.

57f7931

Validate uninstallation of bundle plugins (#2)

9ac0ff3

feat(Add support for bundle descriptions):

f1a244b

test(Add tests using an entity query):

f95c543

dawehner merged commit 061376a into fago:8.x-1.x Sep 18, 2017


		use Drupal\entity\BundlePlugin\BundlePluginInterface;

		interface BundlePluginTestInterface extends BundlePluginInterface {

Issue #2817751 by bojanz: Create an API for bundle plugins #49

Issue #2817751 by bojanz: Create an API for bundle plugins #49

Conversation

bojanz commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bojanz Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Berdir commented Oct 13, 2016

bojanz commented Oct 13, 2016 • edited

larowlan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

larowlan commented Oct 13, 2016

bojanz commented Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jibran commented Oct 13, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dawehner commented Sep 18, 2017

bojanz Oct 13, 2016 •

edited

bojanz commented Oct 13, 2016 •

edited

bojanz commented Oct 13, 2016 •

edited

jibran commented Oct 13, 2016 •

edited