On SRP violations in buses and real decorators

[everzet]

This PR is an interactive blog post to demonstrate how easy it is to violate SRP with inheritance and how easy it is to solve such violation with real decorators. As a reference point we'll take this class.

[laracasts]

Firstly, this is a really excellent way to illustrate a concept. So nice job on this, and thank you.

Next, in my defense (haha), we covered this exact thing in the Commands and Domain Events series (which this repo is the product of).

Episode 6: https://laracasts.com/series/commands-and-domain-events/episodes/6 Here's proof: http://cl.ly/image/1w080v2R2t0Z

...But, at the very end of the lesson, I made a note that, if the decorator stuff was confusing, folks could also just use inheritance, and then I demonstrated what that might look like. Maybe I shouldn't have done that, but mostly it was just to ease the worries of people who were really confused at that point. I never brought this class back to the original decorator-specific version. I can do that today or tomorrow.

However, on that note, I'd love to hear what some of you guys think about the current implementation for decorating the command bus (if decoration is even the right term). The basic idea is this: when you execute a command, there are a number of things that you may need to do first:

• Validate the command
• Sanitize stuff
• Whatever else might be appropriate

Now, you have to do that stuff somewhere. The CommandBus offers an option to allow for this (if you want). Something like this, maybe in your controller.

$bus->decorate('SanitizeThingy')->decorate('ValidationThingy')->execute($command);

Basically, behind the scenes, those classes will be resolved out of the IoC container and executed before that actual handler for the command is triggered.

So my questions is: would you call that a decorator (even though it's not the traditional style)? And would you be against such a thing? If yes, how would you prefer to tackle those sorts of sequential tasks that need to be handled? I don't want that stuff injected into a controller unless it's UI-specific (which, if it's form validation, it would be).

My thinking (and I've talked to some others about this stuff) is to treat the CommandBus almost like pipes. "Do this, and then pipe it to that, and then that, and then finally execute the command handler to process the instruction/command."

[mathiasverraes]

I agree that ValidationCommandBus as a proper decorator is a much better solution, for all the reasons @everzet listed. The technique is very useful if you are extending an existing a system that you have no control over. But it is, in effect, a bit of a hack that you'd want to avoid if you do control the system.

Decorators should be about composing behaviour at runtime, where the composable behaviours are still very cohesive with the purpose of the interface being decorated.

So how to resolve it? No silver bullet I'm afraid. Whenever we have this kind of design friction, we need to take a step back and look at what we really want to achieve and why. I'm not intimate with the code in this repo, but I assume you want to make sure you only handle commands that adhere to some validation rules, that are implemented using laravel's validation api.

Stated differently, we are:

1. Protecting a commands validity on the outside of the command.
2. Validating at the last possible moment, right before it gets dispatched.

Nr 1 is inherently procedural. First get a command, then validate it, then execute it. Do this, then do that. Nr 2 means that you are passing potentially passing around invalid objects, not knowing who relies on them being valid.

Objects should be responsible for their own validity. An object is a consistency boundary. So instantiating a command should fail when it's not valid. This concerns only consistently rules that are internal to that command. We don't try to make the command consistent with outside objects. That's the job of the outside objects, for example an entity that is affected by the command. Having all these collaborating objects be responsible for their own consistency, gives for a much safer and more elegant design, and takes way complexity from the client code. The client is now only responsible for the communication between these objects.

HTH.

[mathiasverraes]

@JeffreyWay Posted my comment at the same time as you.

Command Pipelines are a very nice way to "stop the line" if for some reason the command should not be executed. But

• The concept of the pipeline should not be hidden away inside the command bus, that is just messy. Make it explicit, promote it to it's own thing.
• Listeners on the pipeline should not be about the core (domain) intention of the command. Eg a PayForOrder command should be internally consistent, and the Order aggregate that is affected by it, should be internally consistent after the command is executed, or fail if it can't be. The pipeline can deal with concerns such as permissions (is this user allowed to send this command), versioning (eg upgrading a legacy PayForOrder to a PayForOrderV2 command), logging, etc, anything that is not actually about paying for orders.

[laracasts]

I like the idea of having the command be in charge of its consistency. I'm just trying to think of a clean way to do that, if I'm not leveraging an existing validation API, where I can quickly compare an object against a set of rules. For example, I could do:

<?php

class SubscribeUserCommand {

    public $username;

    public $email;

    public function __construct($username, $email)
    {
        $this->setUsername($username);
        $this->setEmail($email);
    }

    public function setUsername($username)
    {
        if (strlen($username) < 3) // or whatever
        {
            throw new InvalidArgumentException;
        }

        $this->username = $username;
    }

    public function setEmail($email)
    {
        if ( ! filter_var($email, FILTER_VALIDATE_EMAIL)
        {
            throw new InvalidArgumentException;
        }

        $this->email = $email;
    }

}

And I guess that would be fine for simple stuff, but it could get messy pretty quickly, I'm thinking. ...Not to mention that you're introducing the potential for validation mistakes every time you re-write that logic, rather than just using a highly tested validation API.

[everzet]

So my questions is: would you call that a decorator (even though it's not the traditional style)? And would you be against such a thing? If yes, how would you prefer to tackle those sorts of sequential tasks that need to be handled? I don't want that stuff injected into a controller unless it's UI-specific (which, if it's form validation, it would be).

@JeffreyWay I'm affraid that is not decorator and calling it so will be misleading or even confusing to anyone learning design patterns while following your screencasts and code.

As of actual pattern you use ($bus->decorate('SanitizeThingy')->decorate('ValidationThingy')->execute($command);) - it is iterative delegation, which is not really a design pattern per se, but used in many projects and frameworks nevertheless.

Actual decorating variant of your code:

$bus->decorate('SanitizeThingy')->decorate('ValidationThingy')->execute($command);

will look something like this:

$workingBus = new SanitizingThingyBus(new ValidatingThingyBus($bus));
$workingBus->execute($command);

And later you can use builder pattern to do fancy code styling like these guys.

[laracasts]

Yes, it's using a more fluent interface like approach, however, in my mind, it certainly feels like a decorator.

"The decorator pattern applies when there is a need to dynamically add as well as remove responsibilities to a class."

This approach...

$bus->decorate('ClassThing')->execute($command);

...specifically allows me to dynamically extend the behavior of the CommandBus. It's not the traditional style, where you inject one object into another, but maybe that's okay.

Either way, if the community agrees that it's not, I'm happy to update the copy to reflect that.

(UPDATE: just saw the last part of your comment above. Will study that a bit.)

[mathiasverraes]

@JeffreyWay With everything public, the object can't guarantee it's consistency. Also, it should be immutable.

I don't mind having internal validation depend on something elegant and simple like beberlei/assert. See https://groups.google.com/d/msg/dddinphp/YGogT1NSbO0/u22c4dgoxdEJ for some of my thought son the matter.

As for depending on laravel's validation: I generally don't like the sort of api that introduces a whole new language just for the purpose of doing something that can easily be done in plain old php.

[laracasts]

Yeah, people seem to disagree on whether commands should use public properties or getters. I haven't found any real need to worry about the public approach, but I understand what you're saying - especially if the command will be responsible for its own consistency.

Will read that PHP DDD thread tonight.

[mathiasverraes]

$bus->decorate() means that the CommandBus' interface knows about the fact that a CommandBus can be decorated, which sort of defeats the purpose. And it violates ISP.

[mathiasverraes]

Yeah, people seem to disagree on whether commands should use public properties

THOSE PEOPLE ARE NOT MY FRIENDS!!! IMMUTABILITY OR DEATH!!!!!!11!1!!1!! :fire: :fire: :fire:

[tcql]

@mathiasverraes to solve that problem of the Bus knowing about decoration, would a more proper pattern involve an external class, say, BusBuilder?

new BusBuilder(new CommandBus)
    ->decorate('SanitizationBus')
    ->decorate('ValidationBus')
    ->execute();

Where execute delegates to the decorated bus?

[everzet]

@tchannel you essentially reinvented decorator builder, which is very powerful pattern and is an increment over basic decoration.

[laracasts]

Yeah, it violates the ISP a bit...for the sake of making it really easy to tack on this added functionality at runtime - in your controller or whatever. I'm trying to make these concepts simple for the every day developer to leverage (the folks who don't even know what a decorator is).

$bus->decorate('SomeClass')->execute($command);

...is incredibly simple and intuitive. If it's not a decorator, then I can change the method name.

Certainly, if there's a better and more elegant way, I'd love to use that instead. I agree that it does violate ISP. But I don't want new A(new B(new C));.

[mathiasverraes]

new MyDecoratingThing(new DecoratedThing). Nice and simple to explain. Less magic, more understanding.

[laracasts]

But it's usually not that simple. If the DecoratedThing has its own dependencies, then you're probably fetching that out of some container. ...which means you're also probably injecting it into a controller. So you're then trying to decorate an already constructed object.

Yes, you can of course get around that. But, again, it's not as clean. Readability is equally as important as SOLID adherence, in my mind.

[tcql]

It seems like the debate between a decorator builder and straight decoration is whether "magic" lowers the barrier for entry for new people, or makes it harder for them to understand what's happening.

There's also an argument for simplifying declaration. The pipe setup feels pretty simple and readable, but the straight decoration setup has the bonus of being very explicit about what's happening.

[everzet]

@laracasts there's a fine fine line between simplification and dumbing down. If you chose education of php developers as your goal, then avoiding well-fitting design patterns for a sake of making things "convenient" wouldn't help anyone. Wouldn't help developers that want to learn software design and wouldn't help you to educate them in that ;)

[laracasts]

Blindly following dogma is rarely a good thing. Adhering to all the SOLID principles 100% is incredibly, incredibly difficult. Anyone who says differently hasn't studied them well enough. In many cases, they are an excellent goal to constantly push for, but almost impossible to achieve perfectly. SRP is easier, but OCP is a great example of this. Nonetheless, I've tried to cover them in great detail at Laracasts.

In my mind, it's about measuring the pros and cons of a particular approach. Show me a cleaner way to tackle this... a method that developers will actually want to use. We'll definitely use it. The API should come first. "This is how it's going to work." Now, what is the cleanest way that I can implement this?

(My wife is glaring at me, so I'll respond to any other comments once I'm home from dinner.)

[taylorotwell]

For what's it's worth: I have a StackPHP style fluent command handler decorator (real decoration) thing on a local branch that I've been working on for the past few days while on vacation, including related interfaces, which could serve as an optional (or included) package for Laravel.

[mathiasverraes]

Either you build the decorator and the decorated and their deps in the DIC setup, or, if that's complex and requires a bunch of rules, you have a factory, and you call the factory from the DIC setup.

Dynamically changing decorators in php is rare, as every request sets it up again.

[mathiasverraes]

Design is hard. SOLID is not dogma. It's a set of principles that some very smart people have discovered by studying code that had desired qualities (adaptable, testable, readable etc) and comparing to code that didn't. The patterns that emerged were described and refined and ultimately given names and easy to remember acronyms. So SOLID is a heuristic to make good design easier, and easier to communicate about.

"SOLID compliance" is not a goal in itself. But "learn the rules, then learn the exceptions" is a good strategy to draw on the insights of others.

[mathiasverraes]

FTR SOLID are not the only OO design principles that have been described. For example, your CommandBus implementations have the following interface

{ public function decorate($className); public function execute($command); }

(I know the actual CommandBus interface only has execute(), but I'm talking about what DefaultCB and ValidationCB truly look like).

This violates something called the Single Level of Abstraction Principle. It's very closely related to ISP and SRP. The methods are at different levels of abstraction: decorate() is at the abstraction level of managing objects and their composition, execute() is at the level of passing Command objects around and performing some behavior. This level is clearly built on top of the object composition level. So it's mixing abstraction levels in a single interface.

[laracasts]

Yeah, that's a valid point.

I'll merge this PR today, and think about some others ways to tackle this.

[laracasts]

Thanks again to Konstantin for the PR/article. This was fun.