DrupalExtension and Behat 3

[aronbeal]

Hi,

I've been working with DrupalExtension and Behat context files for about a month now. I'm using Behat 3, where the conversion was made to use Contexts only (no sub-contexts). For your module, that conversion meant that folks had to extend RawDrupalContext instead of DrupalContext. This caused the side effect of having all the functionality established in DrupalContext to be not available generally, unless one used a workaround like capturing said contexts in a BeforeScenario hook so you could ask questions of them later.

This worked all fine, until recently when I was working with which user was logged in at any given moment. It was at this point that I realized that logged in users are localized to the context that created them, and even if I can leverage DrupalContext to create said user, I can't subsequently gain access to the same.

An approach I've been taking with my context files in Behat 3 is to remove the bulk of the functionality to a class that extends RawDrupalContext but does not define any steps in and of itself. The step defining classes subsequently have lots of methods, but they all then leverage the functionality in the parent class to do all the work.

My best approach at this point seems to be to rewrite DrupalContext entirely so that it doesn't actually define any steps, and then create another stub class that has all the step definitions the parent class had. I'm wondering two things:

1. Do you see a problem with this approach I'm missing, or another less laborious avenue I could pursue?
2. If not, would you be interested in the results of this work once I'm done?

[aronbeal]

For reference, my last posting was here, where I was just starting to deal with inter-context communication.

[jhedstrom]

remove the bulk of the functionality to a class that extends RawDrupalContext but does not define any steps in and of itself

This seems like a fantastic idea. Coupled with inter-context communication, that should provide a very flexible framework for your tests.

My best approach at this point seems to be to rewrite DrupalContext entirely so that it doesn't actually define any steps

Would it make more sense to move non-step-defining methods out of there and into the RawDrupalContext? (I don't remember off hand exactly how many of those there are)

would you be interested in the results of this work once I'm done?

Indeed! I'm also happy to give feedback along the way.

[aronbeal]

RawDrupalContext does seem like a good place for all this "shared" context work. Cool. I'll give this a go - will be in touch.

[aronbeal]

First part of my plan of attack:

1. Convert RawDrupalContext $nodes, $users, $user, $terms, $roles, and $languages to be static variables, so they may be properly shared between context instances. (Will cause breakage with existing installs that rely on inspection of $this->user - is that a deal-breaker? It would seem straightforward to ask folks to replace $this->user with self::user in their step definitions). $drupal, $drupalParameters, and $dispatcher seem like they do not need this treatment - tell me if I'm wrong on this.

Also, as an aside, what's your opinion on traits? Behat 3 currently has a PHP 5.3.3+ dependency, and traits are a 5.4 invention, so I'm leaning away from them, but let me know if you think it's safe to give them consideration.

[aronbeal]

Hey, I'm trying to apply these changes (locally) to the master branch on version control, and I'm seeing the following error:

  [Symfony\Component\DependencyInjection\Exception\InvalidArgumentException]                                                                       
  The file "[path-to-drupal-extension]/src/Drupal/DrupalExtension/ServiceContainer/config/services.yml" does not contain valid YAML.  

  [Symfony\Component\Yaml\Exception\ParseException]                                                                           
  The reserved indicator "@" cannot start a plain scalar; you need to quote the scalar at line 37 (near "- @drupal.random").  

Lots of variables in that file starting with '@'. Are you running that yaml file through a preprocessor prior to behat execution or something?

[aronbeal]

Meh. Never mind - I think this is due to me mixing the composer-mandated requirements for drupalextension with the versioncontrol instance - I think I'm using an outdated YAML parser.

Anyways, this is proving simpler than I had originally thought (at this stage - famous last words?). It's not necessary to remove all the functionality from DrupalContext - only the portions that reference shared context items (created during a scenario and susceptible to cleanup.

I've left all the functions as instance methods instead of making them static, so as to minimize the breakage. I've removed all direct references to things like $this->user, and $this->role, and added functions where necessary to allow step defining classes (like DrupalContext) to retrieve the relevant information. I've also tightened up a bit of the code in DrupalContext to reduce redundancy.

I've applied all this work to Master. So far, it's executing ok. Next, I'm planning on modifying my context files to use this new version (and strip redundant functionality away from them).

This is probably sharable with you at this point. What's the preferred way?

[aronbeal]

Hi again. So, forked version here, with changes applied. There's still surely some bugs, and no tests written yet. I'd welcome your thoughts when you have a chance.

[generalconsensus]

Thank you @aronbeal for taking this on -- I'm in the process of working on content-mocking and it's frustrated by the issues you stated with sharing data through contexts. See below:

[aronbeal]

I'm actually pretty pleased with the way my fork is working thus far - I'm honestly not even sure if I need to do context-sharing after these changes. If I could vouch for its resiliency with confidence, I'd open a pull request. I've updated the master branch on mine just today - I'm curious to see how well it'll work for you as a drop-in replacement. (If you have the time to give it a try).

I came to a couple of realizations while working on this:

1. It's not necessary for the logic to be centralized away from the step-defining classes - the only portion where it is really necessary is when you're dealing with shared state. For instance, my variant of the DrupalContext class still maintains most of its logic, but direct references to $this->user (and similar) have been stripped away, and it now relies on inherited function calls for that sort of thing.
2. Rather than storing the created objects directly in the raw context class, it was better to store an object that was responsible for tracking those created objects - this ensures that you can set up those objects and tear them down at very specific points, consolidate their code in a single class (RawDrupalContext), and have them be generally available for storage by sub-contexts (see 'beforeFeature' and 'afterFeature' in RawDrupalContext to see this in action). I abstracted away the storage and maintenance of created objects into a suite of "cache" classes. These classes are responsible for:
   1. Storing created objects
   2. Checking for objects that are feature-similar to ones already stored
   3. Destroying objects during @AfterScenario

The second one is off-spec, but part of what I was trying to achieve was efficiency in creation. If I needed a user with the 'foo' and the 'bar' roles, and I already created one earlier (and that's the only difference I care about), I wanted to reuse that user and speed the process up.

Anyways, that's the gross architecture. I'm trying to pull on a regular basis from master so I don't drift too far from what you're doing. Let me know if you get a chance to try it out.

[aronbeal]

I've got another issue I'm trying to tackle right now. This requires the above work as a basis for operation, so I'm continuing it in this thread. The basic issue is this: I need a way to dynamically reference values for objects created in prior steps. I'm currently implementing an "object alias" solution, where any object created in a gherkin step can have a globally unique alias, which can be referenced in subsequent steps to retrieve specific values from that created object. Gherkin example:

    Given the user:
        | @     | foo_user                            |
        | roles | authenticated user, content creator |
    And the node:
        | status | 0              |
        | uid    | @:foo_user/uid |

Here, I'm using the '@' symbol to define an alias for the object I'm creating. This alias will be stored by the RawDrupalContext subsystem, so when I reference it in subsequent steps, the referencing object will be able to directly retrieve and dynamically translate the requested field value.

I'm mostly not sure about the best syntax to use here. The symbol indicating an alias can't correspond to any possible field name (which I'm mostly hoping here that nobody would call a drupal field '@'), and even harder, the symbol value must contain a character sequence that wouldn't occur in a field value. For lack of a better option, I've settled on '@:' indicating a dynamic value that needs to be translated, but I welcome your thoughts on this.

[aronbeal]

Above aliasing is created and functional. I've replaced the 'current' user with an alias to a particular user using the above system - alias is _current_user_, which is good enough for the moment.

Above work has been pushed to https://github.com/aronbeal/drupalextension development branch.

[aronbeal]

Just for a bit of clarity, all the above work (coupled with the work I mentioned in #276) would allow feature tests similar to the following:

    Given I am the user:
      | roles              | content moderator|
      | @                  | testuser         |
    And the "blog" node:
      | status | 0        |
      | @      | testblog |
    Then my ability to edit the node named "testblog" is "restricted"
    And I set the values of "testblog" to:
      | author        | @:testuser/uid                          |
    Then my ability to edit the node named "testblog" is "allowed"

[aronbeal]

So, an update on this. I've revamped my logic once again to have the caching layer store only the unique key to the object in question (nid for nodes, uid for users, etc), and am having the cache get functions retrieve directly from the database using D7 function calls.

This unfortunately breaks the ability to meld it back into the original, but I wanted a more lightweight caching layer, and I wanted to stop dealing with stale data in certain tests.

I can rework this later to be importable once again, but it will need the addition of "load[node/user/etc]" functionality from the driver layer, which currently does not support loading created objects that I'm aware of.

Remote branch cache testing implements this new behavior.

[generalconsensus]

@aronbeal Thanks for your work! I pulled down the master branch of your work to see how well it's working. How do you feel about exposing all of the field data associated with a entity that was created? I would like to be able to access that as well? Also what is the preferred method for getting the id (nid) of an entity?

[generalconsensus]

@jhedstrom Contingent on the work that aron is doing, I would like to suggest that can create nodes with dummy string content via the devel generate module if it is enabled. Do you think this wise? If we have access to the field values on a generated node than there is shaves off the extra work of creating and formatting a step table since the values are abstracted away for us

Example of this would be dropping in this block at line 59 on Drupal7.php driver file:

    // Auto-generate fields as requested
    if (\module_exists('devel_generate') && !empty($node->autogenerate) && intval($node->autogenerate)) {
        \module_load_include('inc', 'devel_generate', "fields.devel_generate");
        \devel_generate_fields($node, 'node', $node->type);
        $this->autoGenerateFields($node);
    }

[generalconsensus]

@aronbeal is there a reason why you want to create indeces off ints? What about storing field data for reuse later?

[aronbeal]

@generalconsensus, how did it work for you? It's been working well for me, but you're the first other who's tried my fork. I was pulled off this work a bit back to focus on another project, but I'll hopefully be revisiting it soon. When I left it, I was having some concerns about how to make it so it could be most easily folded back into the original. I'll have to pull and spend some time figuring out what was done since I last touched this, in addition to the aforementioned driver layer work. There are also places in the code that I know I haven't accounted for, like all the work that went into entries that span multiple rows - I have no tests for that use case in my fork yet.

My updated driver code loads nodes that I've previously assigned an "alias" to (alias insofar as drupalextension is concerned, not Drupal itself), and I can directly reference properties or field values on a given node via steps like "Then I debug the alias 'my_node/nid'".

Can you expand on what you mean by "indeces off ints"? I need a bit more context on that.

[generalconsensus]

@aronbeal I've been having conflicts trying to reset my local environment to include your drupal driver repo. Please confirm that your local dev environment for building your dev behat/drupal ext environment is this:

{
    "repositories": [
        {
            "type": "git",
            "url": "https://github.com/aronbeal/drupalextension"
        },
        {
            "type": "git",
            "url": "https://github.com/aronbeal/DrupalDriver.git"
        }
    ],

    "require-dev": {
        "behat/behat": "~3.1.0",
        "behat/mink": "^1.7",
        "behat/mink-goutte-driver": "^1.2",
        "behat/mink-selenium2-driver": "^1.3",
        "drupal/drupal-extension": "dev-master",
        "drupal/drupal-driver": "dev-master",
        "behat/mink-extension": "dev-master"
    },
    "minimum-stability": "dev",
    "config": {
        "preferred-install": "source",
        "bin-dir": "bin/",
        "discard-changes": true
    }
}

[aronbeal]

I'm not getting notifications for thread postings here for some reason. Either that, or they're being swallowed by my spam filter. Sorry for the delay in response.

My current root composer.json is as follows:

{
    "require": {
        "drupal/drupal-extension": "dev-feature/add_field_deletion",
        "behat/behat": "~3.0",
        "emuse/behat-html-formatter": "~0.1",
        "behat/mink-zombie-driver": "^1.4",
        "symfony/console": "^3.1"
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "git@github.com:aronbeal/drupalextension.git"
        },
        {
            "type": "vcs",
            "url": "https://github.com/aronbeal/DrupalDriver"
        }
    ],
    "minimum-stability": "dev",
    "prefer-stable": true,
    "config": {
        "bin-dir": "bin/"
    }
}

I would, however, use the setting 'dev-master' for drupal/drupal-driver and drupal/drupal-extension, as I can't vouch for anything not in the main branch.

[aronbeal]

Moving back into development of this codebase in relatively short order, I hope. I see that @pfrenssen has added some user management code that goes in a similar direction as this codebase, but may be difficult to merge, and redundant even if I was successful. I hope this is not a permanent division of this fork, but the enhancements I've built here have been invaluable to us, and we can't go back. I could theoretically make use DrupalUserManager leverage my caching layer to get its work done; it's a superfluous level of indirection, but I'd do it if it meant being able to contribute these changes back.

[pfrenssen]

I wasn't aware of this. Can you please make a PR so I can view the changes? It is a bit difficult to look at your fork and imagine the changes to the code base :)

I indeed solved the same problem, but I came to the conclusion that having a base class containing shared methods is not the right solution for this problem. We will simply move the problem to another layer. There will be a new RawDrupalContext class which will be extended by a whole bunch of other classes. With this suggestion you are unable to modify the behaviour of one of the base methods, since you cannot override the methods any more.

For example, imagine you have a custom login procedure that uses single sign-on instead of the standard Drupal login form. You would implement your own MyCustomRawDrupalContext class that overrides RawDrupalContext::loggedIn() with your custom logic that checks access with the 3rd party SSO authentication provider.

Using the inheritance based approach this now completely falls apart, since there are a bunch of other classes (like DrupalContext, DrushContext, FeatureContext etc) that are still hardcoded to extend the original RawDrupalContext, and are using the original behaviour instead of your custom behaviour.

Using inheritance is the wrong solution for sharing code between classes. Inheritance should be used only to provide different implementations of code that adheres to a specific interface. For solving this problem we should use composition instead. Currently this is usually solved using services and dependency injection.

So a proper solution would be to encapsulate all user authentication related code in a UserManager service that adheres to a UserManagerInterface and call this instead. This means we can then swap out any implementation with a custom one at runtime.

Here's an example from DrupalContext:

class DrupalContext extends RawDrupalContext implements TranslatableContext {

  /**
   * @Given I am an anonymous user
   * @Given I am not logged in
   */
  public function assertAnonymousUser() {
    // This is bad, it relies on the implementation in RawDrupalContext,
    // this behaviour cannot be customized.
    if ($this->loggedIn()) {
      // Same here, the logout functionality cannot be customized, it is
      // effectively hardcoded.
      $this->logout();
    }
  }
}

The solution would be to inject the UserManager service, so that this can be swapped out with custom functionality if needed:

class DrupalContext extends RawDrupalContext implements TranslatableContext {

  /**
   * The user manager service.
   *
   * @var \Drupal\DrupalExtension\UserManagerInterface
   */
  protected $userManager;

  /**
   * @Given I am an anonymous user
   * @Given I am not logged in
   */
  public function assertAnonymousUser() {
    // This is good, we no longer hard code the parent class.
    if ($this->userManager->loggedIn()) {
      // Same here, the logout functionality can now be customized.
      $this->userManager->logout();
    }
  }

[aronbeal]

@pfrenssen, I agree that injection is a better approach to this. At this point, I'll probably need to rewrite this fork significantly (or start afresh) in order to make it compatible with the DI approach. The additional layers I have at this point for aliasing new and existing nodes leverage the same mechanism I had been using for users (storing the alias->id in what amounts to a static hash that persisted across all contexts) might serve better as an injected dependency as well; not sure. I do know that the aliasing system has served us well in our testing, and I'd like to contribute it back at some point if possible.

I don't have a lot of time these days for this unfortunately, but I'd like to keep this ticket open for now, and I'll set up a PR when I have done the aforementioned rewrite. I need to spend some time to look at what you and others have done in detail first. Any thoughts on what I just mentioned are welcome.

[jhedstrom]

Is there anything left to be done here? We have a separate issue for tracking some of the items that came out of this discussion (#316)...

[aronbeal]

While the aliasing we have from this work is invaluable, it needs to be reworked substantially in order to be compatible with recent changes, and that's a ways off. I know you guys have been cleaning house recently; I'll close this for now as there is no further action that can be taken, and open a new one that references this when I can offer a PR or further action.