add roles when using constructor injection #29

Open
wants to merge 2 commits into
from

Projects

None yet

2 participants

@xenoterracide
Contributor

Bread::Board::ConstructorInjection->new(
class => 'Foo',
roles => ['Bar'],
)

the benefit of this is to allow us to modify classes that we may not have
control over modifying the source of at runtime.

Signed-off-by: Caleb Cushing xenoterracide@gmail.com

Contributor

@doy @stevan

note: state of this is basic functionality, probably needs many more tests, and of course docs. Should this be able to work on any of the other service types? any thoughts on additional error handling? or anything else?

@xenoterracide xenoterracide add roles when using constructor injection
Bread::Board::ConstructorInjection->new(
  class => 'Foo',
  roles => ['Bar'],
)

the benefit of this is to allow us to modify classes, that we may not have
control over modifying the source of, at runtime.

Signed-off-by: Caleb Cushing <xenoterracide@gmail.com>
6115597
Collaborator
doy commented Nov 1, 2013

This is almost certainly going to break typemap entries, for one thing, and I'm not really sure how you'd sanely fix that. Why does Bread::Board need to do this, rather than you doing it yourself? It seems like it'd just be one or two lines otherwise.

Contributor

yeah, I was thinking that I definitely need to ensure working typemap, though the test suite generally functions, not sure what happens if I start adding roles.

Aspect Oriented Programming IMO is one of the great things about some of the non perl dependency injection systems, the ability to modify classes that you can't modify directly.

I suppose you're arguing that you could do this with block injection, and yes you could, of course you can do any of the things with block injection, so why bother with anything else. Sugar, it's nicer to write it this way, block injection gets messy and annoying fast. Could end up being a few lines for every class, also considering parameters support in block injection. Oh and also remember not everyone is Stevan, Doy, Ether, MST, Me etc, basically they haven't the knowledge to write the 2-3 lines, they don't know anonymous subclasses exist. blah blah, I want to write less. because I think writing (presuming all problems can get sorted and you'll be willing to allow this in BBD as well)

has $_ => (
    class => $_,
    roles => [qw( FromJSON ToJSON )],
    infer  => 1,
    allow_extra_parameters => 1,
) foreach qw( Product Payment Sale );

is better than

has $_ -> (
    class => $_, 
    infer  => 1, #does this work here? I have no idea, what do the parameters look like?
    allow_extra_parameters => 1,
    block => sub {
        my $s = shift;

         my $class = with_traits( $s->class, qw(  FromJSON  ToJson ) ) ;
         $class->meta->make_immutable;
         $class->new( $s->params ); # does this work? I can't remember I think I probably need to write more things here to get the params from infer mapped properly assuming they happen on ->params at all? what about the others? oh and if I have manual dependencies? too much stuff to look up... or TIAS

    }, 

This all kinda comes down to, how much more messy does bread board get when I start using it to manage all the construction as mentioned here: http://www.xenoterracide.com/2013/10/providing-with-providers-and-breadboard.html

I'm also considering writing some additional things that make this make more sense. What if in Config::General you could have

<Sale>
 roles ProvisionCpanelAfterPayment
</Sale>

Now we've turned it into a plugin API that's a helluva lot easier to make with this API than if I have to yank it all through block injection.

Contributor

Oh, I'm also hoping to add type inferance based on roles, (if 1 and only one class in the container could satisfy a does, allow inference )

Collaborator
doy commented Nov 1, 2013

I was actually thinking more along the line of creating an actual MyFoo.pm with package MyFoo; use Moose; extends 'Foo'; with 'Bar' and then using MyFoo in your service definitions. I think autogeneration of new classes like you are proposing isn't a very good idea, and isn't something that should be Bread::Board's responsibility in any case. The whole concept of type-based service lookup only really makes sense when you're only dealing with a set of concrete classes.

Also, the only reasons you would want to build classes at runtime like that are either if you're going to be building multiple classes based on different sets of roles, or if you're going to be building an entirely different class on each run based on some kind of configuration. The first of these means that inference can't work (you have multiple possibilities for the type), and the second is just a terrible design (it's quite hard to figure out what a given object in your system even does). If neither of these is the case, you aren't gaining anything by generating the classes at runtime rather than writing real classes.

Also, roles as a plugin API is something that we thought might be a good idea a couple years ago, but looking back was really not a good idea at all. If you want a plugin API, you should really write a real plugin API, not just expose everything and then mush it all together into one big class. Look at Dist::Zilla, or Reply, or p5-mop for better examples of what plugin APIs should look like.

Contributor

Interestingly, java can do type based inference without the concrete classes, and it's more deep magic than this has to be. Which IMO is if you only have one class that has the interface that satisfies the type. Now why would someone do that... I would do that if I perhaps had different targets for my models...

configuration based injection... hmm... reminds me of something... who's looking to mush things together in one big class? no I"m looking to make every class in the system easy to extend without subclassing the shit out of everything just to do something. Which is the opposite of dependency inversion, asking people to do that is tight coupling. Otherwise I could just end up doing FooThatDoesBar and FooThatDoesBaz.

None of the things you've mentioned use Bread::Board, or AFAIK a IoC Container of any sort... so how are they proof of what a DI system should be able to do? I'm not saying it's a fantastic complete replacement for just a plugin API, and hell without providers it's probably not even that useful (though to be fair I was having trouble finding Bread::Board useful without them). Nor would I suggest this be a replacement for real classes where they make sense. But In some cases making the container more flexible is the right thing to do.

Maybe if we can make it useful enough, more people will use it. As it stands, esp with BBD, it's hard to distinguish what it's even doing for people. To them lazy builders seem like they'd satisfy. They don't get it, because DI is for large systems not small ones. But their large systems already exist and are full of varying tight couplings, and service locators. I happen to find it ironic that you suggest not building a system that relies on role upon role upon role application, and yet you suggested I do just that with BBD once, as a preference to hierarchical containers, a god container or numerous subclasses.

I'm going to disagree, but I doubt I'll sell you, so unless stevan decides he'd be willing to merge these changes (given more work) I'll experiment with them on my own.

Collaborator
doy commented Nov 1, 2013

I'm just having trouble seeing how each class in the system isn't already easy to extend. With Bread::Board::Declare at least, you can just pass in a new instance of any subclass of the class the service requires, and things just work. You can even subclass the container class to set that new subclass of the service class as the default instance. Getting this to work with plain Bread::Board would be a bit more work, but still quite doable. The only reason to move in this direction is to move in the direction of configuration-based injection, and I don't really see that as being a good idea at all - you either end up with your container definition split across several files, or you end up with things like perl code inside of XML files, and both of those are pretty awful to work with. In what case would you need to subclass everything in order to make things work?

As for providers, that is actually something I've been thinking about adding to Bread::Board for a while now, just never got around to it. I agree that they would help quite a bit. If you're interested in working on that, I'd be interested in seeing those results. Not having providers is I think one of the main things that confuses new users of Bread::Board, because they want to try to use their container as one big mega-provider, which just leads to all kinds of sadness.

Also, I never said that role application was bad. I definitely agree that building classes out of roles is a good thing. What I'm arguing against here is configuration-based runtime role application. That's the aspect that causes a lot of architectural problems.

Anyway, my basic issue is that I think that mixing class creation with container definitions is mixing behaviors too much to really be a good idea. Those should be separate concepts, handled by separate code. Especially in big systems, having a concrete set of classes is pretty important when trying to track down how your code is actually functioning, and once you have a concrete set of classes, the actual dependency injection logic becomes much more trivial. Trying to mix them together adds quite a lot of complexity to the situation.

Contributor

Java has a problem, they like to be an all A or all B, etc ... so first they were like XML and configure everything is the right answer!!!, no, and now it seems to be all Annotations for everything! (at least this is what Swing, Hibernate, Guice, etc seem to be saying) Of course we in perl know better, TIMTOWTDI, at the same time some ways are better than others.

What I'm trying to get to is a well balanced mix, some configuration based, some BBD, some plain Bread::Board. I've ended up with a container at each vertical layer. Infrastructure, Model, Controller, and a very thin Global/Glue. Largely this is due to me wanting to do things like swap out my persistence layer with a completely different tech (esp for testing), or drop the controller layer entirely (for testing). I feel the same for roles, maybe I'd have a set for debugging, and another for testing, that were only added by the injection system.

I unfortunately have no great example at this exact second, because I'm building a tool that I no longer need due to circumstance, but I wanted previously. I want more Aspect Oriented programming, the ability to modify classes without subclassing (well ... writing a subclass) , obviously Moose has this capability, it's not not super accessible because you either have to do the fancy dance somewhere or write subclasses, neither are always ideal. Spring does it through proxy classes and modifying byte code, my understanding is perl is too slow for that to work. Obviously you should never need to subclass anything in order to make things work, but I have seen systems where you'd have to, seemingly, subclass everything to change one thing, simply because you cannot inject where you need to, the coupling was too tight.

What's wrong with configuration based runtime role application? seems like that's what Dist::Zilla does (though I admit to not understanding it's internals). Of course as with any solution you could let this get out of hand, subclass through multiple inheritance, use nothing but roles to build giant classes (or even small ones), try to make a system that is all configuration (dzil is pretty close).

basically my point is that no one way seems to be a perfect solution. I digress I could be completely wrong, but at the same time it could be a handy tool, or maybe just abused.

I'm curious as to what you think of my providers solution so far (written in that blog post) and what'd you'd think an official solution should look like. I would think provider => 1 would be the api, but how to infer (if that's even possible) seems difficult.

@xenoterracide xenoterracide add failing test
Signed-off-by: Caleb Cushing <xenoterracide@gmail.com>
661d0ee
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment