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

How useful is it actually to specify the binded events via configuration #10

Open
ChuckJonas opened this issue Aug 28, 2020 · 2 comments

Comments

@ChuckJonas
Copy link
Contributor

ChuckJonas commented Aug 28, 2020

Been thinking about this a bit more, and beside just allowing the current Handler implementations to be migrated to configuration binding without code changes, how useful is it really to specify the binded events via configuration?

I'm struggling to come up with a use case where you would actually change between different bindings, without also having to refactor the code.

It does seem like 9 times out of 10, it makes more sense for the handler itself to declare which event it should be bound to and allowing it to be changed via configuration is actually a liability.

I guess the one place it does seems useful would be to turn it off a single event via unchecking the box... Still, I would argue that a better implementation would be the inverse (EG: "disable before update").

Other frameworks have dealt with this by creating methods where you override on each for each event you want to run:

public class OpportunityTriggerHandler extends TriggerHandler {
  
  public override void beforeUpdate() {
    for(Opportunity o : (List<Opportunity>) Trigger.new) {
      // do something
    }
  }

  // add overrides for other contexts

}

We could very easily implement this via an abstract base class. When detected, the manager would run it regardless of the event. The "stubbed" event methods would just immediately return if not overridden. One nice thing about this approach is you could actually pass in the parameters which are valid for each context (EG void beforeInsert(newRecords, newRecordsMap);

Another option could simply be adding a new EventBinding interface to provide a Set of events to run on. which could also be implemented, along side the Handler.

global interface EventBinding {
      Set<System.TriggerOperation> getEvents();
}
global interface AccountGreeter implements Handler, EventBinding{
    public void handle(){
       for(Account acc : (Account[]) Trigger.new){
          System.debug('Hello ' + acc.Name);
     }

    public void getEvents(){
       return new Set<TriggerOperation> {TriggerOperation.BEFORE_INSERT};
   }
}

Again, the manager would check for instanceOf EventBinding and then call getEvents() instead of pulling them from the configuration. The advantage here is it's very easy to migrate our existing handlers.

I know this seems like a big departure, but I think it's worth taking a hard consideration at how we want to future to look.

With handlers we've already written, we really don't gain that much by migrating them to CMDT anyways. We might as well just leave them statically bound until they need to be significantly updated.

Moving forward, I think it's worthwhile really thinking about what approach makes the most sense before we commit 100% and start building on it.

@ChuckJonas
Copy link
Contributor Author

ChuckJonas commented Aug 28, 2020

I guess there is some advantage in being able to look at the custom metadata and see how everything is bound....

On the flip side, that does mean you are NOT seeing how it's bound when you are just looking at the class.

@ralphcallaway
Copy link
Collaborator

Being able to turn off a trigger handler is nice, and be able to adjust config settings like the exception handling temporarily.

But maybe we can do both? You have to bind in the master trigger, but the handler checks the metadata to see if the trigger handler is off, or it's using some of the exception handling methods.

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

No branches or pull requests

2 participants