Give warnings in to_app() auto promotion #369

Merged
merged 2 commits into from Feb 8, 2013

1 participant

@miyagawa
plack member

Here's one of the under-documented features in Plack: If a .psgi file returns an object that has to_app method, plackup will automatically call the method, to make it a PSGI code reference. It was merely a shortcut to make Plack::App and Component subclasses to work even when you forget ->to_app.

However - When you combine that with Plack::Builder with builder { ...; $app }, or use the command line plackup -e 'Plack::App::File->new' with the default development environment, the application object is now wrapped by the builder and default middleware - which means there's no chance of calling ->to_app in the loaders.

PSGI handlers will now have $app as an object to call as a code ref, and at the runtime, Plack::App::Component automatically promotes itself to the coderef via to_app, with the sub overloading, for each request.

This is bad in terms of performance and all that, so here're two attempts in this pull request to make it less worse:

a) Gives warning when such automatic conversion happens, when PLACK_ENV is set to development, so that developers will notice that something is wrong, and that they have to call ->to_app to get the right PSGI application.

b) Move the automatic promotion code to Builder. It doesn't feel like a strictly correct place to do that, but this would catch 99% of the use case where this whole things becomes an issue. Also it is necessary for a backward compatibility reason, to prevent the actual use of plackup -e 'Plack::App::File->new' from breaking with the upgrade.

miyagawa added some commits Feb 7, 2013
@miyagawa miyagawa Add warnings when ->to_app implicit promotion is done.
Usually it's developer's mistake where they forgot ->to_app after
Plack::App::Foo->new(). This patch gives warnings in such case in
the development environment.

This will probably give lots of warnings with the command line use
such as:

  plackup -MPlack::App::File -e 'Plack::App::File->new'

which i will work around with a separate commit.
b2455e1
@miyagawa miyagawa Move to_app auto promotion to Builder instead of load_psgi ec6d32c
@miyagawa miyagawa merged commit 8ba7fcb into master Feb 8, 2013
@miyagawa miyagawa added a commit that referenced this pull request Feb 8, 2013
@miyagawa miyagawa Checking in changes prior to tagging of version 1.0017.
Changelog diff is:

diff --git a/Changes b/Changes
index 084f1fd..db54eea 100644
--- a/Changes
+++ b/Changes
@@ -1,5 +1,15 @@
 Go to http://github.com/plack/Plack/issues for the roadmap and known issues.

+1.0017 Thu Feb  7 19:21:24 PST 2013
+    [INCOMPATIBLE CHANGES]
+        - Gives you warnings when you use one of Plack::App objects in `plackup -e` or
+          in .psgi files but forgot to call ->to_app to make it a PSGI application (#369)
+          Still automatically converts them for backward compatibility, but in the
+          loading time inside Plack::Builder.
+
+    [BUG FIXES]
+        - chdir to the CGI path when executing CGIBin (#338, #368)
+
 1.0016 Thu Jan 31 13:21:14 PST 2013
     [SECURITY]
         - Fixed directory traversal bug in Plack::App::File on win32 environments
d81ca20
@karenetheridge karenetheridge added a commit to karenetheridge/Plack that referenced this pull request Feb 14, 2013
@miyagawa miyagawa Checking in changes prior to tagging of version 1.0017.
Changelog diff is:

diff --git a/Changes b/Changes
index 084f1fd..db54eea 100644
--- a/Changes
+++ b/Changes
@@ -1,5 +1,15 @@
 Go to http://github.com/plack/Plack/issues for the roadmap and known issues.

+1.0017 Thu Feb  7 19:21:24 PST 2013
+    [INCOMPATIBLE CHANGES]
+        - Gives you warnings when you use one of Plack::App objects in `plackup -e` or
+          in .psgi files but forgot to call ->to_app to make it a PSGI application (#369)
+          Still automatically converts them for backward compatibility, but in the
+          loading time inside Plack::Builder.
+
+    [BUG FIXES]
+        - chdir to the CGI path when executing CGIBin (#338, #368)
+
 1.0016 Thu Jan 31 13:21:14 PST 2013
     [SECURITY]
         - Fixed directory traversal bug in Plack::App::File on win32 environments
35805bf
@miyagawa miyagawa added a commit that referenced this pull request Mar 8, 2013
@miyagawa miyagawa Checking in changes prior to tagging of version 1.0018.
Changelog diff is:

diff --git a/Changes b/Changes
index db54eea..26bb00c 100644
--- a/Changes
+++ b/Changes
@@ -1,6 +1,16 @@
 Go to http://github.com/plack/Plack/issues for the roadmap and known issues.

-1.0017 Thu Feb  7 19:21:24 PST 2013
+1.0018 Fri Mar  8 10:43:45 PST 2013
+    [IMPROVEMENTS]
+        - Performance boost in Plack::Request#query_parameters (lestrrat)
+        - Added custom log formats for %m, %U, %q and %H (Hiroshi Sakai)
+        - Fixed warnings in SimpleContentFilter (earino)
+
+    [DOCUMENTATION]
+        - Added docs about plackup --path
+        - Added docs about using manager object in Plack::Handler::FCGI
+
+1.0017-TRIAL Thu Feb  7 19:21:24 PST 2013
     [INCOMPATIBLE CHANGES]
         - Gives you warnings when you use one of Plack::App objects in `plackup -e` or
           in .psgi files but forgot to call ->to_app to make it a PSGI application (#369)
3fb5c9a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment