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

Conversation

Projects
None yet
6 participants
@kbond
Contributor

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

This comment has been minimized.

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

This comment has been minimized.

magnusnordlander commented Jun 1, 2017

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

@dkvk

This comment has been minimized.

dkvk commented Jun 1, 2017

+1

@cmodijk

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

Contributor

kbond commented Jun 13, 2017

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

@unkind

This comment has been minimized.

unkind commented Jun 13, 2017

@kbond

I suppose I could loop over the public methods

Yes, that was the main idea.

@magnusnordlander

This comment has been minimized.

magnusnordlander commented Jun 13, 2017

@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

This comment has been minimized.

Contributor

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 added some commits Jun 14, 2017

@kbond

This comment has been minimized.

Contributor

kbond commented Jun 14, 2017

Ok, I added some functional tests and documentation.

@cmodijk

This comment has been minimized.

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

This comment has been minimized.

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

This comment has been minimized.

magnusnordlander commented Jun 26, 2017

Ping?

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

3 checks passed

codecov/patch 92.3% of diff hit (target 90.4%)
Details
codecov/project Absolute coverage decreased by -0.7% but relative coverage increased by +1.9% compared to 347c884
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@cmodijk

This comment has been minimized.

Member

cmodijk commented Jul 5, 2017

@kbond Tnx, i just merged this.

@kbond

This comment has been minimized.

Contributor

kbond commented Jul 5, 2017

Awesome! Can we get a new release?

@gubler

This comment has been minimized.

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

This comment has been minimized.

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 from [RFC] Add auto-register compiler pass to Add auto-register compiler pass Aug 28, 2017

@kbond

This comment has been minimized.

Contributor

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]

@kbond kbond deleted the kbond:auto-register branch Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment