[Catalyst] catalyst++

[Matt S Trout]

Garrett Goebel wrote:
> On Sep 30, 2006, at 8:19 AM, Matt S Trout wrote:
>> 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 ;)

That I'm still prodding you for examples indicates that you might be 
misguided, but you're not an idiot.

I rewrote the dispatch infrastructure for 5.50 with this sort of stuff in 
mind, and this discussion is making me re-examine the assumptions with which I 
did so. So far they seem to hold :)

> 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.

Auto-selecting appropriate action classes based on attributes in the 
controller is very do-able, which I think would make this at least less 
annoying to you.

> 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.

The dispatch types are fairly explicitly dispatch-on-path, so that would be 
expected.

Look at it from a RESTful POV of "DispatchType identifies the resource 
handling a URI, then Action->match identifies which specific action against 
this resource to dispatch"

> 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.

That's basically what it does, except that the "standard" dispatch types are 
pre-loaded so the precedence order is correct wrt Index and Default (which 
aren't identified by an attribute and as such nee to be loaded). Chained, for 
e.g., is only loaded on-demand.

> 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.

See above wrt the resource vs. action note above for why I believe it *does* 
belong there, or at the very least doesn't actively *not* belong there :)

I'm really not sure what you mean by "scale better" since what you've proposed 
would get substantially less efficient as more dispatch types were added - 
currently resolving for e.g. a Path action is a simple hash lookup; checking 
other dispatch types would be messy. Also, it would break the current 
encapsulation, which is important - with the current system :Whatever in one 
controller can mean something completely different to in another controller 
depending on how the controller chooses to handle it, what you're proposing 
would make that harder if not impossible.

So, in summary: I see your argument, but I think it's based on a mistaken 
interpretation of the way the system's conceptualised and I'm not convinced 
the approach you're suggesting would be any better. That doesn't mean you 
shouldn't carry on trying to convince me though :)

-- 
      Matt S Trout       Offering custom development, consultancy and support
   Technical Director    contracts for Catalyst, DBIx::Class and BAST. Contact
Shadowcat Systems Ltd.  mst (at) shadowcatsystems.co.uk for more information

+ Help us build a better perl ORM: http://dbix-class.shadowcatsystems.co.uk/ +