From: Tomas Doran Date: Sun, 7 Aug 2011 18:42:06 +0000 (+0100) Subject: Merge what I've done over the weekend X-Git-Tag: 5.9000~10^2~3 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=catagits%2FCatalyst-Runtime.git;a=commitdiff_plain;h=7ebac5f89810aab16ec76fc28dea45e936172a67;hp=8e03d1bada5e205dbe4b9aad181ed606dd04dc4e Merge what I've done over the weekend --- diff --git a/Changes b/Changes index b2aea32..35028a9 100644 --- a/Changes +++ b/Changes @@ -1,5 +1,36 @@ # This file documents the revision history for Perl extension Catalyst. + Fixed extensions: + + - A number of modules have been updated to pass their tests or not + produce deprecation warnings with the latest version of Catalyst. + + These are: + + Test::WWW::Mechanize::Catalyst - has been updated to not produce + deprecation warnings. + + Catalyst::ActionRole::ACL - has been updated to fix failing tests + (although older versions still function perfectly with this + version of Catalyst). + + Catalyst::Plugin::Session::Store::DBIC - has been updated to fix + failing tests (although older versions still function perfectly + with this version of Catalyst). + + Backward compatibility fixes: + + - Fix calling MyApp->engine_class to set the engine class manually. + + - Re-add a $res->headers->{status} field to Catalyst::Test responses. + This _should_ be accessed with $c->res->code instead, but is here + for backward compatibility. + + Documentation: + + - Documentation which was in the now removed Catalyst::Engine::* classes + has been moved to Catalyst::Manual::Deployment + 5.89003 2011-07-28 20:11:50 (TRIAL release) Backward compatibility fixes: diff --git a/Makefile.PL b/Makefile.PL index 63a99cd..a2d9499 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -137,27 +137,32 @@ my %conflicts = ( 'Catalyst::Plugin::Unicode::Encoding' => '0.2', 'Catalyst::Plugin::Authentication' => '0.10010', # _config accessor in ::Credential::Password 'Catalyst::Authentication::Credential::HTTP' => '1.009', - 'Catalyst::Plugin::Session::Store::File' => '0.16', - 'Catalyst::Plugin::Session' => '0.21', - 'Catalyst::Plugin::Session::State::Cookie' => '0.10', + 'Catalyst::Plugin::Session::Store::File' => '0.16', + 'Catalyst::Plugin::Session' => '0.21', + 'Catalyst::Plugin::Session::State::Cookie' => '0.10', 'Catalyst::Plugin::Session::Store::FastMmap' => '0.09', - 'Catalyst::Controller::AllowDisable' => '0.03', - 'Reaction' => '0.001999', - 'Catalyst::Plugin::Upload::Image::Magick' => '0.03', - 'Catalyst::Plugin::ConfigLoader' => '0.22', # Older versions work but + 'Catalyst::Controller::AllowDisable' => '0.03', + 'Reaction' => '0.001999', + 'Catalyst::Plugin::Upload::Image::Magick' => '0.03', + 'Catalyst::Plugin::ConfigLoader' => '0.22', # Older versions work but # throw Data::Visitor warns - 'Catalyst::Devel' => '1.19', - 'Catalyst::Plugin::SmartURI' => '0.032', - 'CatalystX::CRUD' => '0.37', - 'Catalyst::Action::RenderView' => '0.07', - 'Catalyst::Plugin::DebugCookie' => '0.999002', - 'Catalyst::Plugin::Authentication' => '0.100091', - 'CatalystX::Imports' => '0.03', - 'Catalyst::Plugin::HashedCookies' => '1.03', - 'Catalyst::Action::REST' => '0.67', - 'CatalystX::CRUD' => '0.42', - 'CatalystX::CRUD::Model::RDBO' => '0.20', - 'Catalyst::View::Mason' => '0.17', + 'Catalyst::Devel' => '1.19', + 'Catalyst::Plugin::SmartURI' => '0.032', + 'CatalystX::CRUD' => '0.37', + 'Catalyst::Action::RenderView' => '0.07', + 'Catalyst::Plugin::DebugCookie' => '0.999002', + 'Catalyst::Plugin::Authentication' => '0.100091', + 'CatalystX::Imports' => '0.03', + 'Catalyst::Plugin::HashedCookies' => '1.03', + 'Catalyst::Action::REST' => '0.67', + 'CatalystX::CRUD' => '0.42', + 'CatalystX::CRUD::Model::RDBO' => '0.20', + 'Catalyst::View::Mason' => '0.17', +# Note these are not actually needed - they fail tests against the +# new version, but still work fine.. +# 'Catalyst::ActionRole::ACL' => '0.05', +# 'Catalyst::Plugin::Session::Store::DBIC' => '0.11', + 'Test::WWW::Mechanize::Catalyst' => '0.53', # Dep warnings unless upgraded. ); check_conflicts(%conflicts); diff --git a/TODO b/TODO index fe3d68f..b905301 100644 --- a/TODO +++ b/TODO @@ -24,48 +24,59 @@ subclass of Catalyst::Log, no ::Plugin:: needed. See also: Catalyst::Plugin::Log::Dispatch and http://github.com/willert/catalyst-plugin-log4perl-simple/tree -# REFACTORING +## Capture arguments that the plack engine component was run with somewhere, + to more easily support custom args from scripts (e.g. Gitalist's + --git_dir) -## PSGI +## throw away the restarter and allow using the restarters Plack provides -### Blockers +## remove per-request state from the engine instance - * Fix nginx middlewares so that they are generic, or can somehow - be used by people with their own .psgi files - * Fix a sane / nicer way to do custom engines. - * I've noticed a small difference with Catalyst::Test. The latest stable - version include two headers, 'host' and 'https'. They are missing from - this version. +## be smarter about how we use PSGI - not every response needs to be delayed + and streaming -### Things to discuss +# The horrible hack for plugin setup - replacing it: + + * Have a look at the Devel::REPL BEFORE_PLUGIN stuff + I wonder if what we need is that combined with plugins-as-roles * Catalyst::Engine::HTTP::Prefork no longer works since it requires Catalyst::Engine::CGI which no longer is in the cataplack distribution. Investigation shows moving CE:CGI to CE:HTTP:Prefork allows tests to pass. -### PSGI Release Punchlist +# PSGI - * Release a version of Task::Catalyst without CE:PSGI and CE:HTTP:Prefork - since those are deprecated. +## To do at release time -#### Script survey + - Release psgi branch of Catalyst-Devel + - Release new Task::Catalyst + - Release 5.9 branch of Catalyst-Manual + - Release Catalyst::Engine::HTTP::Prefork with deprecation notice + (and maybe compat?) -### Nice to have +## Blockers - * Capture arguments that the plack engine component was run with somewhere, - to more easily support custom args from scripts (e.g. Gitalist's - --git_dir) - * throw away the restarter and allow using the restarters Plack provides - * remove per-request state from the engine instance - * be smarter about how we use PSGI - not every response needs to be delayed - and streaming + * Better docs for stopping using Engine::HTTP::Prefork and + starting using Starman, maybe? -## The horrible hack for plugin setup - replacing it: + * Test::WWW::Mechanize::Catalyst new release - * Have a look at the Devel::REPL BEFORE_PLUGIN stuff - I wonder if what we need is that combined with plugins-as-roles + * Test nginx middleware to determine if it is needed with: + + root app - with use_request_uri_for_path + root app - without use_request_uri_for_path + non-root app - with use_request_uri_for_path + non-root app - without use_request_uri_for_path + + If it isn't needed, remove. If it is needed, split it out into it's own + file and document why it's needed. + + * I've noticed a small difference with Catalyst::Test. The latest stable + version include two headers, 'host' and 'https'. They are missing from + this version - Pedro Melo on list + ^^ Cannot replicate this? Mailed back to ask for tests.. -## App / ctx split: +# App / ctx split: NOTE - these are notes that t0m thought up after doing back compat for catalyst_component_class, may be inaccurate, wrong or missing things diff --git a/lib/Catalyst.pm b/lib/Catalyst.pm index 5626071..03f34f8 100644 --- a/lib/Catalyst.pm +++ b/lib/Catalyst.pm @@ -2603,26 +2603,28 @@ Sets up engine. =cut sub engine_class { - my $class = shift; - confess("Setting ->engine_class manually is no longer supported. XXX FIXME") if scalar @_; + my ($class, $requested_engine) = @_; + + if (!$class->engine_loader || $requested_engine) { + $class->engine_loader( + Catalyst::EngineLoader->new({ + application_name => $class, + (defined $requested_engine + ? (requested_engine => $requested_engine) : ()), + }), + ); + } $class->engine_loader->catalyst_engine_class; } sub setup_engine { my ($class, $requested_engine) = @_; - $class->engine_loader( - Catalyst::EngineLoader->new({ - application_name => $class, - (defined $requested_engine - ? (requested_engine => $requested_engine) : ()), - }), - ); + my $engine = $class->engine_class($requested_engine); # Don't really setup_engine -- see _setup_psgi_app for explanation. return if $class->loading_psgi_file; - my $engine = $class->engine_class; Class::MOP::load_class($engine); if ($ENV{MOD_PERL}) { @@ -3059,8 +3061,46 @@ to be shown in hit debug tables in the test server. =item * C - Controlls if the C or C environment -variable should be used for determining the request path. See L -for more information. +variable should be used for determining the request path. + +Most web server environments pass the requested path to the application using environment variables, +from which Catalyst has to reconstruct the request base (i.e. the top level path to / in the application, +exposed as C<< $c->request->base >>) and the request path below that base. + +There are two methods of doing this, both of which have advantages and disadvantages. Which method is used +is determined by the C<< $c->config(use_request_uri_for_path) >> setting (which can either be true or false). + +=over + +=item use_request_uri_for_path => 0 + +This is the default (and the) traditional method that Catalyst has used for determining the path information. +The path is synthesised from a combination of the C and C environment variables. +The allows the application to behave correctly when C is being used to redirect requests +into the application, as these variables are adjusted by mod_rewrite to take account for the redirect. + +However this method has the major disadvantage that it is impossible to correctly decode some elements +of the path, as RFC 3875 says: "C<< Unlike a URI path, the PATH_INFO is not URL-encoded, and cannot +contain path-segment parameters. >>" This means PATH_INFO is B decoded, and therefore Catalyst +can't distinguish / vs %2F in paths (in addition to other encoded values). + +=item use_request_uri_for_path => 1 + +This method uses the C and C environment variables. As C is never +decoded, this means that applications using this mode can correctly handle URIs including the %2F character +(i.e. with C set to C in Apache). + +Given that this method of path resolution is provably more correct, it is recommended that you use +this unless you have a specific need to deploy your application in a non-standard environment, and you are +aware of the implications of not being able to handle encoded URI paths correctly. + +However it also means that in a number of cases when the app isn't installed directly at a path, but instead +is having paths rewritten into it (e.g. as a .cgi/fcgi in a public_html directory, with mod_rewrite in a +.htaccess file, or when SSI is used to rewrite pages into the app, or when sub-paths of the app are exposed +at other URIs than that which the app is 'normally' based at with C), the resolution of +C<< $c->request->base >> will be incorrect. + +=back =item * diff --git a/lib/Catalyst/Test.pm b/lib/Catalyst/Test.pm index ef3def5..5c0cbe7 100644 --- a/lib/Catalyst/Test.pm +++ b/lib/Catalyst/Test.pm @@ -311,6 +311,10 @@ sub _local_request { for my $f ( $h->header_field_names ) { $resp->init_header( $f, [ $h->header($f) ] ); } + # Another horrible hack to make the response headers have a + # 'status' field. This is for back-compat, but you should + # call $resp->code instead! + $resp->init_header('status', [ $resp->code ]); }, }, @_); } diff --git a/t/aggregate/unit_load_catalyst_test.t b/t/aggregate/unit_load_catalyst_test.t index 399b190..68dfbdf 100644 --- a/t/aggregate/unit_load_catalyst_test.t +++ b/t/aggregate/unit_load_catalyst_test.t @@ -153,4 +153,8 @@ lives_ok { request(GET('/dummy'), []); } 'array additional param to request method ignored'; +my $res = request(GET('/')); +is $res->code, 200, 'Response code 200'; +is $res->headers->{status}, 200, 'Back compat "status" header present'; + done_testing;