[Catalyst] catalyst++

[Garrett Goebel]

On Sep 30, 2006, at 8:19 AM, Matt S Trout wrote:

> Garrett Goebel wrote:
>> On Sep 29, 2006, at 11:22 AM, John Napiorkowski wrote:
>>
>>> I'm not sure what you mean by 'multi-attribute
>>> dispatch routing'.  I have someone here working on
>>> integrating Class::Workflow as a controller so you can
>>> dispatch based on workflow states and not just on
>>> actions.  Is that something that interests you?
>>
>> I'm talking about the difference between single and multiple
>> dispatch. Catalyst's action attributes whether they're local, path,
>> regex, chained, etc. all are specific to the request uri. The
>> dispatcher tries each dispatch type until if finds one that matches
>> the path.
>>
>> I've modified the dispatcher so I can dispatch an action if it
>> matches request method and uri path. Dispatching based on workflow
>> states would be a nice addition. I've looked at Class::Workflow, but
>> never got past just looking at it. Yes, it does interest me.
>>
>> However, what I've done is a hack to meet my needs. It'd be nice if
>> there were a generic catalyst dispatcher which handled multiple
>> dispatch.
>
> There already is. It's called Catalyst::Dispatcher.
>
> We had a long thread about this where you kept saying "but it can't  
> do X" and
> I followed up saying "yes it can, you do it like this".

Yes, you answered the questions I raised. However I believe I'm  
raising a new one.

> I can't think of any point you raised that wasn't addressed as  
> being viable
> against the standard dispatcher; if there are still things you  
> believe can't
> be achieved please post *specific* examples rather than just  
> complaining in
> general that it won't do what you want :)

Ignorance can always be cured. Let's hope I'm not being an idiot ;)

Per your request, let's see if I can be more specific...

package Catalyst::Dispatcher;
...
sub prepare_action {
...
         foreach my $type ( @{ $self->dispatch_types } ) {
             last DESCEND if $type->match( $c, $path );
         }
...
}

and

package MyApp::Controller::Foo;
...
sub bar : Method(GET) Path('') Args(2) { ... }

This is a simplification, but for a given request, the catalyst  
dispatcher attempts to match the request's uri path by iterating  
through the dispatch types Index, Path, Regex, and Default invoking  
the match class method, which in turn invokes the $action->match  
method for each action instance for that $path until we find the  
first match. IMHO this is effectively single-dispatch, in that  
actions are dispatched based on the first invocation of dispatch_type  
to match.

So if Catalyst::Dispatcher supports multiple dispatch, how do you  
make an action dispatch based on more than one attribute of the  
subroutine definition for a given action? I.e. path _and_ request  
method? The recommendations I've gotten, if I've understood them  
correctly, have not been to modify the behavior of  
Catalyst::Dispatcher or Catalyst::DispatchType::* as one might expect  
to affect dispatching behavior, but to rather to subclass  
Catalyst::Action and the Catalyst::Controller. And while that might  
work for my hack to dispatch based on request method, path, and args,  
it won't scale well. I'd rather make one dispatch type class for each  
dispatch type, than an Action class for each possible combination of  
all the dispatch types.

Next, take for example, dispatching an action declared with  
attributes for both Regex _and_ a custom dispatch types. The side  
effects of calling the dispatch_type->match method will only occur  
for the first dispatch type which matches. If my action matches on  
the custom dispatch type first, $c->request->captures won't get set  
by Catalyst::DispatchType::Regex. I.e., you don't get the side  
effects of invoking dispatch_type->method for all of the dispatch  
types indicated by the action's attributes.

Speaking without the experience with and intimate knowledge of the  
code which you have, I would expect that the attribute handling code  
in Catalyst::Base would work in conjunction with Catalyst::Dispatcher  
to determine dispatch types in use. And that  
Catalyst::Dispatcher::prepare_action would iterate through this list  
of used dispatch types, not the complete list of all dispatch types  
supported.

I would also expect that Catalyst::Dispatcher::prepare_action would  
instead look something like:

package Catalyst::Dispatcher;
...
sub prepare_action {
...
         OUTER: foreach my $type ( @{ $self->dispatch_types } ) {
             push @done, $type;
             if ($type->match( $c, $path )) {
                 @todo = diff($c->action->dispatch_types, \@done);
                 INNER: foreach my $other_type (@todo) {
                     next OUTER unless $other_type->match($c, $path);
                 }
                 last DESCEND;
             }
         }
...
}


Not only do I believe this would scale better, but this would allow  
me to put the dispatch type match logic in my Catalyst::DispatchType  
classes and out of my Action classes where IMHO it doesn't belong.  
That said, I haven't wrapped my head around the Chained dispatch type  
sufficiently well enough to see if Args processing in  
Catalyst::Action::match could be moved to a  
Catalyst::DispatchType::Args class.

cheers,

Garrett