Move command bus logic into controllers

[tobyzerner]

Currently requests to the API go something like this:

1. Based on the route, we pass a ServerRequestInterface object into an Api\Controller.
2. The controller extracts data from the request, maps it into a command, then dispatches onto the command bus.
3. The command handler receives the command, does its thing, dispatches an event for extensions to do their thing, and finally returns data.

This is nice, but it's not great for extensibility, because the raw request data is inaccessible to extensions, since the command is only given a subset of the request data. So as a crude example, if I wanted to make an extension that alters the discussion title when creating a discussion based on the value of a cookie, it wouldn't be possible.

Now we have two options to solve this:

1. Add an event in the controllers which allows extensions to map other data from the request into the command.
2. Pass the Request object directly into the command, instead of mapping it onto the command. i.e. eliminate step 2 from above.

---

Option 1 would look something like this:

// core
class CreateDiscussionController extends AbstractCreateController
{
    protected function data(ServerRequestInterface $request, Document $document)
    {
        $command = new StartDiscussionCommand($actor = $request->getAttribute('actor'), $etcetera);

        // This method, defined in a superclass, would dispatch an event which allows
        // extensions to map data from $request onto the $command, before dispatching
        // to the command bus.
        $discussion = $this->dispatchCommand($request, $command);

        // Mark the created discussion as read by dispatching another command.
        $this->dispatchCommand($request, new ReadDiscussion($discussion->id, $actor, 1));

        return $discussion;
    }
}

// extension
class ModifyDiscussionTitle
{
    public function subscribe(Dispatcher $events)
    {
        $events->listen(ConfigureCommand::class, [$this, 'mapRequestToCommand']);
        $events->listen(DiscussionWillBeSaved::class, [$this, 'whenDiscussionWillBeSaved']);
    }

    public function mapRequestToCommand(ConfigureCommand $event)
    {
        $event->command->modifyTitle = $event->request->getCookieParams(); // or whatever
    }

    public function whenDiscussionWillBeSaved(DiscussionWillBeSaved $event)
    {
        if ($event->command->modifyTitle) {
            $event->discussion->title .= "MODIFIED";
        }
    }
}

Pros: Well-defined programatic API. Clear separation of concerns (core doesn't need to know about Requests). Cons: More convoluted, harder to reason about, harder to do simple things. More code.

---

Option 2 would look something like this:

// core
class CreateDiscussionController extends AbstractCreateController
{
    public function __construct(CreateDiscussion $createDiscussion, UpdateDiscussion $updateDiscussion)
    {
        $this->createDiscussion = $createDiscussion;
        $this->updateDiscussion = $updateDiscussion;
    }

    protected function data(ServerRequestInterface $request, Document $document)
    {
        // In this case, CreateDiscussion is like a command handler and the Request
        // itself is the command. We inject the command handler into this controller
        // and pass the request right in.
        $discussion = $this->createDiscussion->execute($request);

        // Mark the discussion as read by simulating another request and passing
        // it into another "command handler".
        $readDiscussionRequest = $request
            ->withQueryParams(['id' => $discussion->id])
            ->withParsedBody(['data' => ['attributes' => ['readNumber' => 1]]]);

        $this->updateDiscussion->execute($readDiscussionRequest);

        return $discussion;
    }
}

// extension
class ModifyDiscussionTitle
{
    public function subscribe(Dispatcher $events)
    {
        $events->listen(DiscussionWillBeSaved::class, [$this, 'whenDiscussionWillBeSaved']);
    }

    public function whenDiscussionWillBeSaved(DiscussionWillBeSaved $event)
    {
        if ($event->request->getCookieParams()) { // or whatever
            $event->discussion->title .= "MODIFIED";
        }
    }
}

Pros: Simpler for extensions to do what they want to do, simpler to think/explain/reason about. Cons: Bit of overlap with concerns - core knows about HTTP requests.

---

I think I'm in favour of option 2. As said in the pros, it's simpler for extensions to do what they want to do, simpler to think/explain/reason about. I just don't think there's much advantage to having the command bus in our case – since we're such an extension-oriented application, reflecting the pure "business needs" in the code isn't as important. And since we have a first-class JSON-API right on top, why not just use that as a programatic API too (i.e. passing in Requests instead of Commands)?

[luceos]

I totally agree with you that the command bus is a complicating factor in the extension development process. Once I understood how it worked, I hardly ever found a need to change the default behavior of specifying arguments for the Command.

I'd also like to propose to generalize data a bit if possible to prevent RAW payloads as much as possible. Auto detecting uploads for instance, clear documentation of the actor. I'm not sure whether this is related to your proposal, but it's something I thought about while reading it.

[tobyzerner]

clear documentation of the actor

Covered in flarum/framework#869. Not sure what you mean by "Auto detecting uploads"?

[luceos]

Something like "getUploads()"?

[tobyzerner]

@Luceos that's already part of PSR-7 ServerRequestInterface - and yeah, more relevant to flarum/framework#869

[solhuebner]

I am also in favour for 2 as it is "easier"

[franzliedke]

I'd prefer option 2, too.

Regarding the concerns. How about we combine this with flarum/framework#869 in such a way that the "command handler" does not receive a HTTP request, but only some implementation of an internal "request" or "command", which happens to be implemented by our HTTP request class? That separates the concerns, but still gives us the simplicity of passing request objects into the command bus, right?

[tobyzerner]

Cool idea, that could work :) But wouldn't we end up wanting it to implement most of the ServerRequestInterface methods anyway? (i.e. getCookieParams in my example)

[franzliedke]

We just decided we'll go with number two. :) No additional interface for now.

[franzliedke]

@tobscure Can you come up with another example where we need this change? The one with the cookie data seems a bit far-fetched. ;)

[tobyzerner]

@franzliedke getServerParams is probably a better example. Say, getting the request IP address. We pass the IP address along explicitly (as a command parameter) in StartDiscussion and PostReply... but what if you wanted to make an extension that logs the IP address for other actions? (editing/deleting posts, etc.)

Basically any source of information from ServerRequestInterface other than getParsedBody is currently unavailable to extensions which is bad. And even only a subset of getParsedBody (the data) is available – any meta information that may exist in the JSON document is not.

[franzliedke]

Yeah, I can see that, but on the other hand an extension that would log data for every action would probably be implemented on a different level anyways (not hooking into each and every action), right?

Just making sure we have actually encountered this problem before...

[tobyzerner]

but on the other hand an extension that would log data for every action would probably be implemented on a different level anyways (not hooking into each and every action), right?

Not necessarily? I mean, it depends on the purpose of the extension (I didn't say anywhere that it would log data for every action). My point is that, while we may not have a well-defined use-case for this at the moment, I'm confident that we can't rule out that an extension may want access to this data – and given that there's no workaround, this is very concerning.

[sijad]

custom handler removed in laravel 5.2+, there's two package to add this feature back LaravelCollective/bus (have problem with laravel 5.3) and AltThree/Bus. this option will make things hard for upgrading to incoming laravel version.

options 2 makes some other tasks a little hard, for example if an extension need to create bunch of discussions or users, etc it needs to create custom requests for each one.

option 2 still looks better but can't we add request helper? this way things become far cleaner I guess.

[tobyzerner]

options 2 makes some other tasks a little hard, for example if an extension need to create bunch of discussions or users, etc it needs to create custom requests for each one.

I guess we might have a helper that takes the pain out of constructing requests. But ultimately I can't see it being that much harder than constructing Command objects - they're just conforming to our JSON-API spec rather than to a programmatic interface.

I wouldn't be confident in adding the request helper at least until we fully flesh out how everything is going to fit together. Because it means you wouldn't be able to pass a custom request into a command, since they'd be coupled to the "global" request, does it not?

[sijad]

it means you wouldn't be able to pass a custom request into a command, since they'd be coupled to the "global" request, does it not?

by using something like option 1 there's no need to have more than one request I guess. option 1 would seem like this with request helper:

// core
class CreateDiscussionController extends AbstractCreateController
{
    protected function data(ServerRequestInterface $request, Document $document)
    {
        $command = new StartDiscussionCommand($actor = $request->getAttribute('actor'), $etcetera);

        // This method, defined in a superclass, would dispatch an event which allows
        // extensions to map data from $request onto the $command, before dispatching
        // to the command bus.
        $discussion = $this->dispatchCommand($command);

        // Mark the created discussion as read by dispatching another command.
        $this->dispatchCommand(new ReadDiscussion($discussion->id, $actor, 1));

        return $discussion;
    }
}

// extension
class ModifyDiscussionTitle
{
    public function subscribe(Dispatcher $events)
    {
        $events->listen(DiscussionWillBeSaved::class, [$this, 'whenDiscussionWillBeSaved']);
    }

    public function whenDiscussionWillBeSaved(DiscussionWillBeSaved $event)
    {
        if (request()->getCookieParams()) { // or whatever
            $event->discussion->title .= "MODIFIED";
        }
    }
}```

[sijad]

I'm not really against Option 2, but I think it'll make things complicated in future. as it mentioned above extensions rarely need to request and having multiple request doesn't seems right.

[tobyzerner]

What I'm currently thinking is more radical:

• Where domain logic is thin (ie, basically just mapping request data to model attributes, eg, creating a group), move it directly into the API controller. While this couples the API and some domain logic, we do so for the sake of much simpler extensibility. eg, https://gist.github.com/tobscure/c7940d60750f22208bdc465fa03f382d

• Where domain logic is more substantial (eg, avatar handling), extract it into services as appropriate, without sacrificing extensibility.

[sijad]

can I ask what kind of services?

[tobyzerner]

For example, an AvatarUploader service class. You give it a file upload object and it resizes/moves it and gives you back an avatar path. Then the controller is still responsible for updating/saving the user's avatar path.

[tobyzerner]

My reason for suggesting this:

Ultimately I think in most cases our events shouldn't be on the domain level, but rather need to be higher up to give extensions more flexibility (ie, access to HTTP information). In order to accomplish that, we need to move some of that logic (stuff like mapping input to model attributes) into the HTTP layer too. So if we just eliminate that middleman (commands), we can move the logic which it contained either into services or the models themselves, and then the controllers can use it appropriately.

External code wanting to perform domain actions can either use those services / model methods or can call the JSON-API. For example, where previously you might've gone:

$this->bus->dispatch(new EditDiscussionCommand($discussionId, $rawRequestData));

Now for simple stuff you'd go:

$discussion = Discussion::find($discussionId);
$discussion->update($dataMappedFromRequest);
event(new Discussion\Event\Updated($discussion)); // optionally, if you want to trigger stuff

And for more complex stuff you'd use a service.

I can see the argument that the Updated event should still be triggered in the domain layer... maybe so. Maybe we should call those events inside the models themselves. Or maybe we should indeed have a middleman layer to do that after all. But my point is that an EditDiscussionCommand command which you pass an array of attributes into seems overkill and convoluted. At the very least, I think we should revert to the simplest way of doing things first, and then re-assess the need for that extra layer later.

Certainly the command bus itself is overkill because we will never need alternative command handlers. So if we do end up needing an extra layer, it should just be in the form of service classes.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. We do this to keep the amount of open issues to a manageable minimum. In any case, thanks for taking an interest in this software and contributing by opening the issue in the first place!

[askvortsov1]

• Move logic into controllers
• Separately, allow controllers to be overriden

[askvortsov1]

When this is done, https://github.com/flarum/core/issues/2480 should be closed as moot.