Entity & Field Plugins

[jonathanjfshaw]

Addresses #155. I've added a general discussion of top-level matters to that issue.

Some notes for reviewers on implementation matters...

Code flow

The code flow typically begins with a DriverEntityDrupal8 wrapper object.

This then discovers and wraps a DriverEntity plugin, and the DriverEntity plugin creates or loads and wraps a drupal entity.

The DriverEntity Plugin calls DriverField wrapper object when it wants to process field values. The DriverField wrapper discovers and process through matching field plugins.

Review order

I suggest reviewing in this order:

1. Start by looking at the plugins, to see how end developers would work with this stuff.
2. Then look at the Driver's Drupal8 core to see how the driver calls these new systems.
3. Then look at the Entity kernel tests both to get confidence and to see some more API examples.
4. Then look at DriverEntityDrupal8 and it's parent DriverEntityBase. You'll see that this does 3 things:

• gets itself a DriverEntity plugin
• resolves the question of what the bundle is, if and when it can.
• passes almost everything else straight on to the plugin.

5. Then look at DriverEntityPluginDrupal8Base and its parent DriverEntityPluginBase. You'll see that the functionality plugins provide is for 3 main purposes:

• to access the Drupal API and provide information like bundle keys that are needed for plugin discovery etc. to work
• to discover field plugins and process field values through them
• to wrap the real Drupal entity and it's standard methods

6. Next up look at the DriverEntityPluginManager and base to see how plugins are discovered and the right plugin is identified.
7. If you're still alive, look at the DriverField wrapper, plugins, plugin manager and tests.

The bundle dance

A source of major pain in the DriverEntity wrapper is that when creating an entity we are sometimes explicitly told what bundle it should be, sometimes it has no bundle, and sometimes it has a bundle but we have to find it amongst the supplied fields. But the bundle is part of what is (optionally) used to identify the right plugin for an entity, so we don't get the definitive plugin and a drupal entity until we've resolved the bundle.

My solution is to distinguish between a provisional (bundle-agnostic) plugin loaded on a mere entity-type match, and a final plugin (which may or may not be different) that also uses the bundle when considering which plugin to use. This allows us to use plugin methods even before we have resolved the bundle, provided they are not methods that set data on the plugin that would be lost if we switched to a different final plugin. Basically it allows us to load a provisional plugin matched by entity type, ask that plugin what the bundle key and bundle key labels are for this entity type, and use that information to find the bundle field from amongst an array of bundles; once the bundle is set we hen ask for the definitive plugin with which we do much more.

The DriverField wrapper

Th DriverField wrapper does wrap a plugin, but it doesn't wrap a Drupal object in the way that the DriverEntity wrapper does. I imagine the DriverEntity wrapper objects will be stored in the Behat extension once instantiated, instead of the repository of ids we currently have, and could be used for all kind of purposes. The DriverField wrapper is much more of a single trick pony. I do wonder whether it should actually be slimmed altogether and just become a method on the DriverEntity plugin base. The danger is that would fatten up the DriverEntity plugin base, which is already complex.

ExpandEntityFields ignored base fields

The ExpandEntityFields method on the driver made use of the driver's getEntityTypes and isField. This seems to have made expanding not happen for baseFields. This behaviour is undocumented, and I could see no purpose to it, so I have not continued it.

ParseEntityFields

The ParseEntityFields method on the extension's RawDrupalContext is a rats nest of untested branches and loops, that frequently calls in to the driver's getEntityTypes and isField methods. It should probably be using a DriverEntity wrapper to call methods on a DriverEntity plugin, and we could thendeprecte the driver's getEntityTypes and isField. But I'm not sure what the plugin methods should be and figuring it out would require a complete refactoring and unit testing of parseEntityFields.

Tests

There is a failing user kernel test because of the call to validateDrupal in the driver's userCreate, discussed in #155.

[jhedstrom]

Wow, this is an amazing start! For resolving the tests, we'll need to pull down Drupal (not sure we need to add it as a dev dependency or not...) I'll try to give this a proper review sometime soon.

[haringsrob]

I will try to test this later as it looks promising, for now we were in the need of adding all sorts of entities as well and we came up with this:

  /**
   * The entities created.
   *
   * @var \Drupal\Core\Entity\EntityInterface[]
   */
  private $createdEntities = [];

  /**
   * Creates an entity of a given type.
   *
   * @Given :entity with bundle :type content:
   *
   * @throws \Exception
   */
  public function createEntityWithBundle(string $entity_type, string $type, TableNode $table) {
    foreach ($table->getHash() as $entityHash) {
      $entity_data = (object) $entityHash;
      $entity_data->type = $type;

      /** @var \Drupal\Driver\Cores\Drupal8 $core */
      $core = $this->getDrupal()->getDriver()->core;

      $method = new ReflectionMethod(Drupal8::class, 'expandEntityFields');
      $method->setAccessible(TRUE);

      $this->parseEntityFields($entity_type, $entity_data);
      $method->invoke($core, $entity_type, $entity_data);

      $definitions = \Drupal::entityTypeManager()->getDefinitions();
      if (isset($definitions[$entity_type])) {
        /** @var \Drupal\Core\Entity\EntityInterface $class */
        $class = $definitions[$entity_type]->getOriginalClass();
        $entity = $class::create((array) $entity_data);
        if (!$entity->save()) {
          throw new \Exception(sprintf('Entity could not be saved.'));
        }
        $this->createdEntities[] = $entity;
      }
      else {
        throw new \Exception(sprintf('Entity type %entity_type does not exist.', ['%entity_type' => $entity_type]));
      }
    }
  }

  /**
   * Cleanup entities after scenario.
   *
   * @afterScenario
   */
  public function cleanEntities() {
    foreach ($this->createdEntities as $entity) {
      $entity->delete();
    }
  }

Example:

    And "commerce_product" with bundle "default" content:
      | title |
      | test  |
      | test2 |

[jonathanjfshaw]

I've got an old PR for that on the extension: https://github.com/jhedstrom/drupalextension/pull/300

I'm using it as a composer patch, works OK. Doesn't use this new plugin goodness from the driver side, uses the entity handling yo committed to the driver a year ago.

On 12 April 2018 at 16:32, Harings Rob [email protected] wrote:

I will try to test this later as it looks promising, for now we were in the need of adding all sorts of entities as well and we came up with this:

/**

• The entities created.
• @var \Drupal\Core\Entity\EntityInterface[] */ private $createdEntities = [];

/**

• Creates an entity of a given type.

• @Given :entity with bundle :type content:

• @throws \Exception */ public function createEntityWithBundle(string $entity_type, string $type, TableNode $table) { foreach ($table->getHash() as $entityHash) { $entity_data = (object) $entityHash; $entity_data->type = $type;

  /** @var \Drupal\Driver\Cores\Drupal8 $core */ $core = $this->getDrupal()->getDriver()->core;

  $method = new ReflectionMethod(Drupal8::class, 'expandEntityFields'); $method->setAccessible(TRUE);

  $this->parseEntityFields($entity_type, $entity_data); $method->invoke($core, $entity_type, $entity_data);

  $definitions = \Drupal::entityTypeManager()->getDefinitions(); if (isset($definitions[$entity_type])) { /** @var \Drupal\Core\Entity\EntityInterface $class */ $class = $definitions[$entity_type]->getOriginalClass(); $entity = $class::create((array) $entity_data); if (!$entity->save()) { throw new \Exception(sprintf('Entity could not be saved.')); } $this->createdEntities[] = $entity; } else { throw new \Exception(sprintf('Entity type %entity_type does not exist.', ['%entity_type' => $entity_type])); } } }

/**

• Cleanup entities after scenario.
• @afterScenario */ public function cleanEntities() { foreach ($this->createdEntities as $entity) { $entity->delete(); } }

Example:

And "commerce_product" with bundle "default" content:
  | title |
  | test  |
  | test2 |

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/jhedstrom/DrupalDriver/pull/157#issuecomment-380847569, or mute the thread https://github.com/notifications/unsubscribe-auth/ADWYQVBI8U9KXtqbAkQG6sLhQRQoCNUTks5tn3OmgaJpZM4Rdln6 .

[jhedstrom]

I've just kicked tests off for the Drupal Extension with this branch to see what happens.

https://travis-ci.org/jhedstrom/drupalextension/builds/374181918

[jhedstrom]

Looks like the failures would be resolved by rebasing this to include the mail stuff that went in with #134.

[jhedstrom]

I still need to review some of the low-level implementation, but getting tests green here, and on the Drupal Extension will definitely be good :)

[jonathanjfshaw]

I've rebased and force pushed that to the PR, so maybe the extension tests will work now.

I'm working on fixing coding standards, huge amounts of nits to address there.

[jhedstrom]

I re-triggered the test run and there appears to be some interface compatibility issues: https://travis-ci.org/jhedstrom/drupalextension/jobs/374181923

[jonathanjfshaw]

OK, this time the rebase is good, all green locally except the expected fail.

[jonathanjfshaw]

Now kernel tests and extension features passing locally with php5.5 & D8.5.

[jonathanjfshaw]

Coding standards fixed. Now green and clean locally, should be green for CS here.

[jonathanjfshaw]

My attempts to execute the kernel tests building on #188 have foundered because we have a namespace collision with core's composer.json:

    "autoload": {
        "psr-4": {
            "Drupal\\Core\\": "lib/Drupal/Core",
            "Drupal\\Component\\": "lib/Drupal/Component",
            "Drupal\\Driver\\": "../drivers/lib/Drupal/Driver"
        },

hence phpunit gives me

Fatal error: Class 'Drupal\KernelTests\Core\Entity\EntityKernelTestBase' not found in /app/tests/Drupal/Tests/Driver/Kernel/Drupal8/Entity/DriverEntityKernelTestBase.php on line 13

[jonathanjfshaw]

I misdiagnosed the problem above, it wasn't namespace collision, it was just that Drupal doesn't autoload the KernelTests namespace normally, it relies on tests/bootstrap.php to do that.

Tests

I've got things working on Travis, including always running the drupal-extension features for drupal-driver PRs.

Some issues:

• HHVM. Fails a lot of things, doesn't plan to support php 7 or wish to be seen as php engine, and is not supported by symfony4. Therefore I dropped it from Travis as it was just slowing down the return of more interesting test results.
• there are 2 expected kernel test fails to do with user creation
• there are some behat fails that happen at random with php >= 7.0 and D8. They're "cURL error 7: Failed to connect" and always the same 3 scenarios from api.feature fail or pass together. Two of them are obviously user-related; this makes me wonder if they're related to the kernel fails, but that doesn't seem to fit the randomness or error message.
• there's a behat fail for D7 with php 7.2 (caught here first, the extension doesn't yet test php 7.2)

To do

1. Remove the unneccessary driver field wrappers system I created, as discussed in the issue summary above.

2. Make the DriverPluginManagers work with only drupal/core-plugin, no dependency on drupal/core; necessary for D7 support.

3. Create D7 plugins.

4. Test loading plugins from a module.

Help

I could really do with advice on these:

1. We have 2 expected kernel test fails, due to the call to validateDrupal in the driver's userCreate, discussed in #155. Why is that call there? I think it might be a necessary prerequisite for user_cancel (which uses the batch API). We should be able to make our own substitute for user_cancel in plugins (even for D7) that doesn't use batch API, and therefore I think it's fine to remove validateDrupal here if this is the reason for it.

2. There's complex things happening in the driver when it comes to base fields and configurable fields. The ExpandEntityFields method on the driver made use of the driver's getEntityTypes and isField. When I first coded these plugins, this seems to have made expanding not happen for baseFields. This behaviour was undocumented, and I could see no purpose to it, so I have not continued it. But the driver has made changes in this area this year that explicitly mention base fields. I don't know what the desired behavior was in the past, is now, and if I've preserved it (the new D8 plugins I've created work with both base and configurable fields without distinction AFAIK). Have I got it right? Do we have behat features testing expanding for both base and configurable fields?

[jonathanjfshaw]

I've got plugins mostly working for D7, though not pushed here yet as there is more to do for that and some serious refactoring is needed as this tangle of classes to manage it all is too painful.