Merge 'namespace_handling_refactor' into 'trunk'
Rafael Kitover [Thu, 25 Jun 2009 09:49:35 +0000 (09:49 +0000)]
r5367@hlagh (orig r10459):  t0m | 2009-06-06 05:56:50 -0700
Branch for mo namespace handling refactor

r5368@hlagh (orig r10460):  mo | 2009-06-06 06:33:53 -0700
refactor of namespace handling
r5369@hlagh (orig r10461):  mo | 2009-06-06 06:34:42 -0700
TODO fix
r5371@hlagh (orig r10466):  mo | 2009-06-07 04:14:23 -0700
controller actions without attributes which are defined via config
r5372@hlagh (orig r10467):  mo | 2009-06-07 04:49:58 -0700
removed commented code
r5373@hlagh (orig r10468):  mo | 2009-06-07 05:28:53 -0700
Added support for ~ prefix to plugins and action classes
r5374@hlagh (orig r10469):  mo | 2009-06-07 05:36:55 -0700
Changes patches
r5686@hlagh (orig r10655):  caelum | 2009-06-24 19:22:27 -0700
cleanup TODO

12 files changed:
lib/Catalyst/Action.pm
lib/Catalyst/DispatchType.pm
lib/Catalyst/DispatchType/Default.pm
lib/Catalyst/DispatchType/Index.pm
lib/Catalyst/DispatchType/Path.pm
lib/Catalyst/Dispatcher.pm
t/aggregate/live_component_controller_action_index_or_default.t [new file with mode: 0644]
t/lib/TestAppIndexDefault.pm [new file with mode: 0644]
t/lib/TestAppIndexDefault/Controller/Default.pm [new file with mode: 0644]
t/lib/TestAppIndexDefault/Controller/IndexChained.pm [new file with mode: 0644]
t/lib/TestAppIndexDefault/Controller/IndexPrivate.pm [new file with mode: 0644]
t/lib/TestAppIndexDefault/Controller/Root.pm [new file with mode: 0644]

index e2e78b6..97b9235 100644 (file)
@@ -18,8 +18,9 @@ L<Catalyst::Controller> subclasses.
 =cut
 
 use Moose;
-
+use Scalar::Util 'looks_like_number';
 with 'MooseX::Emulate::Class::Accessor::Fast';
+use namespace::clean -except => 'meta';
 
 has class => (is => 'rw');
 has namespace => (is => 'rw');
@@ -28,8 +29,6 @@ has attributes => (is => 'rw');
 has name => (is => 'rw');
 has code => (is => 'rw');
 
-no Moose;
-
 use overload (
 
     # Stringify to reverse for debug output etc.
@@ -38,6 +37,10 @@ use overload (
     # Codulate to execute to invoke the encapsulated action coderef
     '&{}' => sub { my $self = shift; sub { $self->execute(@_); }; },
 
+    # Which action takes precedence
+    'cmp' => 'compare',
+    '<=>' => 'compare',
+
     # Make general $stuff still work
     fallback => 1,
 
@@ -70,6 +73,22 @@ sub match {
     return scalar( @{ $c->req->args } ) == $args;
 }
 
+sub sort_order {
+    my $self = shift;
+
+    my ($args) = @{ $self->attributes->{Args} || [] };
+
+    return $args if looks_like_number($args);
+
+    return ~0;
+}
+
+sub compare {
+    my ($a1, $a2) = @_;
+
+    return $a1->sort_order <=> $a2->sort_order;
+}
+
 __PACKAGE__->meta->make_immutable;
 
 1;
@@ -105,6 +124,14 @@ context and arguments
 Check Args attribute, and makes sure number of args matches the setting.
 Always returns true if Args is omitted.
 
+=head2 sort_order
+
+Returns the value of the C<Args> attribute, or C<~0> if it has no value.
+
+=head2 compare
+
+Returns C<< $a->sort_order <=> $b->sort_order >> .
+
 =head2 namespace
 
 Returns the private namespace this action lives in.
index 75de1c5..0cde651 100644 (file)
@@ -73,6 +73,8 @@ about expand_action.
 
 sub expand_action { }
 
+sub _is_low_precedence { 0 }
+
 =head1 AUTHORS
 
 Catalyst Contributors, see Catalyst.pm
index d90ff57..6586351 100644 (file)
@@ -41,7 +41,7 @@ other possibilities have been exhausted.
 
 sub match {
     my ( $self, $c, $path ) = @_;
-    return if $path =~ m!/!;    # Not at root yet, wait for it ...
+    return if $path ne '';    # Not at root yet, wait for it ...
     my $result = ( $c->get_actions( 'default', $c->req->path ) )[-1];
 
     # Find default on namespace or super
@@ -58,6 +58,8 @@ sub match {
     return 0;
 }
 
+sub _is_low_precedence { 1 }
+
 =head1 AUTHORS
 
 Catalyst Contributors, see Catalyst.pm
index 86e45ee..c0be9e6 100644 (file)
@@ -2,7 +2,7 @@ package Catalyst::DispatchType::Index;
 
 use Moose;
 extends 'Catalyst::DispatchType';
-no Moose;
+use namespace::clean -except => 'meta';
 
 =head1 NAME
 
@@ -25,6 +25,12 @@ dispatch types, see:
 
 =back
 
+=cut
+
+has _actions => (
+    is => 'rw', isa => 'HashRef', default => sub { +{} }
+);
+
 =head1 METHODS
 
 =head2 $self->match( $c, $path )
@@ -40,6 +46,8 @@ sub match {
     return if @{ $c->req->args };
     my $result = $c->get_action( 'index', $path );
 
+    return 0 unless $result && exists $self->_actions->{ $result->reverse };
+
     if ($result && $result->match($c)) {
         $c->action($result);
         $c->namespace( $result->namespace );
@@ -50,6 +58,20 @@ sub match {
     return 0;
 }
 
+=head2 $self->register( $c, $action )
+
+Register an action with this DispatchType.
+
+=cut
+
+sub register {
+    my ( $self, $c, $action ) = @_;
+
+    $self->_actions->{ $action->reverse } = $action;
+
+    return 1;
+}
+
 =head2 $self->uri_for_action( $action, $captures )
 
 get a URI part for an action; always returns undef is $captures is set
@@ -67,6 +89,8 @@ sub uri_for_action {
     return "/".$action->namespace;
 }
 
+sub _is_low_precedence { 1 }
+
 =head1 AUTHORS
 
 Catalyst Contributors, see Catalyst.pm
index 6be3893..bff5f21 100644 (file)
@@ -6,7 +6,6 @@ extends 'Catalyst::DispatchType';
 use Text::SimpleTable;
 use Catalyst::Utils;
 use URI;
-use Scalar::Util ();
 
 has _paths => (
                is => 'rw',
@@ -62,16 +61,6 @@ sub list {
       if ( keys %{ $self->_paths } );
 }
 
-sub _action_args_sort_order {
-    my ( $self, $action ) = @_;
-
-    my ($args) = @{ $action->attributes->{Args} || [] };
-
-    return $args if Scalar::Util::looks_like_number($args);
-
-    return ~0;
-}
-
 =head2 $self->match( $c, $path )
 
 For each action registered to this exact path, offers the action a chance to
@@ -85,10 +74,7 @@ sub match {
 
     $path = '/' if !defined $path || !length $path;
 
-    # sort from least args to most
-    my @actions = sort { $self->_action_args_sort_order($a) <=>
-                         $self->_action_args_sort_order($b) }
-            @{ $self->_paths->{$path} || [] };
+    my @actions = @{ $self->_paths->{$path} || [] };
 
     foreach my $action ( @actions ) {
         next unless $action->match($c);
@@ -130,8 +116,11 @@ sub register_path {
     $path =~ s!^/!!;
     $path = '/' unless length $path;
     $path = URI->new($path)->canonical;
+    $path =~ s{(?<=[^/])/+\z}{};
 
-    unshift( @{ $self->_paths->{$path} ||= [] }, $action);
+    $self->_paths->{$path} = [
+        sort { $a <=> $b } ($action, @{ $self->_paths->{$path} || [] })
+    ];
 
     return 1;
 }
index 88ea7d5..be7787a 100644 (file)
@@ -370,9 +370,7 @@ sub prepare_action {
 
   DESCEND: while (@path) {
         $path = join '/', @path;
-        $path =~ s#^/##;
-
-        $path = '' if $path eq '/';    # Root action
+        $path =~ s#^/+##;
 
         # Check out dispatch types to see if any will handle the path at
         # this level
@@ -459,9 +457,6 @@ sub get_containers {
     }
 
     return reverse grep { defined } @containers, $self->_container_hash->{''};
-
-    #return (split '/', $namespace); # isnt this more clear?
-    my @parts = split '/', $namespace;
 }
 
 =head2 $self->uri_for_action($action, \@captures)
@@ -531,9 +526,28 @@ sub register {
         }
     }
 
+    my @dtypes = @{ $self->_dispatch_types };
+    my @normal_dtypes;
+    my @low_precedence_dtypes;
+
+    for my $type ( @dtypes ) {
+        if ($type->_is_low_precedence) {
+            push @low_precedence_dtypes, $type;
+        } else {
+            push @normal_dtypes, $type;
+        }
+    }
+
     # Pass the action to our dispatch types so they can register it if reqd.
-    foreach my $type ( @{ $self->_dispatch_types } ) {
-        $type->register( $c, $action );
+    my $was_registered = 0;
+    foreach my $type ( @normal_dtypes ) {
+        $was_registered = 1 if $type->register( $c, $action );
+    }
+
+    if (not $was_registered) {
+        foreach my $type ( @low_precedence_dtypes ) {
+            $type->register( $c, $action );
+        }
     }
 
     my $namespace = $action->namespace;
diff --git a/t/aggregate/live_component_controller_action_index_or_default.t b/t/aggregate/live_component_controller_action_index_or_default.t
new file mode 100644 (file)
index 0000000..e10d6d2
--- /dev/null
@@ -0,0 +1,40 @@
+#!perl
+
+use strict;
+use warnings;
+
+use FindBin;
+use lib "$FindBin::Bin/../lib";
+
+our $iters;
+
+BEGIN { $iters = $ENV{CAT_BENCH_ITERS} || 1; }
+
+use Test::More tests => 6*$iters;
+
+use Catalyst::Test 'TestAppIndexDefault';
+
+if ( $ENV{CAT_BENCHMARK} ) {
+    require Benchmark;
+    Benchmark::timethis( $iters, \&run_tests );
+}
+else {
+    for ( 1 .. $iters ) {
+        run_tests();
+    }
+}
+
+sub run_tests {
+    is(get('/indexchained'), 'index_chained', ':Chained overrides index');
+    is(get('/indexprivate'), 'index_private', 'index : Private still works');
+
+# test :Path overriding default
+    is(get('/one_arg'), 'path_one_arg', ':Path overrides default');
+    is(get('/one_arg/foo/bar'), 'default', 'default still works');
+
+# now the same thing with a namespace, and a trailing / on the :Path
+    is(get('/default/one_arg'), 'default_path_one_arg',
+        ':Path overrides default');
+    is(get('/default/one_arg/foo/bar'), 'default_default',
+        'default still works');
+}
diff --git a/t/lib/TestAppIndexDefault.pm b/t/lib/TestAppIndexDefault.pm
new file mode 100644 (file)
index 0000000..9a129cb
--- /dev/null
@@ -0,0 +1,8 @@
+package TestAppIndexDefault;
+use strict;
+use warnings;
+use Catalyst;
+
+__PACKAGE__->setup;
+
+1;
diff --git a/t/lib/TestAppIndexDefault/Controller/Default.pm b/t/lib/TestAppIndexDefault/Controller/Default.pm
new file mode 100644 (file)
index 0000000..1009f30
--- /dev/null
@@ -0,0 +1,15 @@
+package TestAppIndexDefault::Controller::Default;
+
+use base 'Catalyst::Controller';
+
+sub default : Private {
+    my ($self, $c) = @_;
+    $c->res->body('default_default');
+}
+
+sub path_one_arg : Path('/default/') Args(1) {
+    my ($self, $c) = @_;
+    $c->res->body('default_path_one_arg');
+}
+
+1;
diff --git a/t/lib/TestAppIndexDefault/Controller/IndexChained.pm b/t/lib/TestAppIndexDefault/Controller/IndexChained.pm
new file mode 100644 (file)
index 0000000..18a8034
--- /dev/null
@@ -0,0 +1,12 @@
+package TestAppIndexDefault::Controller::IndexChained;
+
+use base 'Catalyst::Controller';
+
+sub index : Chained('/') PathPart('indexchained') CaptureArgs(0) {}
+
+sub index_endpoint : Chained('index') PathPart('') Args(0) {
+    my ($self, $c) = @_;
+    $c->res->body('index_chained');
+}
+
+1;
diff --git a/t/lib/TestAppIndexDefault/Controller/IndexPrivate.pm b/t/lib/TestAppIndexDefault/Controller/IndexPrivate.pm
new file mode 100644 (file)
index 0000000..08367ff
--- /dev/null
@@ -0,0 +1,10 @@
+package TestAppIndexDefault::Controller::IndexPrivate;
+
+use base 'Catalyst::Controller';
+
+sub index : Private {
+    my ($self, $c) = @_;
+    $c->res->body('index_private');
+}
+
+1;
diff --git a/t/lib/TestAppIndexDefault/Controller/Root.pm b/t/lib/TestAppIndexDefault/Controller/Root.pm
new file mode 100644 (file)
index 0000000..8acebf8
--- /dev/null
@@ -0,0 +1,17 @@
+package TestAppIndexDefault::Controller::Root;
+
+use base 'Catalyst::Controller';
+
+__PACKAGE__->config->{namespace} = '';
+
+sub default : Private {
+    my ($self, $c) = @_;
+    $c->res->body('default');
+}
+
+sub path_one_arg : Path('/') Args(1) {
+    my ($self, $c) = @_;
+    $c->res->body('path_one_arg');
+}
+
+1;