Change PostProcess methods visibility to be able to decorate them in a custom module

[sluc23]

Overview

This is a very specific PR, and probably needs debate to get to a happy ending! Since webform_civicrm moved to D9 version, it includes the Symfony approach using Services. This is a really good approach, but I think it could be improved.

We have a real use case where we need to change the default behaviour of webform_civicrm regarding how it process Memberships. We don't think we will get a consensus to move forwards to merge a PR into core with this feature (old discussion for D7 version could be found here 382 / 383 ) so using the new D9 features, we should be able to develop our D9 custom module to override/decorate the methods of the service webform_civicrm.postprocess and add our custom code. But to be able to decorate some parts of the code.. the visibility of the methods we need to change, need to be at least protected instead of private. Decorators are classes that extends the ones that will override some methods, but needs to be able to access those methods to be "decorated", so that's our proposal, to change that visibility. The Decorators could implement an Interface, instead of extending the class decorated, but that will demand to implement every method specified in the interface, duplicating much of the code from the existing service

This idea could be spread around all webform_civicrm services, and we can discuss which methods of all webform_civicrm could be available for "decoration" in external custom modules, I guess specially those methods that have a very strict business logic inside, which could not fit for some real use cases.

So this is an example on how to develop a custom module to decorate, i.e.: method Drupal\webform_civicrm\WebformCivicrmPostProcess->processMemberships($c, $cid) (once it's been changed to protected in core..)

• webform_civicrm_extras.services.yml:

services:
  webform_civicrm_extras.postprocess:
    class: Drupal\webform_civicrm_extras\PostProcess
    decorates: webform_civicrm.postprocess
    decoration_priority: 5
    public: false
    arguments: ['@webform_civicrm_extras.postprocess.inner', '@webform_civicrm.utils']

• src/PostProcess.php:

<?php

namespace Drupal\webform_civicrm_extras;

class PostProcess extends \Drupal\webform_civicrm\WebformCivicrmPostProcess {

  protected $originalService;

  public function __construct(\Drupal\webform_civicrm\WebformCivicrmPostProcess $originalService, \Drupal\webform_civicrm\UtilsInterface $utils) {
    $this->originalService = $originalService;
    parent::__construct($utils);
  }

  protected function processMemberships($c, $cid) {
    // My custom code
    // I can call original method as $this->originalService->processMemberships();
  }

}

This proposal is based in my Drupal9 / Symfony research, development and testing, but it could be another way to achive this which I'm missing. So in that case, please let me know your thoughts!

Before

Some Business Logic methods in Drupal\webform_civicrm\WebformCivicrmPostProcess->processMemberships($c, $cid) cannot be decorated by other D9 custom modules

After

Methods in Drupal\webform_civicrm\WebformCivicrmPostProcess->processMemberships($c, $cid) now can be decorated by other D9 custom modules

Technical Details

Some references about this topic:

• Drupal 8: Service Decorators
• How to Decorate Services

[KarinG]

Hi @sluc23 - technically -> we're not opposed to making these edits - but is private sufficient? Should it not be public?

I also read through the previous issue write ups - and I think we can accommodate this feature. My main concerns back then were to a) preserve existing behaviour (good or bad - many orgs are counting on that) - and b) it was a new feature so it could not go into D7 -> it had to go into D9 because we have testing in place that is going to help ensure existing behaviour is preserved.

What would you prefer?

[sluc23]

Hi @sluc23 - technically -> we're not opposed to making these edits - but is private sufficient? Should it not be public?

public sounds good too.. protected is the minim visibility needed, but public works too. Would be good to hear other's opinions about which methods are good candidates to this change.

I also read through the previous issue write ups - and I think we can accommodate this feature. My main concerns back then were to a) preserve existing behaviour (good or bad - many orgs are counting on that) - and b) it was a new feature so it could not go into D7 -> it had to go into D9 because we have testing in place that is going to help ensure existing behaviour is preserved.

What would you prefer?

We can do both approaches, right? It would be really nice to have this new feature in core, I vote +1 for it. But my concern is how long it will take us to get it done (after an upcoming long debate I imagine.. but we could sort it out eventually.. :) ) So meanwhile we work on that new feature in core, having this easy PR merged, will give us the chance to develop our custom module to modify the behavior

[jackrabbithanna]

This is a good idea. We've had to patch this file for some customs on a number of occasions. Using service decorator for some other WC services like Fields.php

This shouldn't do any harm for people using WC without customs.

[demeritcowboy]

Some thoughts on whether it should be protected vs public:

1. In the most general situation as I understand it, a decorator does not necessarily extend the original service class, and then this part of the example code above would fail if using "protected":

// I can call original method as $this->originalService->processMemberships();

In the example above, it works because the decorator happens to extend WebformCivicrmPostProcess (as opposed to e.g. just implements WebformCivicrmPostProcessInterface)

2. Changing it to protected now, and then deciding public later, will cause people who went and made code like below in between to then give a fatal error. Although it is an easy change for them - they just need to change theirs to public too - but in the meantime the code fails.

class MyClass extends WebformCivicrmPostProcess {
  protected function processMembership() {
    // do stuff
  }
}

3. Note that a similar fatal error will happen with changing to either protected or public if anyone currently already has code like this, and they've used it to replace the service. They'll need to update the visibility to match the parent after this PR either way whether it's protected or public. I don't know how much code like this is out there. I'm not saying hold off, just if you think there might be a lot then give people a heads-up.

class MyClass extends WebformCivicrmPostProcess {
  private function processMembership() {
    // copy/paste code from the original but change a little
  }
}

4. So if there really is a need for the full range of decorators, then it would lean towards making them public. But if most people are going to just replace the service (and not fully decorate), then protected gives a little extra, well, protection.

[sluc23]

@demeritcowboy public then sounds good to me.. thanks for the explanation and samples!

The biggest issue I see implementing interface instead of inheriting the service base class, is that implementing the interface you'll have to code all the methods in the interface, even if you don't want to decorate them.. and I see a similar braking change issue with interfaces in case in the future webform_civicrm core adds any new methods to it.. That would break all custom modules that implement that interface, saying you must implement ALL methods in the interface.. right?

Anyway, that's why I think we need to get to an agreement for next release on which methods to change visibility, or even add some of them to an interface if needed too, so we have an starting point as base for our custom developments.

[demeritcowboy]

I agree with all that.

One idea but might be overkill is to break it down into more services, e.g. processing memberships could be its own service. Not sure if it's worth the effort.

[sluc23]

coming back to this issue a year after we still have the need to decorate some methods.. in this case my initial approach on extending the decorated class on the decorator is not the best choice, because it won't let other modules to add other decorators.. Extending the class breaks this pattern and as @demeritcowboy mentioned, decorators classes should implement interfaces not extending classes, so in the case of decorating class WebformCivicrmPostProcess the decorator class should be as:

<?php

namespace Drupal\webform_civicrm_decorator;

use Drupal\Core\Form\FormStateInterface;
use Drupal\webform\WebformSubmissionInterface;
use Drupal\webform_civicrm\WebformCivicrmPostProcessInterface;
use Drupal\webform_civicrm\UtilsInterface;

class PostProcess implements WebformCivicrmPostProcessInterface {

  protected $originalService;

  public function __construct(WebformCivicrmPostProcessInterface $originalService, UtilsInterface $utils) {
    $this->originalService = $originalService;
  }

  public function initialize(WebformSubmissionInterface $webform_submission) {
    // add your custom code before/after calling original service method
    return $this->originalService->initialize($webform_submission);
  }

  public function validate($form, FormStateInterface $form_state) {
    // add your custom code before/after calling original service method
    $this->originalService->validate($form, $form_state);
  }

  public function preSave(WebformSubmissionInterface $webform_submission) {
    // add your custom code before/after calling original service method
    $this->originalService->preSave($webform_submission);
  }

  public function postSave(WebformSubmissionInterface $webform_submission) {
    // add your custom code before/after calling original service method
    $this->originalService->postSave($webform_submission);
  }
}

the only issue is that I can only decorate methods that are in the interface, not in the base class, and if I want to change the behavior of membership processing, which is changing method processMemberships($c, $cid) I would need to rewrite the whole postSave() method to be able to change the private call of processMemberships().

So I think it could make sense to add important business logic methods to interface to be able to change their behavior with in custom module

function createContact($contact)
function saveCurrentEmployer($contact, $cid)
function processRelationship($params, $cid1, $cid2)
function processParticipants($c, $cid)
function processMemberships($c, $cid)
function processSharedAddresses()
function processCases()
function processActivities()

or being more radical as demeritcowboy suggested, splitting up all of this in more services..

Thoughts?

[demeritcowboy]

It just means they need to be public, but I don't think anybody was opposed to that, there's just the two breaking issues which probably aren't very common, and they are easy updates:

1. Somebody has already declared one of the existing ones as private in a subclass (fatal because of the mismatch).
2. Somebody has implemented the interface in their own class (fatal because they won't have implemented the new methods yet).

[sluc23]

following again this thread.. I think the whole webform_civicrm services implementation needs to be reviewed.. I'm not an expert here in the service pattern, but my guess is that this implementation of the handler is not the correct one for postprocess service, for instance:

ref: https://github.com/colemanw/webform_civicrm/blob/6.x/src/Plugin/WebformHandler/CivicrmWebformHandler.php#L129-L154

public function postSave(WebformSubmissionInterface $webform_submission, $update = TRUE) {
    $this->civicrm->initialize();
    $processor = \Drupal::service('webform_civicrm.postprocess')->initialize($webform_submission);
    $processor->postSave($webform_submission);
  }

The declared service webform_civicrm.postprocess implemented in the class Drupal\webform_civicrm\WebformCivicrmPostProcess does not offer the possibility of working with it as a a classic service, the only method we could override/decorate in the service is initialize(). In this case postSave() cannot be decorated, because is not being called as a service method, but as a object method.. so unless you override completely the class, and add your implementation of postSave(), that overrides the original one.. it won't be called..

I'm not sure if I'm following correctly this pattern, but the consequence is that is really hard to develop a custom module to change default webform_civicrm behavior in different sections.. services are declared, but internally not really usable as services..

Would be really good to start adding this new features for futures releases, so we can expand the ecosystem of webform_civicrm_* modules..

[demeritcowboy]

Sorry it takes some time to get your head back into this... is the suggestion to break the services into smaller services? That seems like a valid option.