Create EntityContext

[jonathanjfshaw]

Basic features work locally, let's see what Travis says about the before/after hooks.

[jonathanjfshaw]

Fixes #194.

[jonathanjfshaw]

Relies on parallel PR jhedstrom/DrupalDriver#101

[djdevin]

Heh, I think it's only failing because Drupal.org changed their layout and some of the tests are checking regions. The new design launched in September but those tests are likely from before then. Core drupalextension is failing too. https://travis-ci.org/jhedstrom/drupalextension

Looks good though, I tested this PR against jhedstrom/DrupalDriver#113 and was able to add entities, view them, and have them cleaned up in D7.

[jonathanjfshaw]

Great, very good to know.
#310 is the issue to fix the tests for drupalextension to resolve the drupal.org issue.

[jonathanjfshaw]

We need to add make sure the tests this PR adds are tagged for D7 and pass with your PR.

@djdevin Could you try the new tests from this PR make a PR to my repo (source of this PR) with any needed changes?

features/entity.feature is the file; I suggest you add the @d7 tag to any test that should pass in D7. You may want to add/modify some tests if some of the current ones are fundamentally incompatible with D7.

Some of the tests use a comment type defined in fixtures/drupal8/modules/behat_test/config/install/comment.type.user_comments.yml. That will obviously fail unless an equivalent D7 fixture is created.

[jonathanjfshaw]

I thought these should pass now the blackbox tests have been fixed, but there is a failing test.

The FeatureContext function tagged @beforeEntityCreate hook is not firing - the whole function is not being invoked - and so the scenario I created to test it fails. I can't figure out the cause; it needs someone with either a better understanding of the Behat hook system or a better debugging setup than I have.

I couldn't copy the Node/User/Term hooks exactly because they extend an already existing EntityScope, so I had to name mine OtherEntityScope; perhaps somewhere here lies the root of the problem. In particular, I don't know if it matters what constants are used:

abstract class OtherEntityScope extends BaseEntityScope {

  const BEFORE = 'otherentity.create.before';
  const AFTER = 'otherentity.create.after';

}

Perhaps @jhedstrom or @pfrenssen can give a pointer.

[pfrenssen]

This code only works for nodes, not for all entities. The code in FeatureContext is meant as an example to show people how things can be implemented so we should demonstrate best practices.

Instead of checking if certain parameters are set on the entity, we should check its type, I would love this code to look like this:

if ($entity->bundle() === 'node') {
  $entity->name = ...;
  // ...
}

This means however that we have to transform $entity from a stdClass into a real class. I think this would be a good thing though, and something that would be great to have in 4.x.

[Berdir]

This has nothing to do with nodes, nodes don't have a name field. This is a user. also, the method would be getEntityTypeId(), not bundle. bundle is article, page, ...

[jonathanjfshaw]

Yes, it would be great if these entity hooks could know what entity type they are being passed. That could be achieved by a "step_type" property, the same awful hack I'm using to keep track of bundles using "step_bundle" without colliding with entities that have fields of their own called "bundle" or "type". I agree we should create a real object to avoid these awful workarounds.

However, this "step_bundle" syntax has already been committed on the Drupal Driver, and the code is usable without adding a "step_type" argument. I suggest we accept this PR as is, and then rearchitect to use a real class in a subsequent PR as that naturally has consequences/uses beyond this EntityContext. (Although that could be a BC break for anyone who wrote hooks or cleanup code expecting step_bundle to be there). I've opened #337 to discuss the bigger issues here.

[pfrenssen]

I don't like this from a BDD perspective, this is using the machine name of the label property in the step definition. The machine names are not usable for several reasons: first of all they often don't match the domain specific language, they might contain abbreviations and special symbols like underscores, and they are usually in English so including machine names breaks translatability.

Also, not all entities have a label, this is not a required base field. I just had a look and actually it seems like entities with labels are in the minority, most do not have one. We cannot determine it programmatically either because there is no entity key for the label.

I would drop this step.

[pfrenssen]

I guess you've chosen step_bundle to avoid potential conflicts with actual properties like type and bundle on real entities? This is another argument for using a real class for the $entity instead of a plain object.

[pfrenssen]

I think this is making too many assumptions again, not all entities have a canonical path where it can be viewed. For example this cannot be used with File, BlockContent or ContentModerationState entities. I would drop this, the added benefit of directly going to the canonical path doesn't weigh up against the confusion that people will have if their test fails unexpectedly.

I think we only need a single step, to create an entity of a given type and bundle.

[pfrenssen]

Tabs used instead of spaces for indentation, I also saw this in some other places.

[jonathanjfshaw]

I'm currently treating this as postponed on #337 .

[jonathanjfshaw]

Rebased

[brooke-heaton]

This would seem like a critical improvement - we can't yet create Media entities in Drupal 8 with the Extension. Anything we are waiting on here?

[noemi-mancini]

Agree with @brooke-heaton .

We have used this patch extensively on several D8/D9 projects.
With the update to D10 and the module update to v5.0.0rc1, the patch no longer applies, impacting many of our behat tests.
Is there a chance the patch will be integrated, or should we bring it in as custom code?

Edit: sorry, didn't catch this issue before, following there (and thanks for the effort!).