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

Pre-discard flash messages #18721

Merged
merged 3 commits into from Feb 1, 2015

Conversation

Projects
None yet
3 participants
@sj26
Copy link
Contributor

sj26 commented Jan 29, 2015

We are running into CookieOverflow errors due to judicious use of flash.now which seemed odd. After some digging I found that the full flash, including discarded values, is being saved into the session between each request, yet the discarded values are immediately swept away when reconstituting the values. So why not discard them before marshalling? Seems to work. Saves a lot of cookie bytes, too.

Also revealed a bug where functional controller tests which removed all flash entries weren't removing the marshalled flash from the session when processing and recycling requests, a difference in behaviour to the actual middleware.

sj26 added some commits Jan 29, 2015

Fix flash remaining after last flash deleted
Inside a controller functional test after the last flash is deleted it
still persists the flash because to_session_value is nil. We should
delete it from the session when the serialized version is nil, same as
the flash middleware.

sj26 added a commit to envato/rails_4_session_flash_backport that referenced this pull request Jan 30, 2015

Pre-discard flash messages
Trims down session storage by throwing away values from the flash before
persisting into the session.

Also includes the patch for rails 4.

See rails/rails#18721

@sj26 sj26 referenced this pull request Jan 30, 2015

Merged

Pre-discard flash messages #12

sj26 added a commit to envato/rails_4_session_flash_backport that referenced this pull request Jan 30, 2015

Pre-discard flash messages
Trims down session storage by throwing away values from the flash before
persisting into the session.

Also includes the patch for rails 4.

See rails/rails#18721

@sj26 sj26 changed the title Pre discard flash Pre-discard flash messages Jan 30, 2015

tenderlove added a commit that referenced this pull request Feb 1, 2015

Merge pull request #18721 from sj26/pre-discard-flash
Pre-discard flash messages

@tenderlove tenderlove merged commit e6d8f43 into rails:master Feb 1, 2015

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details
@carlosantoniodasilva

This comment has been minimized.

Copy link
Member

carlosantoniodasilva commented Feb 1, 2015

I think we may need to add a changelog entry for the change on pre-discarding the message, just in case people end up having any issues with that, they'll have where to search for. Does it make sense?

@sj26

This comment has been minimized.

Copy link
Contributor Author

sj26 commented Feb 1, 2015

Yeah, it's a slight change. If one was accessing or manipulating the flash outside of rails (please no) or making assertions directly against the session (oh gosh I hope not) this could be surprising.

Side note, though: I've rolled this change into our rather large, heavily trafficked app using the backports gem without issue.

@sj26

This comment has been minimized.

Copy link
Contributor Author

sj26 commented Feb 2, 2015

(Thanks @tenderlove 💖)

rafaelfranca added a commit that referenced this pull request May 6, 2016

Make flash messages cookie compatible with Rails 4
In #18721 we removed the discard key from the session hash used to flash
messages and that broke compatibility with Rails 4 applications because they
try to map in the discarded flash messages and it returns nil.

Fixes #24726.

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request Jun 7, 2016

@sj26 sj26 deleted the sj26:pre-discard-flash branch Dec 5, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.