BDE 4.x: move all code in RawDrupalContext to services

[pfrenssen]

This is based on my comment #267 (comment)

Currently we are having a whole bunch of commonly used functionality in RawDrupalContext, and this is extended by all the context classes that BDE provides.

This is problematic, it is practically impossible to modify the behaviour of these methods. The only way a method can be overridden is by extending RawDrupalContext with a custom class like MyCustomContext, but then the modified version is only available to MyCustomContext, all other classes like DrupalContext and FeatureContext will still extend the original RawDrupalContext and are not using the customized behaviour. They are effectively hardcoded to a single implementation that cannot be modified.

For example, imagine you are working on a project that uses single sign-on instead of the standard Drupal login form. You implement your own MyCustomContext class that overrides RawDrupalContext::loggedIn() with your custom logic that checks access with the 3rd party 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. This causes some step definitions to use the original Drupal login form, while others use the SSO provider, chaos!

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, this is the current (bad) implementation:

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();
    }
  }

Is this something we can aim for for the 4.x branch?

[jhedstrom]

Is this something we can aim for for the 4.x branch?

Absolutely! This is the kind of cleanup and refactoring I'd love to see for the next major release!

[pfrenssen]

I have a real use case for this now: I'm using the Email Registration module in a project which alters the standard login form to allow to log in users with their e-mail address rather than their user name. The username field is replaced with an email field. This causes the current implementation to fail, since RawDrupalContext::login() is hardcoded to use $user->name instead of the email address.

We can solve this by extending UserManager or providing a dedicated AuthenticationManager service which can then be swapped out for a version compatible with the Email Registration module. I am wondering though how we can achieve this in a practical way. Currently the services are described in src/Drupal/DrupalExtension/ServiceContainer/config/services.yml. What would be a practical way to allow people to override a service if they have Email Registration installed?

It is not possible to do this in a subcontext, this is too late, the services have all been instantiated already at that point.

[pfrenssen]

Had a look, \Drupal\DrupalExtension\ServiceContainer\DrupalExtension seems to be the way to go. In there the services are loaded from the services.yml file, and it is also responsible for managing the configuration of DrupalExtension. We will probably be able to let people override services by specifying the classes in their behat.yml files.

[jonathanjfshaw]

I'm using Email Registration in D8, and get Behat working by using in behat.yml

text:
        username_field: "E-mail"

But probably you know that and have more complex needs.

[pfrenssen]

Well that only works if you have the option "Also allow to log in using the username" enabled, not in the default configuration where logging in with the username throws an error.

But I don't want to solve this for the sake of Email Registration, but for anyone who doesn't use the standard login form from Drupal core for authenticating. I just think that Email Registration is a nice and simple use case for this issue, it's a lot easier to set up than things like OAuth or single sign on. As an added bonus, I'm actually using Email Registration in a project which is a good motivation for actually getting this done :)

[morsok]

Hello there !

I'm actually having the same issue kinda, we use SSO (shibboleth) and need to do exactly what @pfrenssen is suggesting.

I'm still discovering behat but tell me if I can help !

[pfrenssen]

I'm going to start working on the user authentication service.

[jhedstrom]

Now that user manager and authentication manager are done, what's left to do here? The entity field parser looks like a good candidate, are there others?

[aronbeal]

One area that occurs to me offhand has to do with post-scenario cleanup. We actually use a custom module that calls back to our testing code whenever a new entity is created; this is in response to supplementary nodes that are created in response to the creation of certain node types in our system. The addition of type A automatically creates type B sort of thing. It would be nice to be able to add our own generated content manager that would provide an api for cleanup that the core system would leverage?

[jonathanjfshaw]

The entity field parser looks like a good candidate

I think that's best handled in the bigger context of #337. My idea is use D8's core-plugins component as a composer dependency and build Field and Entity plugin systems using that. Invoking the plugins would be the job of plugin manager field and entity services.

I'm keen to work on it but unsure when I'll be able to.

I think this would help with the cleanup @aronbeal asks for, as entity plugins could have a postDelete method that cleaned up any of their autocreated offspring.

[mlncn]

@pfrenssen @jhedstrom Apologies for the late request for handholding. I promise to document thoroughly... but what is the recommended way to override a service like the AuthenticationManager one introduced in #425 ?

Is it still basically the same approach used in https://www.drupal.org/project/email_registration/issues/2843505 (which code dates from before the above code was merged), using https://github.com/FriendsOfBehat/ServiceContainerExtension ?

Or do these improvements make that unnecessary? In short: Where should a project place a service such as an AuthenticationManagerInterface implementation, and where/how (in behat.yml?) should we instruct that our custom implementation be used?

(Using DrupalExtension 4.x of course.)