Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Given (a/an) :entity entity/entities/item/items #194

Open
jonathanjfshaw opened this issue Jul 27, 2015 · 17 comments · May be fixed by #661
Open

Given (a/an) :entity entity/entities/item/items #194

jonathanjfshaw opened this issue Jul 27, 2015 · 17 comments · May be fixed by #661

Comments

@jonathanjfshaw
Copy link
Contributor

Following on from: https://www.drupal.org/node/2537618, planning for adding support beyond nodes to the more basic content entities.

JHedstrom commented there:
"In the Drupal Drivers, there would be new methods added. For the changes to the Drupal Extension, these could go into a dedicated EntityContext class so people could enable that as needed."

Questions:

  1. Is "Given (a/an) :entity entity/entities/item/items" the only relevant step definition for basic entities?
  2. Is the turnip syntax I'm suggesting - requiring the keyword "entity" or "item" - the right one?
  3. Am I correct that implementation would require
  • a nakedEntityCreate and createNakedEntity corresponding to every nodeCreate and createNode that there is in the Drupal Driver (D7 & D8) and Drupal Extension;
  • likewise for other "node" syntax like nodeDelete, beforeNodeCreate, etc.
  • a step definition using the syntax above
  • a feature to test this module is working right

Lastly, teardown is a significant challenge. AFAIK this project removes test data by removing all data created by the user created in the test process. However, basic entities do not necessarily record who created or owns them. My suggestion is that when working with basic entities we would have to record the full set of fields specified in the Given, and then delete entities that match those fields.

This could potentially delete too many entities in the case that the supplied fields overlapped with existing entities of the type, so we would need to document clearly the importance of giving novel values for the fields of the entities to be Given...
"Given a Pizza item with Flavor "test flavor gdfdfsh"

@jhedstrom jhedstrom added this to the 3.1.0 release milestone Jul 28, 2015
@jhedstrom jhedstrom removed this from the 3.1.0 release milestone Aug 26, 2015
@jonathanjfshaw
Copy link
Contributor Author

http://cgit.drupalcode.org/lightning/tree/tests/features/bootstrap/entity.behat.inc?h=8.x-1.x
Looks like this functionality has been created for the Drupal Lightning distribution. Hopefully it's ripe for picking out and including here.

@djdevin
Copy link

djdevin commented Jul 29, 2016

Basic subcontext I wrote up that does the basics. Took some inspiration from Lightning but only kept what I needed.

This is for D7, understandable it needs to be abstracted for D8. I named the steps similar to how they are for content in Drupalextension.

<?php

/**
 * @file
 * Contains \EntitySubContext.
 */
use Drupal\DrupalExtension\Context\RawDrupalContext;
use Drupal\DrupalExtension\Context\DrupalContext;
use Drupal\DrupalExtension\Context\MinkContext;
use Behat\Gherkin\Node\TableNode;

/**
 * Subcontext for creating and cleaning up entities of any type.
 */
class EntitySubContext extends RawDrupalContext {

  /** @var DrupalContext */
  private $drupalContext;

  /** @var MinkContext */
  private $minkContext;

  /**
   * Set up the contexts we will be using.
   *
   * @BeforeScenario
   */
  public function gatherContexts(Behat\Behat\Hook\Scope\BeforeScenarioScope $scope) {
    $environment = $scope->getEnvironment();
    $this->drupalContext = $environment->getContext('Drupal\DrupalExtension\Context\DrupalContext');
    $this->minkContext = $environment->getContext('Drupal\DrupalExtension\Context\MinkContext');
  }

  /**
   * @Given I am viewing a/an :entity_type entity with the label :label
   * @Given I am viewing a/an :bundle :$entity_type entity with the label :label
   */
  public function assertViewingEntity($entity_type, $label, $bundle = NULL) {
    if ($entity_info = entity_get_info($entity_type)) {
      if (empty($bundle)) {
        $bundle = $entity_type;
      }

      $values = array();

      if (isset($entity_info['entity keys']['bundle'])) {
        $values[$entity_info['entity keys']['bundle']] = $bundle;
      }

      if (isset($entity_info['entity keys']['label'])) {
        $values[$entity_info['entity keys']['label']] = $label;
      }

      $entity = entity_create($entity_type, $values);
      $entity->save();
      $this->entities[$entity_type][$entity->identifier()] = $entity;
    }
    else {
      throw new Exception("Entity type $entity_type not found.");
    }
  }

  /**
   * @Given I am viewing a/an :entity_type entity:
   */
  public function createEntity($entity_type, TableNode $data) {
    if ($entity_info = entity_get_info($entity_type)) {
      $values = $data->getRowsHash();
      $entity = entity_create($entity_type, $values);
      $entity->save();
      $this->entities[$entity_type][$entity->identifier()] = $entity;
    }
    else {
      throw new Exception("Entity type $entity_type not found.");
    }
  }

  /**
   * @Given :entity_type entities:
   */
  public function createEntities($entity_type, TableNode $data) {
    if ($entity_info = entity_get_info($entity_type)) {
      foreach ($data->getHash() as $hash) {
        $values = array();
        foreach ($hash as $k => $v) {
          $values[$k] = $v;
        }

        $entity = entity_create($entity_type, $values);
        $entity->save();
        $this->entities[$entity_type][$entity->identifier()] = $entity;
      }
    }
    else {
      throw new Exception("Entity type $entity_type not found.");
    }
  }

  /**
   * @Given I visit the :entity_type with the label :title
   */
  public function visitEntity($entity_type, $title) {
    if ($entity_info = entity_get_info($entity_type)) {
      if (isset($entity_info['entity keys']['label'])) {
        $entities = entity_load($entity_type, FALSE, array($entity_info['entity keys']['label'] => $title));
        /* @var $entity Entity */
        $entity = reset($entities);
        $uri = $entity->uri();
        $this->minkContext->visit($uri['path']);
      }
      else {
        throw new Exception("Can't reference $entity_type by label.");
      }
    }
  }

  /**
   * Deletes all entities created during the scenario.
   *
   * @AfterScenario
   */
  public function cleanEntities() {
    foreach ($this->entities as $entity_type => $entities) {
      entity_delete_multiple($entity_type, array_keys($entities));
    }
  }

}

@djdevin
Copy link

djdevin commented Jul 29, 2016

Testing an entity type created from ECK:

Scenario: I need to create entities
Given I am viewing a "course_application" entity with the label "My course app 1"
Given I visit the "course_application" with the label "My course app 1"
Given "course_application" entities:
| type | title |
| course_application | My course app 2 |
| course_application | My course app 3 |
| course_application | My course app 4 |
Given I am viewing a "course_application" entity:
| type | course_application |
| title | My course app 5 |
Given I am logged in as a user with the "eck administer entity types,eck administer entities" permission
When I visit "admin/structure/entity-type/course_application/course_application/list"
Then I should see "My course app 1"
Then I should see "My course app 2"
Then I should see "My course app 3"
Then I should see "My course app 4"
Then I should see "My course app 5"

@jonathanjfshaw
Copy link
Contributor Author

Great. I was thinking to work on this for D8 in the coming week or 2 anyway, maybe you can help me with the D7 part? I'll build on your work, and as you say try to abstract it a bit.

@djdevin
Copy link

djdevin commented Aug 3, 2016

Absolutely, my guess it it would go in the Drupal7/Drupal8 implementations of AbstractCore.

@jonathanjfshaw
Copy link
Contributor Author

OK, I've got something that works for D8.

Todo:

  • fix coding standards
  • test the before-after hooks. I've added tests for these to the drupal extension, but don't know how to run them locally
    • see if the errors are helpful enough when required fields are missed out in steps
  • support d7

@djdevin It turned out that adding this to the Drupal extension itself is very different from a straight port of your/Lightning's code. But extending it to D7 is a fairly contained task: there's just an entityCreate and entityDelete stub in DrupalDriver that need filling in. Are you up for it? I know little about D8 but nothing at all about D7.

Feel free to PR my repo and I'll quickly merge your PR, or make a follow-up PR here, whatever works for you.

@jonathanjfshaw
Copy link
Contributor Author

jonathanjfshaw commented Aug 8, 2016

I've added a File field handler as well. Now we can have file entities as "given", in D8 field fields can be treated as just entity references to file entities. I haven't created tests for it, partly because it's nothing more than an empty wrapper and partly because creating the fixtures needed is hard for me.
This now works for me locally:

  Scenario: Test a file reference 
    Given a "file" entity:
    | uri               | 
    | public://test.txt |
    Given I am viewing an "article":
    | title        | test article |
    | field_myfile | test.txt     |
    Then I should see "test.txt"
    When I am at "admin/content/files"
        Then I should see "test.txt"

@jonathanjfshaw
Copy link
Contributor Author

Some issues with bundle names other than "type", I will have a fix shortly.

@jonathanjfshaw
Copy link
Contributor Author

Bundles fixed. I've had to adopt the ugly convention of passing around the entity_type and bundle as array/object keys "step_entity_type" and "step_bundle" in order to avoid collision with other fields. For example, comments have a required field called "entity_type" that references the entity type the comment is about.

This is part of a more general issue already present for Nodes etc. : we're passing complex objects around to represent entities, back and forth between the driver and extension as well as within each, and these have no predefined structure or interface. The entity object we pass around is basically a collection of desired field names and values, and so it gets messy when we went to add non-field values like entity type and these sit amidst the fields with no defined way of distinguishing them, and potentially colliding with them. Ideally we could have some kind of interface that defines properties: entity_type, bundle, id, canonical url, edit url, and fields. So it's entity->fields that has the collection of desired fields.

This should be a separate issue however as it requires refactoring beyond the scope of this issue and is not caused by the new EntityContext, it's already present.

@djdevin
Copy link

djdevin commented Aug 12, 2016

Feel free to PR my repo and I'll quickly merge your PR, or make a follow-up PR here, whatever works for you.

Sure, I think getting it done first in D8 would be best so I could just do a followup PR and write the D7 implementation.

The entity object we pass around is basically a collection of desired field names and values, and so it gets messy when we went to add non-field values like entity type and these sit amidst the fields with no defined way of distinguishing them, and potentially colliding with them. Ideally we could have some kind of interface that defines properties: entity_type, bundle, id, canonical url, edit url, and fields. So it's entity->fields that has the collection of desired fields.

Even outside of Behat I think it's safe to assume that there shouldn't be a field that is called entity_type. IIRC in Drupal 7 this was also possible, you could create a field that clashed with a property.

I haven't looked at your PR yet but If you take a look at my steps though I put the entity type and bundle into the step itself, so that the defined id/bundle key would be used, and the table would only contain properties and fields. The test writer should not have to know what those keys are.

@jonathanjfshaw
Copy link
Contributor Author

jonathanjfshaw commented Aug 12, 2016

Even outside of Behat I think it's safe to assume that there shouldn't be a field that is called entity_type.
Sadly that's not true. The core comments entity has a field called "entity_type". It surprised me, but if that could be true then we also can't assume that no one would ever want to create a field called "bundle".

I haven't looked at your PR yet but If you take a look at my steps though I put the entity type and bundle into the step itself ...The test writer should not have to know what those keys are

Agreed, and I have implemented the same. (although I have also allowed bundle to be set by column as well). The challenge is in passing entity_type and bundle to the driver, hooks and cleanup without colliding with field names.

@djdevin
Copy link

djdevin commented Aug 12, 2016

Sadly that's not true. The core comments entity has a field called "entity_type". It surprised me, but if that could be true then we also can't assume that no one would ever want to create a field called "bundle".

Ridiculous :) Does the same apply for a field called "nid"? I thought all user fields had to be prefixed with field_ unless they were created programmatically?

@jonathanjfshaw
Copy link
Contributor Author

Certainly fields I create through UI get "field_". Basefields created programmatically don't seem to need that. So yes, there could very well be one called "nid"! A developer making a custom entity that always had a parent node might think to use "nid" for a basefield. The joy of catering for Drupal's infinite use cases ...

@jhedstrom jhedstrom added this to the 4.0 release milestone Sep 28, 2016
@djdevin
Copy link

djdevin commented Oct 13, 2016

I opened jhedstrom/DrupalDriver#113 for the D7 driver functionality.

Going to test against #300 in a bit.

Seems like this issue isn't needed any more. Close?

@MPParsley
Copy link
Contributor

Here's some documentation in order to get this working:

    "require-dev": {
        "drupal/drupal-extension": "^4.0",
    },
    "extra": {
        "patches": {
            "drupal/drupal-extension": {
                "Entity support": "patches/jhedstrom_drupalextension_300.patch"
            }
        }
    },

@MPParsley
Copy link
Contributor

Any update on this? It would be a major improvement.

@jhedstrom
Copy link
Owner

The key pieces are already in the Drupal Driver, so if somebody wants to pick up #300 and push it through that would be great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants