-
Notifications
You must be signed in to change notification settings - Fork 274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Feature no default middleware #1459
Conversation
If `no_default_middleware` is true, do NOT wrap the app in * Plack::Middleware::FixMissingBodyInRedirect * Plack::Middleware::Head This allows anyone who needs to manipulate response headers based on the response body in middleware to do so, before middleware layers like Plack::Middleware::Head are applied. These default middleware layers can then be applied as part of the app's PSGI stack (not part of Dancer2's internals). Tests included!
In this case, we determined some middlewares to be implementations of features we want in the web framework, but do not want to write it ourselves. Kind of like how you decide you see a plugin you want in core and just make it core, despite it written as a plugin. Both |
I think we are stating the same thing different ways. Agreed that both If you are building a PSGI app for which Dancer2 is just one part; there may be situations where you require other middleware to act on the response before Plack::Middleware::Head is applied. Adding eTag headers is one example; you can't calculate a (weak) eTag in middleware wrapping a Dancer2 app for a HEAD request once the content has been removed by our current use of Plack::Middleware::Head. Adding more middleware into our stack isn't the way to go; so this PR goes the other way and allows the two middleware we use by default to be removed. |
I like this, @veryrusty! 👍 Might I suggest (in a separate PR) that we add some examples of ETags and ContentMD5 in the cookbook? You cover the why in this PR very well, it might be nice to show people how this change is best used. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good!
Merged, thank you! |
[ BUG FIXES ] * GH #1427: Allow layout_dir to be configured by set keyword (Russell @veryrusty Jenkins) * GH #1456: Engine logging respects minimum level filtering (Daniel Perrett) * PR #1479: Remove arbitrary Perl 5.10 requirement from tests (Dan Book) * PR #1480: Correct dynamic HTTP::XSCookies requirement (Dan Book) * PR #1486: Install dzil deps for use by Appveyor (Dan Book) [ ENHANCEMENTS ] * GH #1418: Send plain text content with send_as() (Steve Dondley) * PR #1457: Serializer mutable with custom mapping. Also resolves issues #795, #973, and #901 (Russell @veryrusty Jenkins, Yanick Champoux, Daniel Böhmer, Steven Humphrey) * PR #1459: Add no default middleware feature. Also resolves #1410 (Russell @veryrusty Jenkins) * GH #1469: Code of Conduct enhancements (MaxPerl) [ DOCUMENTATION ] * GH #1166: Add behind_proxy docs to Deployment manual (Nuno Ramos Carvalho) * GH #1417: Add "set engines" documentation (Deirdre Moran) * PR #1450: Add calculator example (Gabor Szabo) * PR #1452: Fix Pod formatting for CPAN (simbaque) * PR #1454: Fix typos in docs (Gil Magno) * PR #1464: Can't set environment with 'set' keyword (Ben Kaufman) * PR #1470: Use session for flash and explain in detail (simbaque) * PR #1472: Migration, tutorial, other doc fixes (Jason A. Crome) * PR #1473: Show support resources after generating new app (Jason A. Crome) * PR #1474: Use the correct URL for HAProxy (Jason A. Crome) * PR #1475: Add manual section for security concerns (Jason A. Crome) * PR #1487: Clarify deprecation of Dancer2::Test (Steve Dondley)
Add a
no_default_middleware
config option that when set to a true value, does not wrap Dancer2 apps in the following default middleware:These are both "nice to have" and completely sane for general use. However there are some cases where a developer may want to manipulate the response headers for say a HEAD request in their PSGI stack; but as we drop the request content (via Plack::Middleware::Head) its currently not possible to do so.
This
no_default_middleware
approach was based on @xsawyerx comment of "Middlewares are not application level and we shouldn't confuse them" (see #600 (comment)). Going this way also resolves issues such as #1410.