Give warnings in to_app() auto promotion #369
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
miyagawa
added a commit
that referenced
this pull request
Feb 8, 2013
Give warnings in to_app() auto promotion
miyagawa
added a commit
that referenced
this pull request
Feb 8, 2013
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
karenetheridge
pushed a commit
to karenetheridge/Plack
that referenced
this pull request
Feb 14, 2013
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 (plack#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 (plack#338, plack#368) + 1.0016 Thu Jan 31 13:21:14 PST 2013 [SECURITY] - Fixed directory traversal bug in Plack::App::File on win32 environments
miyagawa
added a commit
that referenced
this pull request
Mar 8, 2013
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)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Here's one of the under-documented features in Plack: If a .psgi file returns an object that has
to_appmethod, 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 lineplackup -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_appin the loaders.PSGI handlers will now have
$appas an object to call as a code ref, and at the runtime, Plack::App::Component automatically promotes itself to the coderef viato_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_appto 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.