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

Add auto-register compiler pass #43

Merged
merged 4 commits into from
Jul 5, 2017
Merged

Conversation

kbond
Copy link

@kbond kbond commented May 25, 2017

This is an alternative to #40/#39.

This allows you to leave off the subscribes_to and handles tag attribute for subscribers/handlers that meet the following conditions:

  1. Does not already have subscribes_to/handles tag parameters
  2. Uses __invoke
  3. Has a single, non optional class type hinted __invoke method parameter

The compiler pass auto-adds the proper subscribes_to/handles tag parameter.

In Symfony 3.3, it allows you to simply have this as your config:

services:
    App\Domain\User\CommandHandler\:
        resource: ../../src/Domain/User/CommandHandler
        tags: [command_handler]

    App\Domain\User\EventSubscriber\:
        resource: ../../src/Domain/User/EventSubscriber
        tags: [event_subscriber]

@codecov
Copy link

codecov bot commented May 25, 2017

Codecov Report

Merging #43 into master will decrease coverage by 0.7%.
The diff coverage is 92.3%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #43      +/-   ##
=========================================
- Coverage    90.4%   89.7%   -0.71%     
=========================================
  Files          17      18       +1     
  Lines         271     301      +30     
=========================================
+ Hits          245     270      +25     
- Misses         26      31       +5
Impacted Files Coverage Δ
src/SimpleBusEventBusBundle.php 100% <100%> (ø) ⬆️
src/SimpleBusCommandBusBundle.php 100% <100%> (ø) ⬆️
src/DependencyInjection/Compiler/AutoRegister.php 88.88% <88.88%> (ø)
src/RequiresOtherBundles.php 50% <0%> (-4.55%) ⬇️
...c/DependencyInjection/Compiler/CollectServices.php 46.15% <0%> (-3.85%) ⬇️
...DependencyInjection/Compiler/AddMiddlewareTags.php 72.41% <0%> (-1.67%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 347c884...cad85d2. Read the comment docs.

@magnusnordlander
Copy link

I'm totally 👍 on this approach over the ones in #40/#39.

@dkvk
Copy link

dkvk commented Jun 1, 2017

+1

@cmodijk
Copy link
Member

cmodijk commented Jun 12, 2017

@kbond I really like the clean way of implementing it this way. The only question i have is we should add tests to it. Can you provided these or should i merge it an do it myself?

@kbond
Copy link
Author

kbond commented Jun 13, 2017

Yeah, I wanted to see if this was acceptable before adding tests. I will try to get to writing some this week.

@unkind
Copy link

unkind commented Jun 13, 2017

Hi,

My idea was to avoid changing configuration when event handler is very trivial, e.g.

final class LogUserEvents
{
    public function onUserWasRegistered(UserWasRegistered $event)
    {
        $this->pinba->log('user_was_registered');
    }

    public function onUserActivatedPremiumAccount(UserActivatedPremiumAccount $event)
    {
        $this->pinba->log('user_activated_premium_account');
    }

    // ...
}

It looks waste of time for me to create class per event + fix configuration for cases like this one.
Second case is when event handler is the same for 2 or more events like sending email with verification code.

By the way, my approach also supports __invoke as it has appropriate method signature.

@kbond
Copy link
Author

kbond commented Jun 13, 2017

@unkind I suppose I could loop over the public methods - does anyone foresee any side effects to this?

@unkind
Copy link

unkind commented Jun 13, 2017

@kbond

I suppose I could loop over the public methods

Yes, that was the main idea.

@magnusnordlander
Copy link

@kbond: Yeah, it'd register a listener for each public setter you have on the class.

Also, it's way less clean. With Symfony 3.3 and autowiring, there really isn't any overhead to creating a new listener class. There's no configuration change, it's just a new file.

@kbond
Copy link
Author

kbond commented Jun 13, 2017

I'll leave it as __invoke only for this PR and leave that discussion for a future issue/PR.

@cmodijk cmodijk self-assigned this Jun 14, 2017
@kbond
Copy link
Author

kbond commented Jun 14, 2017

Ok, I added some functional tests and documentation.

@cmodijk
Copy link
Member

cmodijk commented Jun 14, 2017

Yes, I liked the clean solution with only support for the __invoke method if you want more then you need to create a new class. If they share something you can always make a abstract class. With Symfony 3.3 this should not be a problem at al like @magnusnordlander said. It also advocates the 1 class 1 job principle which you should already follow if you use the command -> handler.

@kbond Tnx for the test AND docs! Very happy with it I will review the rest later this week.

@unkind
Copy link

unkind commented Jun 14, 2017

It also advocates the 1 class 1 job principle which you should already follow if you use the command -> handler.

It makes sense when you define what "job" is. SRP is not about it.

@magnusnordlander
Copy link

Ping?

@cmodijk cmodijk merged commit 0fcf941 into SimpleBus:master Jul 5, 2017
@cmodijk
Copy link
Member

cmodijk commented Jul 5, 2017

@kbond Tnx, i just merged this.

@kbond
Copy link
Author

kbond commented Jul 5, 2017

Awesome! Can we get a new release?

@gubler
Copy link

gubler commented Jul 13, 2017

A new release would be very helpful - I want to use this in a project, but I'm not going to set dev in my composer.json.

@cmodijk
Copy link
Member

cmodijk commented Aug 7, 2017

I updated some major things in the library and i'm waiting on crating a new major release because of that. The one thing i'm waiting for is the following I did not have had the time to test this out myself. I was on a holiday the past 3 weeks. I wil try to get the new release going asap.

@cmodijk cmodijk changed the title [RFC] Add auto-register compiler pass Add auto-register compiler pass Aug 28, 2017
@kbond
Copy link
Author

kbond commented Aug 29, 2017

FYI, in 3.4, registering your handlers/subscribers in YAML will be even easier (see symfony/symfony#23991)

Currently, your config might look like this:

services:
    App\Domain\User\CommandHandler\:
        resource: ../../src/Domain/User/CommandHandler
        tags: [command_handler]

    App\Domain\User\EventSubscriber\:
        resource: ../../src/Domain/User/EventSubscriber
        tags: [event_subscriber]

    App\Domain\Article\CommandHandler\:
        resource: ../../src/Domain/Article/CommandHandler
        tags: [command_handler]

    App\Domain\Article\EventSubscriber\:
        resource: ../../src/Domain/Article/EventSubscriber
        tags: [event_subscriber]

    App\Domain\Comment\CommandHandler\:
        resource: ../../src/Domain/Comment/CommandHandler
        tags: [command_handler]

    App\Domain\Comment\EventSubscriber\:
        resource: ../../src/Domain/Comment/EventSubscriber
        tags: [event_subscriber]

In 3.4, this config can be reduced to:

services:
    command_handlers:
        namespace: App\Domain\
        resource: ../../src/Domain/*/CommandHandler
        tags: [command_handler]

    event_subscribers:
        namespace: App\Domain\
        resource: ../../src/Domain/*/EventSubscriber
        tags: [event_subscriber]

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

Successfully merging this pull request may close these issues.

6 participants