Skip to content
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

Closed
wants to merge 2 commits into from
Closed

Conversation

veryrusty
Copy link
Member

Add a no_default_middleware config option that when set to a true value, does not wrap Dancer2 apps in the following default middleware:

  • Plack::Middleware::FixMissingBodyInRedirect
  • Plack::Middleware::Head

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.

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!
@veryrusty veryrusty requested a review from cromedome June 26, 2018 04:50
@xsawyerx
Copy link
Member

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 Head and FixMissingBodyInRedirect make sense to be part of core because they're not extra functionality, they're part of what the HTTP spec says. (I thought of implementing the latter in core to prevent another subroutine wrapping for each request - which is not a bad thing. The former could also fit.)

@veryrusty
Copy link
Member Author

I think we are stating the same thing different ways. Agreed that both Head and FixMissingBodyInRedirect make sense to be part of core by default. This PR doesn't change that.

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.

@cromedome
Copy link
Contributor

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.

Copy link
Contributor

@yanick yanick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good!

@cromedome
Copy link
Contributor

Merged, thank you!

@cromedome cromedome closed this Nov 13, 2018
cromedome added a commit that referenced this pull request Nov 14, 2018
    [ 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)
@veryrusty veryrusty deleted the feature_no_default_middleware branch May 15, 2019 09:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants