| Subject: | The working of $c->view() with no defaults |
A few days ago, me and my colleagues at work have
been bitten by a bug in a Catalyst application
which used Catalyst::Action::RenderView.
This application used two views
App::View::TT
App::View::JSON
We used at a base controller
sub end : ActionClass('RenderView') {}
and expected to work ok (using App::View::TT).
And so it did... until we renamed the application.
Then it started to use the JSON view.
The solution was rather obvious to my
co-workers (which are more used to Catalyst
development): include a "default_view" in
the config file.
I investigated further the case because it
bugged me having no help of Catalyst itself
to diagnose this.
The docs of Catalyst::Action::RenderView says:
"This action implements a sensible default
end action, which will forward to the first
available view."
which is a little cunning for an unaware
developer.
I traced the code behavior to the implementation
of Catalyst::view() when used without parameters.
The docs says:
=head2 $c->view($name)
...
If the name is omitted, it will look for
- a view object in $c->stash{current_view_instance}, then
- a view name in $c->stash->{current_view}, then
- a config setting 'default_view', or
- check if there is only one view, and return it if that's the case.
and there was tests at t/unit_core_mvc.t
is (MyApp->view , 'MyApp::View::V', 'view() with no defaults ok');
But contrary to the documentation, "MyApp->view" should not
return such answer at this case, because there is not only one
view in the application skeleton being tested.
The problem lives in the code of _comp_singular,
which is supposed to "return a component if only one matches".
--- Catalyst-Runtime-5.7011/lib/Catalyst.pm 2007-10-18
15:06:26.000000000 -0300
+++ Catalyst-Runtime/lib/Catalyst.pm 2007-11-09 23:52:27.000000000 -0200
@@ -474,7 +474,7 @@
my ( $comp, $rest ) =
map { $c->_comp_search("^${appclass}::${_}::") } @prefixes;
- return $comp unless $rest;
+ return $rest ? undef : $comp;
}
The last line "return $comp unless $rest" works ok
if there is only one view. But if there is more than
one view, the return depends on the last statement
(which is the result of the "map"). Thus, it always
returned a component that matched the criteria.
I don't think this undefined behavior which can
change if the implementation changes is desirable.
So the fix is a more explicit expression like
return $rest ? undef : $comp;
And then $c->view() with no defaults will return
undef if there is more than one view.
The "bug" affected "$c->model()" as well and so
does the patch.
The attached patch tweaks this bit and the mentioned
test to make this coherent with this argument.
NOTE. Obviously, if many Catalyst developers
out there and Catalyst applications rely on this
behavior (namely, "$c->view() with no defaults return
some view"), this change may be not the right
thing to do.
| Subject: | view-patch.diff |
diff -ru Catalyst-Runtime-5.7011/lib/Catalyst.pm Catalyst-Runtime/lib/Catalyst.pm
--- Catalyst-Runtime-5.7011/lib/Catalyst.pm 2007-10-18 15:06:26.000000000 -0300
+++ Catalyst-Runtime/lib/Catalyst.pm 2007-11-09 23:52:27.000000000 -0200
@@ -474,7 +474,7 @@
my ( $comp, $rest ) =
map { $c->_comp_search("^${appclass}::${_}::") } @prefixes;
- return $comp unless $rest;
+ return $rest ? undef : $comp;
}
# Filter a component before returning by calling ACCEPT_CONTEXT if available
diff -ru Catalyst-Runtime-5.7011/t/unit_core_mvc.t Catalyst-Runtime/t/unit_core_mvc.t
--- Catalyst-Runtime-5.7011/t/unit_core_mvc.t 2007-09-20 06:29:36.000000000 -0300
+++ Catalyst-Runtime/t/unit_core_mvc.t 2007-11-09 23:50:42.000000000 -0200
@@ -1,17 +1,23 @@
-use Test::More tests => 27;
+use Test::More tests => 29;
use strict;
use warnings;
use_ok('Catalyst');
-my @complist =
- map { "MyApp::$_"; }
- qw/C::Controller M::Model V::View Controller::C Model::M View::V Controller::Model::Dummy::Model Model::Dummy::Model/;
-
-my $thingie={};
-bless $thingie,'MyApp::Model::Test::Object';
-push @complist,$thingie;
{
+ my @complist =
+ map { "MyApp::$_"; }
+ qw/ C::Controller
+ M::Model
+ V::View
+ Controller::C
+ Model::M
+ View::V
+ Controller::Model::Dummy::Model
+ Model::Dummy::Model /;
+
+ my $thingie = bless {},'MyApp::Model::Test::Object';
+ push @complist,$thingie;
package MyApp;
@@ -51,7 +57,7 @@
[ qw/Dummy::Model M Model Test::Object/ ],
'models ok');
-is (MyApp->view , 'MyApp::V::View', 'view() with no defaults ok');
+is (MyApp->view , undef, 'view() with no defaults returns undef (two views)');
is ( bless ({stash=>{current_view=>'V'}}, 'MyApp')->view , 'MyApp::View::V', 'current_view ok');
@@ -61,7 +67,7 @@
is ( bless ({stash=>{current_view_instance=> $view, current_view=>'MyApp::V::View' }}, 'MyApp')->view , $view,
'current_view_instance precedes current_view ok');
-is (MyApp->model , 'MyApp::M::Model', 'model() with no defaults ok');
+is (MyApp->model , undef, 'model() with no defaults returns undef (multiple models)');
is ( bless ({stash=>{current_model=>'M'}}, 'MyApp')->model , 'MyApp::Model::M', 'current_model ok');
@@ -90,3 +96,19 @@
is_deeply($args, [qw/foo bar/], '$c->model args passed to ACCEPT_CONTEXT ok');
MyApp->view('V', qw/baz moo/);
is_deeply($args, [qw/baz moo/], '$c->view args passed to ACCEPT_CONTEXT ok');
+
+{
+ package MyApp2;
+
+ use base qw/Catalyst/;
+
+ my @complist = map { __PACKAGE__ . '::' . $_ }
+ qw/ Model::M
+ View::V
+ /;
+ __PACKAGE__->components( { map { ( ref($_)||$_ , $_ ) } @complist } );
+}
+
+is (MyApp2->view , 'MyApp2::View::V', 'view() with no defaults ok');
+
+is (MyApp2->model , 'MyApp2::Model::M', 'model() with no defaults ok');
\ No newline at end of file