thephpleague / tactician

A small, flexible command bus
http://tactician.thephpleague.com
MIT License
858 stars 88 forks source link

Symfony2 Bundle #36

Closed rosstuck closed 9 years ago

xtrasmal commented 9 years ago

Hi Ross,

I'm new to the bundle world, but I couldn't wait to use Tactician inside of a Symfony2 project. So instead of just waiting, I took a small step. If you have any ideas, just spread them out and I'll incorporate them.

I was thinking about calling it the TacticianQuickstartBundle. So for starters, here's how I saw the config.yml after enabling the bundle:

command_bus:
    quickstart:
        CommandBundle\Commands\CreateBlogPostCommand: CommandBundle\Handlers\CreateBlogPostHandler
        CommandBundle\Commands\RenewLicenseCommand: CommandBundle\Handlers\RenewLicenseHandler
        # etc

There is a services.yml:

parameters:
  quickstart.class: CommandBusBundle\Setup\Quickstart
  tactician.commandbus.class: League\Tactician\CommandBus

services:
    tactician.commandbus.factory:
        class: %quickstart.class%
    tactician.commandbus:
        class:   %tactician.commandbus.class%
        factory: ["@tactician.commandbus.factory", create]

After that you're free to add the CommandBus service into the __construct of your Controller of choice.

<?php namespace Blog\Controller;

use League\Tactician\CommandBus;
use Blog\Commands\CreatBlogPostCommand;

class CreateBlogPostController
{

    private $commandbus;

    public function __construct( CommandBus $commandbus )
    {
        $this->commandbus = $commandbus;
    }

    public function createBlogPost()
    {
        $command = new CreatBlogPostCommand('Hello World', 'Lorum Ipsum Dolar');
        $this->commandbus->handle($command);
    }

}

So, yeah.. how do you want it?

xtrasmal commented 9 years ago

Just before you ask me why I didn't use the quickstart class that came with the package: In order to make the above work, I had to add something to the execute method of the CommandHandlerMiddleware class in order to create a object from the handler that came through as a string. Might need to create a compiler pass in order to use the Tactician Quickstart class.

    /**
     * Executes a command and optionally returns a value
     *
     * @throws CanNotInvokeHandlerException
     * @param Command $command
     * @param callable $next
     * @return mixed
     */
    public function execute(Command $command, callable $next)
    {

        $handler = $this->handlerLocator->getHandlerForCommand($command);
        $methodName = $this->methodNameInflector->inflect($command, $handler);

        // is_callable is used here instead of method_exists, as method_exists
        // will fail when given a handler that relies on __call.
        if (!is_callable([$handler, $methodName])) {
            throw CanNotInvokeHandlerException::forCommand(
                $command,
                "Method '{$methodName}' does not exist on handler"
            );
        }

        // Had to add this:
        if ( !is_object($handler) && is_string($handler) ) {
            $handler = new $handler;
        }

        return $handler->{$methodName}($command);
    }
xtrasmal commented 9 years ago

Tommorrow I'll push it all to a github repo.

rosstuck commented 9 years ago

Hi @xtrasmal, thanks for the info (and enthusiasm)! I love that you got this up and running so quickly!

I've been thinking about how to do a Symfony bundle for a little while since the mapping is a bit tricky with the DI container. Here's some ideas:

Middleware Configuration

Right now, you've got a factory method to do the setup and configuration which is a good idea. We still need a way to configure the middleware that get dropped in for different users, though. My first thought was DI tags but that's kind of confusing for defaults and the order middleware run in is really important so we should make it really easy to see, not just use priority attributes on the middleware tags

Therefore, my plan was to do this in the app/config.yml as a list of service ids.

tactician:
    middleware:
        - tactician.middleware.locking
        - tactician.middleware.logging.simple
        - tactician.middleware.doctrine.transactions
        - my_app.middleware.custom
        - tactician.middleware.command_handler

This lets us ship with extra definitions that are optional and folks can easily configure this, order them and even add their own middleware in a pinch. We could still use a factory or compiler pass to set this up. There's always only one command bus in Tactician right now so that's fairly simple, we could always add it as a config option in the future.

Thoughts? :)

Changes to the CommandHandler middleware

I can see why this works but there's a specific interface in Tactician for this: HandlerLocator. We could make a custom HandlerLocator that works specifically for the Symfony DI container and lazy loads those handlers on the fly from the container. That also lets us support handlers with dependencies, etc.

It would just require changing the mapping from the Handler class itself to the service id for the handler (in fact we could go a step further and do the class mapping as a containter tag but that can be done later).

You can see an example of this based on Pimple (so almost the same interface) here https://github.com/thephpleague/tactician/issues/24#issuecomment-73956347

So, in summary, I'm a Symfony guy most of the time as well and would love to have a bundle. This seems like a good start so let me know if these changes sound interesting and we can work together on them going forward. :)

xtrasmal commented 9 years ago

@rosstuck I've just read this and and it goes beyond the Quickstart. Which is great, cause I've been thinking about how to get Middlewares set up as well and we're on the same line with that. So I'll work on that this week, plus look at the HandlerLocator in order to get a concept bundle up and running in a few days.

xtrasmal commented 9 years ago

@rosstuck Hi Ross, I've got the basic raw sketch up for grabs https://github.com/xtrasmal/TacticianBundle Now I'm working on the middlewares stuff. Will push later or tomorrow.

xtrasmal commented 9 years ago

@rosstuck Ok and I've pushed the first go at middleware inclusion. I'm off to bed now, so will try my assumption tomorrow :)


   tactician:
       quickstart:
         # command + handler pairs
          YourName\Commands\DoSomethingCommand: YourName\Handlers\DoSomethingHandler
       middlewares:
         # all your middlewares, top down. First in, first out.
         - YourName\Middleware\DoSomethingMiddleware
rosstuck commented 9 years ago

Cool, looks like you've really hit the ground running. :) Would you prefer I open issues/PR on that repo going forward then? :)

xtrasmal commented 9 years ago

Yeah why not, whatever is easy and how you see it. The main goal is to get the Bundle up and running swiftly and tested. I'm around #dev-discussions (nick Netbulae) if you want to talk.

xtrasmal commented 9 years ago

@rosstuck is it possible to add it to another branch or somethign, so that the bundle can be managed by the league and contributors?

rosstuck commented 9 years ago

@xtrasmal So far, what we've done is let the individual packages mature a bit in their own repos and then when it looks solid, it gets merged to a Tactician plugin package, like say league\tactician-bundle.

I know that means stuff changes for the original authors which stinks and I apologize for, it's just that once it hits the League namespace I'm kinda obliged to care for the quality and future maintenance so I'm a bit hesitant in allowing that.

Also, it gives the original authors the chance to yell and ignore my PRs so they're okay if I do it later. :smile_cat:

I'm a bit swamped this week but I've got some stuff I'd really like to contribute next week if that's alright. I'll open some issues on your repo. :)

xtrasmal commented 9 years ago

@rosstuck Ok cool, I just needed to know if grabbed your intentions correctly. I'll be looking forward on your PR's after you return from the swamp. Don't worry about the stuff changes and and the original author part. Some people mind it, some people just want to boogie all night long. A small mention is always nice to make my mother proud :smile: mhuaha :+1:

bweston92 commented 9 years ago

Any update on this? I'm just starting Symfony and want to use Tactician.

xtrasmal commented 9 years ago

@bweston92 https://github.com/xtrasmal/TacticianBundle <--try it out ;)

rtuin commented 9 years ago

I take it this can be closed now @rosstuck!