Entity & Field Plugins #157

jonathanjfshaw · 2018-01-14T13:08:04Z

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:

Start by looking at the plugins, to see how end developers would work with this stuff.
Then look at the Driver's Drupal8 core to see how the driver calls these new systems.
Then look at the Entity kernel tests both to get confidence and to see some more API examples.
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.

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

Next up look at the DriverEntityPluginManager and base to see how plugins are discovered and the right plugin is identified.
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 · 2018-01-17T19:37:39Z

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 · 2018-04-12T15:32:53Z

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 · 2018-04-12T15:56:43Z

I've got an old PR for that on the extension: jhedstrom/drupalextension#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 ***@***.***> 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 <#157 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ADWYQVBI8U9KXtqbAkQG6sLhQRQoCNUTks5tn3OmgaJpZM4Rdln6> .

jhedstrom · 2018-05-02T22:51:17Z

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 · 2018-05-02T23:01:35Z

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

jhedstrom

Here's a more complete review for now. Thanks so much for all your work on this!

jhedstrom · 2018-05-02T23:03:19Z

src/Drupal/Component/Utility/DriverNameMatcher.php

+
+    /**
+   * Identifies the targets from the pool of candidates.
+   *


Basic indentation fix.

jhedstrom · 2018-05-02T23:05:30Z

src/Drupal/Component/Utility/DriverNameMatcher.php

+        if (is_array($targets)) {
+            $this->targets = $targets;
+        } else {
+            throw new \Exception("Targets to be identified must be passed as an array with their human-friendly name as the keys and anything as the values.");


Is there a reason we can't just type-hint the function? (array $targets)

jhedstrom · 2018-05-02T23:07:24Z

src/Drupal/Component/Utility/DriverNameMatcher.php

+        $this->identifyByMethod("MachineNameWithoutUnderscores");
+        if ($mayHavePrefix) {
+            $this->identifyByMethod("MachineNameWithoutPrefixAndUnderscores");
+        }


The logic above here could use some code comments I think. It would be hard to grok 6-months, or a year, down the road I think.

jhedstrom · 2018-05-02T23:08:02Z

src/Drupal/Component/Utility/DriverNameMatcher.php

+                if ($this->$methodFunctionName($identifier, $machineName, $label)) {
+                    //$this->candidates = array_filter($this->candidates, function ($value, $key) use ($machineName) {
+                     // return $value === $machineName;
+                    //}, ARRAY_FILTER_USE_BOTH);


This looks like it's been moved down below, so can be removed here?

jhedstrom · 2018-05-02T23:09:21Z

src/Drupal/Driver/Cores/Drupal8.php

-    $this->expandEntityFields('node', $node);
-    $entity = Node::create((array) $node);
+    $entity = $this->getNewEntity('node');
+    $entity->setFields((array) $node);


I love how simple this becomes! 👍

jhedstrom · 2018-05-02T23:11:18Z

src/Drupal/Driver/Cores/Drupal8.php

-    $this->expandEntityFields('taxonomy_term', $term);
-    $entity = Term::create((array) $term);
+    $entity = $this->getNewEntity('taxonomy_term');
+    $entity->setFields((array) $term);


jhedstrom · 2018-05-02T23:14:51Z

tests/Drupal/Tests/Driver/DriverNameMatcherTest.php

+/**
+ * Tests the Driver's name matcher utility.
+ */
+class DriverNameMatcherTest extends \PHPUnit_Framework_TestCase


Yay unit tests!

jhedstrom · 2018-05-02T23:18:03Z

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 · 2018-05-09T20:20:54Z

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 · 2018-05-09T22:37:37Z

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

jonathanjfshaw · 2018-05-30T20:52:15Z

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

jonathanjfshaw · 2018-06-01T18:00:56Z

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

jonathanjfshaw · 2018-06-02T13:26:54Z

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

jonathanjfshaw · 2018-06-02T19:56:39Z

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 · 2018-06-04T10:43:29Z

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

Remove the unneccessary driver field wrappers system I created, as discussed in the issue summary above.
Make the DriverPluginManagers work with only drupal/core-plugin, no dependency on drupal/core; necessary for D7 support.
Create D7 plugins.
Test loading plugins from a module.

Help

I could really do with advice on these:

We have 2 expected kernel test fails, due to the call to validateDrupal in the driver's userCreate, discussed in Entity & Field plugins #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.
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 · 2018-07-10T19:25:44Z

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.

This was referenced Jan 14, 2018

Entity & Field plugins #155

Open

Better tests #135

Open

Uninformative error handling with unknown entity/bundle types #118

Open

jhedstrom requested changes May 2, 2018

View reviewed changes

jonathanjfshaw added 5 commits May 9, 2018 19:55

Initial kernel tests

1dff9f8

Field plugin management

e3800d0

Field plugins finished

9a5930c

Finished plugins but not cs

0969fbf

Behat tests passing

1006452

jhedstrom mentioned this pull request May 18, 2018

Better Drupal setup for testing #188

Open

Fix rebase of ExpandEntityFields & DriverNameMatcher

1641d57

jonathanjfshaw added 2 commits May 30, 2018 23:17

Fix dirname for php5

0e76376

Fix failing image test

789db21

Fix coding standards

43f668f

jonathanjfshaw added 2 commits June 3, 2018 20:59

Automated kernel tests on Travis

46d12c3

Refine automated testing on Travis

81a1a32

Tests with behat and on php5.5 & 7.2

cd3b007

jonathanjfshaw mentioned this pull request Jun 9, 2018

Adding a custom field handler jhedstrom/drupalextension#399

Open

jonathanjfshaw mentioned this pull request Jul 19, 2018

Remove all hook implementations from RawDrupalContext jhedstrom/drupalextension#492

Open

jonathanjfshaw closed this by deleting the head repository Apr 18, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Entity & Field Plugins #157

Entity & Field Plugins #157

jonathanjfshaw commented Jan 14, 2018 •

edited

Loading

jhedstrom commented Jan 17, 2018

haringsrob commented Apr 12, 2018

jonathanjfshaw commented Apr 12, 2018 via email

jhedstrom commented May 2, 2018

jhedstrom commented May 2, 2018

jhedstrom left a comment

jhedstrom May 2, 2018

jhedstrom May 2, 2018

jhedstrom May 2, 2018

jhedstrom May 2, 2018

jhedstrom May 2, 2018

jhedstrom May 2, 2018

jhedstrom May 2, 2018

jhedstrom commented May 2, 2018

jonathanjfshaw commented May 9, 2018

jhedstrom commented May 9, 2018

jonathanjfshaw commented May 30, 2018

jonathanjfshaw commented Jun 1, 2018

jonathanjfshaw commented Jun 2, 2018

jonathanjfshaw commented Jun 2, 2018

jonathanjfshaw commented Jun 4, 2018 •

edited

Loading

jonathanjfshaw commented Jul 10, 2018

Entity & Field Plugins #157

Entity & Field Plugins #157

Conversation

jonathanjfshaw commented Jan 14, 2018 • edited Loading

Code flow

Review order

The bundle dance

The DriverField wrapper

ExpandEntityFields ignored base fields

ParseEntityFields

Tests

jhedstrom commented Jan 17, 2018

haringsrob commented Apr 12, 2018

jonathanjfshaw commented Apr 12, 2018 via email

jhedstrom commented May 2, 2018

jhedstrom commented May 2, 2018

jhedstrom left a comment

Choose a reason for hiding this comment

jhedstrom May 2, 2018

Choose a reason for hiding this comment

jhedstrom May 2, 2018

Choose a reason for hiding this comment

jhedstrom May 2, 2018

Choose a reason for hiding this comment

jhedstrom May 2, 2018

Choose a reason for hiding this comment

jhedstrom May 2, 2018

Choose a reason for hiding this comment

jhedstrom May 2, 2018

Choose a reason for hiding this comment

jhedstrom May 2, 2018

Choose a reason for hiding this comment

jhedstrom commented May 2, 2018

jonathanjfshaw commented May 9, 2018

jhedstrom commented May 9, 2018

jonathanjfshaw commented May 30, 2018

jonathanjfshaw commented Jun 1, 2018

jonathanjfshaw commented Jun 2, 2018

jonathanjfshaw commented Jun 2, 2018

jonathanjfshaw commented Jun 4, 2018 • edited Loading

Tests

To do

Help

jonathanjfshaw commented Jul 10, 2018

jonathanjfshaw commented Jan 14, 2018 •

edited

Loading

jonathanjfshaw commented Jun 4, 2018 •

edited

Loading