-
Notifications
You must be signed in to change notification settings - Fork 17
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
How does Relay fit in with Radar and Spark? #2
Comments
👍 |
Well, other than that it be invokable/callable. |
Do we not like the idea of using a |
I think the Route gets returned too late for that. It would have to be passed to the action handler in some way. And then the action handler would have to actually create the objects specified in the route. Thus, my initial inclination toward an ActionDescriptor (or whatever) that has an injector/factory/builder, and gets information fed into it from the route about the input/domain/responder specifications. That ActionDescriptor then gets shoved into the Request as a uniquely "well-known" attribute for a later Action handler to extract and use to create the necessary objects. (Hope that made sense.) |
@pmjones I guess I only suggest that since it's how it's happening now. The application resolves the route before the action handler is called. What if we injected the |
/me ponders That would also mean injecting the ActionDescriptor (or whatever) into the Routing/Router middle ware piece, as a shared object between the Routing/Router and the Action middleware. Starting to sound like a service of sorts there. Do we care about changing the state of shared objects in that way? |
We wouldn't have to share the route necessarily. The flow I think could work is as follows.
I'm sure I'm missing something in there, but from my thought process and the way Spark works, that flow seems to solve all the issues and allows ActionHandlers to just be middleware that follows the PipelinePHP interface. |
Thanks for that workflow! I think one disconnect with my way of thinking is that the Router IMO is "just another middleware." The Router middleware has no special relationship with the application as a whole, it just happens to get executed in the queue before the Action middleware. For example:
That's what I'm getting at when I say that the RoutingHandler needs to be able to communicate with the ActionHandler in some way; it's "just another middleware" and so is not special. Hope that makes sense. (p.s. I am beginning to really dislike the word "middleware." I so wish whoever had come up with that had just stuck with "decorator." Well, whatever.) |
Now I'm seeing where you're coming from. How do you suggest resolving the Domain, Input, Responder classes in this flow? If the RouteHandler determines that information, would you have to put the Resolver/Injector inside that class? My initial reaction to this is to think that the router should be outside the stack, but I suppose I could see advantages to this. |
Sweet.
Yeah, this is the tricky part. By way of example, this is what happens in Radar:
While I'm not enthusiastic about using a Request attribute to convey the routing information to the ActionHandler, it seemed the most obvious and straightforward approach. |
Sounds like a good approach to me. I initially was inclined to keep the Injector out of the Handlers, but if you think that doesn't conflict with a service oriented architecture, then who am I to argue? 😄 I do like the single attribute instead of multiple. Now what do we call it? |
I'm still opposed to having the Injector exist anywhere except the Application. Auryn is strictly not a service locator and we shouldn't attempt to use it that way, nor should we use any service locator. |
From my perspective, you have to start from the position that service location doesn't exist and that there is no way to "store" objects or "fetch" them later. Radar doesn't do this at all and it's one of my biggest issues with it. |
Let's not overstate the case. ;-) For the record, I'm not super-happy about dropping the Aura.Di InjectorFactory into the ActionHandler. The one thing that soothes me is that it's "only" a factory, and (@shadowhand take note) not a service locator or container, so I can avert my eyes a bit when I see it. I'm with @shadowhand on avoiding putting containers anywhere "inside" the system.
Whatever interface package name we come up with, I'd suggest that. The Aura naming convention where global keys are involved is "package/vendor:varname", but that may not be suitable here. |
Agreed -- not sure why you think Radar does this, or where, but I may be too close to it to see it. |
@shadowhand Is it possible to have both? In order to resolve the Domain/Input/Responder in the RouteHandler / ActionHandler, Auryn would need to be injected, just like we are doing in the Router currently.
@shadowhand Are you suggesting that this method is using the ServerRequest object to store and fetch objects? |
Unless I misunderstand service location, this is clearly a service location. |
(/me thinks) @shadowhand So that I'm clear, your position is that the passing of an object via the Request attributes counts as service location? |
I think there are two ways to do it. Injector goes into the Handler, or the Router comes out of the stack. I'm not sure we can have both. Pipeline would easily support both methods as it is right now. |
@pmjones on second read, I think I had a knee-jerk reaction... I'll answer your question first, and then explain myself better:
I didn't initially realize it was with the Request, which is where the "knee-jerk" comes in. That said, I am still opposed to it, because the route is an implementation detail, not a property of HTTP. If anything, I would suggest that the ActionHandlerInterface receive the RouteInterface as a collaborator: class ActionHandler implements ActionHandlerInterface
{
public function __construct(RouteInterface $route)
{
$this->route = $route;
}
public function __invoke($request, $response, $next)
{
$domain = $di_create_instance($this->route->getDomain());
...
} The implication here is that the middleware stack is created in a lazy fashion, so that the route can be injected. Thoughts? |
Granted. Having said that, the PSR-7 ServerRequest $attributes property is for non-HTTP elements associated with the request. The original intent IIRC was to pass things like path-info values discovered by routing. Passing other request-related values seems a reasonable use.
Which means it needs to be constructed with the discovered Route, which means the Route needs to be discovered before the ActionHandler is constructed, which means the RouterHandler needs to construct the ActionHandler perhaps via a factory ... ? |
Or that middleware dispatcher would construct the middleware as it works through the stack? |
@pmjones though I do see your point about tl;dr: I apologize for the diversion, carry on. 8-) |
(/me nods) It does (or "can do") that now via the $resolver, but it does so outside of any particular middleware, which leaves two problems: 1. How to get the Route out of the RouteHandler to the Resolver, and 2. When-and-how to specify arguments to pass to the $resolver mechanism. Compare that with passing a Route object as a Request attribute for the ActionHandler to retrieve later. |
Not at all. ... where does that leave us, BTW? ;-) |
Going back to the topic before I derailed us... the discussion was about where to resolve the Domain, Input, and Responder (DIR). I see nothing wrong with allowing the ActionHandler to use a factory to create instances of the specs attached to the route. So long as the injector lives solely outside of the DIR, I will consider the architecture to be sound. |
So do we want this project to contain an |
Hi again, I've been working through the Spark interfaces to find the commonalities with Radar, and then extract them. Here's what I have so far (and I think this reiterates some of what I've said previously). None of this is in stone, of course, and I offer it here for feedback. ActionI think @shadowhand's intuition toward a RouteInterface to retain the action specifications points in the right direction. Having worked it over some, I have concluded that what we want is not a RouteInterface per se, but an Action value object that carries the input/domain/responder specifications as extracted from elsewhere (likely a route). The Action VO is created by an earlier middleware using an ActionFactory, then attached to a request attribute for a later middleware to use. Likewise, I think @shadowhand's intuition toward an ActionInterface and an ActionTrait also points in the right direction. Again, having worked it over some, it seems that an ActionHandler object proper, that is not itself a middleware piece, is the right way to go here. We can standardize the action work, without necessarily specifying how the middleware piece itself must be built. The middleware piece in charge of performing the action can then either extend, or compose, the ActionHandler. (UPDATE: the middleware piece would likely be project-specific, not something in this "standard package.") You can see those four elements (Action, ActionFactory, ActionHandler, Middleware) here: https://gist.github.com/pmjones/1eca32131759ef600051 (UPDATE: The logic for these is derived from Radar.) Incidentally, the ActionHander needs to create or resolve objects from the input/domain/responder specifications. That occurs via the $resolver, and is typehinted as a callable to allow wide flexibility. Input, Domain, ResponderAfter that, it doesn't seem to me that we need (or even can fairly describe) interfaces for input, domain, and responder. For example, when it comes to the domain, the actual domain work is outside the scope of ADR. The domain work hould be completely independent. The full extent of what we need is a plain old PHP callable specification that refers to some domain class and method, or a domain invokable object. It can be invoked used Tied to that is the input specification. As long as it takes a ServerRequest and returns an array of params for
... but I don't see that as being that big a deal. As far as a standard InputTrait, the first thing that jumped out at me from the Spark implementation is that it's doing validation. IMO that belongs in the domain; let the domain validate the inputs, not the user interface. Aside from that, the merging of values from various request locations seems fine to me as a default; Radar does something similar. Providing a default Input object proper, instead of a trait, seems the more reasonable approach to me. Finally, the responder. We know it needs a request and a response, but for the payload (if any) I don't think we can typehint to anything useful. The payload is going to come from the domain. The domain should not be dependent on anything from the user interface layer, which is what we're working with here. That alone makes it hard to typehint. Likewise, we may not even need a payload for some responders, as some routing may point directly to a responder without a domain payload. Even if we did this ...
... it would prevent implementors from typehinting to the payload of their choice. (Same problem if we leave off $payload entirely; implementors adding a $payload param would get strictness errors.) In all, I think saying "the ActionHandler has these expectations of input and responder" would be sufficient. For example:
That would allow implementors to typehint, or leave out entirely, the $payload as they wish. Finally, to reiterate, the domain is specified with a callable, and is invoked with /me wipes brow Over to you! |
As a followup, both Radar and Spark should be able to use the Action* classes as-is via composer. The classes should probably be in a separate package so they don't "identify" with either Radar or Spark; hell, they probably ought to be separate from Relay, since the ActionHandler doesn't require a particular middleware signature. |
I say it ought to be in a separate project as a standalone library. That keeps it from being "a particular framework's handler." (It's the same idea as having Relay be a standalone library.)
I confess I don't see how the Input/Domain/Responder objects are being instantiated there; does the Route object deal with that? |
On further consideration, the $resolver could default to null as an optional injection, and the resolve() method could just return the spec directly if no $resolver has been injected. I have updated the gist to allow for that: https://gist.github.com/pmjones/1eca32131759ef600051 |
@pmjones I'm starting to add in Relay to Spark, and I'm running into some issues. It's probably just my brain thinking about it one way, but what do you see the order being of these middlewares? I'm thinking it will be ExceptionHandler --> {all other middleware} --> RouteHandler --> ActionHandler --> ResponseSender. Does that make sense? |
ResponseSender, ExceptionHandler, Router, Action. Cf. https://github.com/relayphp/Relay.Middleware for the reasoning. To wit:
|
Well silly me. I should have read through that whole document first. 😃 |
@shadowhand @dolfelt et al: Any further thoughts on this, especially the extraction of the Action-specific elements? |
@pmjones I've done some work on it and got it to what I think it a functional state. https://github.com/sparkphp/Spark/pull/17 |
I like it. I think your work proves the concept of Relay and its middleware as a reusable package. I see that you've got your own ExceptionHandler, but it totally makes sense given that you're trying to present different/extended information. So as far as Relay itself is concerned I think https://github.com/sparkphp/Spark/pull/17 is a success. I opine that you could go a little further, if you had the inclination, by doing these things in reference to the action-related gist above (https://gist.github.com/pmjones/1eca32131759ef600051):
class AurynResolver
{
public function __construct(Injector $injector)
{
$this->injector = $injector;
}
public function __invoke($spec)
{
return $this->injector->make($spec);
}
} So you would pass the AurynResolver instance to the Spark\ActionHandler constructor. The upshot is that you can remove even more code from Spark, and depend on the Action stuff from the gist (which itself would need to become yet another separate package, probably not something in Relay). FWIW, I have a Radar branch using the Action stuff from the gist, and it seems to be working fine. Again, the action-related stuff is over-and-above the Relay-related work, and may not be related to your goals. Hope all that made sense. |
I have converted the action-related gist above to a separate repo, tentatively Elemental: https://github.com/elementalphp/Elemental.Elemental (Apparently the vendor name "elemental" is already taken on Packagist, so I'll have to change the name; the purpose was to fully extract the functionality and test it separately.) |
Renamed to Arbiter, hopefully to indicate an arbitrator/mediator: https://github.com/arbiterphp/Arbiter.Arbiter |
Oh and @dolfelt @shadowhand I have successfully integrated Arbiter into Radar per a commit this morning: radarphp/Radar.Adr@cfe4ead |
That leaves us with Input and Responder elements. Regarding input, I asserted above:
To get to that, It might be fair to merge the default Radar input object and the Spark trait (without making allowance for validation) into an Arbiter input object of some sort. Responders are a tougher nut to crack. For the reasons I note above, I think the best we could do would be to come up with a responder package (probably yet another separate package) that type hints against Aura\Payload_Interface\PayloadInterface (or some other interface-only package) for its payloads. That avoids having the payload type embedded in the responder package, and thus tying domain-related elements to a UI-relate package. Then we could collect the Radar responder and accepts-aware interface, and the two Spark responders, into that separate package, on which Radar and Spark could then depend. But I'm really not sure about that one; responders are a heavy-lifting element in ADR, and they do a ton of payload-specific work, which may make them harder to generalize. At that point I think we'll have extracted everything possible, leaving only the project-specific elements that go around those things: config/setup, routing, etc. @dolfelt @shadowhand etc, thoughts? |
@pmjones Finally got time to implement the new Relay stuff into my middleware branch (https://github.com/sparkphp/Spark/pull/17). I'm still debating the need for all these packages. Arbiter seems to make sense, but trying to create all these separate packages for defaults for the Domain, Input, Responder stuff seems overly complicated and unnecessary. Spark will probably have sane defaults for Input and Responder, but I would expect most people to just write their own. Also, the less requirements we have for this kind of stuff the better. I'd also like to find a way to not require a certain payload to come back. Maybe one for the default responder, but again, extending it to use whatever payload you want should be easy. For the responder portion, I'm leaning towards no. I don't want to limit the payload that needs to come back. It should be up to the application to decide that. For the Input portion, I'm on the fence. I don't think it really matters, but could be good to include something simple in the Arbiter package. P.S. I might give a crack at adding the Arbiter package later today. Looks simple enough. |
Nice.
As am I, mostly because I can't see any way clear to the goal of not being dependent on a particular payload type.
I'm leaning toward "no" here as well. The input to the Domain is going to be very Domain-dependent (same way output from the Domain is Domain-dependent) and although a basic no-frills Input object is possible I think it would serve little purpose other than as a naive example for newcomers. In all, it seems that Input and Responder should be specific to the combination of framework and domain, and cannot be reliably extracted without specifying a third dependency (esp. the payload).
Right on. Let me know how it goes. With Radar it was dead-stupid-easy. With that, unless there are further comments from @shadowhand et al, I think we have reached the end of this issue. The middleware queue and processing, and the action processing, have been extracted to independent packages, and input/domain/responder portions remain Domain-specific concerns to be addressed on a case-by-case basis. Further thoughts? |
I think this issue has served its purpose. I am closing it, pending later desires to reopen it. |
Taking our conversation from email to Github so that others can have some input...
I see Pipeline as the nexus between the two, and (one hopes) a wide range of other middleware systems. The idea would be, again one hopes, for a collection of middleware packages to spring up that follow the same signature and expectations. Then Radar, and Spark, and whatever-else-comes can say they are Pipeline compliant.
/me nods
As far as core ADR stuff, Pipeline might provide a good non-project-specific nexus, or maybe not. There's a case for publishing separate package that is not attached to any particular project.
Regardless, the fact that the middleware dispatching is separated out means each project gets to its own .env setup (before Pipeline), container setup (which creates Pipeline and other objects), and middleware objects (which Pipeline executes). Those surrounding pieces are where the fun and preferences live.
You've already talked about this, but I'll reiterate. I think the remaining pieces for an ADR set of interfaces are:
__invoke(ServerRequest $request)
would be just fine, which you have in place. The return would have to be an array of params suitable forcall_user_func_array()
.call_user_func_array($domain, $input($request))
seems to cover every possible case.Actual implementations of these, might go well in their own projects. That would mean an interface-only package, and then separate projects that implement them according to the desires of the implementors.
(/me wipes brow)
None of that is set in stone, obviously, but it seems like a good start combined with your spark/adr work.
Over to you!
The text was updated successfully, but these errors were encountered: