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

Removed request_eu? helper method #18

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

kogre
Copy link

@kogre kogre commented Nov 29, 2021

Description

This PR removes the request_eu? helper helper method from this gem. In practice, it rolls back this PR.

The main reason to remove it, is that it seems to be unrelated to the purpose of this gem. It doesn't seem to belong here.

Additionally, what triggered me to open this PR: The gem depends on ActionController::Helpers being available in the ApplicationController, which is not the case for Rails api-only projects.

Library-Specific
  • Increment the changelog (CHANGELOG.md)

@metaskills
Copy link
Contributor

Thanks. We use this helper as a convenience method for CloudFront headers. So it does feel like it somewhat belongs. Could it use more documentation or a change to avoid issues you were having?

@kogre
Copy link
Author

kogre commented Dec 5, 2021

Thanks for the quick response. My original message was a bit rushed, didn't want to come over as demanding or ungrateful, so allow me to clarify.

I disagree that the helper somewhat belongs here, at least if you consider this a general-purpose gem, based on the readme or the gem description.

Configure ActionDispatch::RemoteIp trusted proxies for Amazon CloudFront.

Adding a view helper method to the ApplicationController, to specifically detect whether a user is in the EU from CF headers is really something different, and feels very random and app-specific to me. It also assumes more about the app that adopts it than needed for its purpose, like ApplicationController being the root controller and ActionController::Helpers being used. So I think removing it, from that perspective, is the right decision.

I thought about solutions to solve it, like

  • Add some kind of parameter to make adding the helper optional
  • Use a different mechanism than a mixed in helper method to provide the functionality

But both solutions would require changes to your application (that's using the gem) anyway, which draws me back to the original conclusion that it would make more sense to make that code part of your application, or another gem, and remove it from this gem entirely.

I can come up with ways to solve this for us (e.g. include ActionController::Helpers, implement a fake dummy helper_method method), but it all feels rather hacky so rather won't go there.

If this PR is something you can't make work with the way you use the gem, or you disagree, that's your decision to make, and I totally understand and respect that. Feel free to close this. Thank you for providing the gem in the first place, it's saving us quite some work, even if we'll have to run with a fork!

@metaskills
Copy link
Contributor

didn't want to come over as demanding or ungrateful

Nah! Came across just fine!

But both solutions would require changes to your application

Well, the config could default to true and for your app you could set it to false. This would be important for us since we are using the method now. Then maybe in a future major version number we can default that the value to false. How does that sound?

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.

2 participants