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

use session for flash and explain in detail #1470

Merged

Conversation

simbabque
Copy link
Contributor

When we use a file-wide lexical variable as a closure in the flash functions, we risk that a different user sees the flash message because it's shared between all users. This would occur if a different user's request happened between the time the server returns the 302 response and their browser following it. Saving it to the session removes this problem, which would be hard to explain to a beginner and gives a chance to explain what a session is in more detail.

Closes #1465

When we use a file-wide lexical variable as a closure in the flash functions, we risk that a different user sees the flash message because it's shared between all users. This would occur if a different user's request happend between the time the server returns the 302 response and their browser following it. Saving it to the session removes this problem, which would be hard to explain to a beginner and gives a chance to explain what a session is in more detail.
@SysPete
Copy link
Member

SysPete commented Oct 1, 2018

👍

We also have plugins for this:

and I wonder whether one (or both) of these could be suggested as an alternative in the tutorial.

I personally prefer Dancer2::Plugin::Deferred since it also handles multi-tab race conditions.

@cromedome
Copy link
Contributor

👍 Thank you for doing this!

@SysPete - I think that is a great idea. What would you think of adding a "Further Reading" or some such section to the tutorial that provides links to modules and other sites that explain some of the tutorial topics in further detail? It might work in conjunction with (or replace) the See Also section that is currently there.

@cromedome cromedome merged commit 24952cf into PerlDancer:master Oct 2, 2018
@cromedome
Copy link
Contributor

Merged in at 1f48c36. Closing!

@simbabque
Copy link
Contributor Author

simbabque commented Oct 3, 2018 via email

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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants