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

Make it clear that the cache store can be required #247

Closed
wants to merge 1 commit into from
Closed

Make it clear that the cache store can be required #247

wants to merge 1 commit into from

Conversation

frenkel
Copy link

@frenkel frenkel commented Oct 31, 2017

In order to prevent issues such as #225

grzuy
grzuy previously approved these changes Jan 19, 2018
Copy link
Collaborator

@grzuy grzuy left a comment

Choose a reason for hiding this comment

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

Sounds good to me

Copy link
Collaborator

@grzuy grzuy left a comment

Choose a reason for hiding this comment

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

Well, thinking again, you don't necessarily need to configure it. It depends whether it's defaulting to Rails.cache or not... right?

@grzuy grzuy dismissed their stale review March 9, 2018 20:12

Changed my mind after reviewing second time

@lmansur
Copy link
Contributor

lmansur commented Mar 15, 2018

The gem uses Rails.cache as a fall back in case you didn't configure Rack::Attack::Cache.store. Depending on your Rails version, Rails.cache in development environment uses NullStore, which doesn't actually store anything.

I think we could add some kind of notice regarding this possible case to avoid unnecessary debugging.

@grzuy
Copy link
Collaborator

grzuy commented Mar 15, 2018

Agree.

E.g. #274 was an attempt to aid users in debugging store config errors, but there's definitely room for more improvement. Either by giving better warning/errors and/or clearer docs.

@grzuy
Copy link
Collaborator

grzuy commented Mar 15, 2018

Created a new issue to track that need: #300

@grzuy
Copy link
Collaborator

grzuy commented May 17, 2018

Closing this PR for now given the conversation stalled, plus there were other changes which were trying to solve the same confusion as part of #300.

@frenkel Thank you for trying to contribute anyway. Let me know if you still think what you were trying to solve isn't yet covered, so we can continue discussing how to do that.

@grzuy grzuy closed this May 17, 2018
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