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

Issue #4939: don’t delete by default when retrieving stored_location_for #4946

Closed
wants to merge 2 commits into from
Closed

Issue #4939: don’t delete by default when retrieving stored_location_for #4946

wants to merge 2 commits into from

Conversation

murb
Copy link

@murb murb commented Sep 27, 2018

First attempt :)

As described in issue #4939; I found the current behaviour unpredictable. This pull request introduces a new method #pop_stored_location_for that returns and deletes the location stored (on all cases, my idea was that by making this explicit it is no longer required to check #is_navigational_format?)

Introducing a new method pop_stored_location_for; which also no longer
checks `is_navigational_format?` for increased predictability.
@murb
Copy link
Author

murb commented Sep 27, 2018

As for discussion: I don't know why the check for navigational_format? was introduced here. I don't think it is something such a method should be dealing with at all, and I'd argue that this can be perfectly decided upon per controller action.

@murb
Copy link
Author

murb commented Sep 28, 2018

not entirely happy with what I ended up with here... although it is a slight improvement by making popping the data more explicit by moving it up the call stack; the same issue still exists with after_sign_in_path (one would not assume from the method name that this adjusts state).

To fix this it would require a larger effort affecting even more code; I'd flag all checks for if_navigational_format? in *_path(resource)-methods as smelly, and I'd create new methods to deal with redirect_to/respond_withs. Not sure whether that's the right path to move forward ...

@tegon
Copy link
Member

tegon commented Dec 21, 2018

@murb I just finished to read your comments and the code. It turns out the rabbit hole was deeper than we initially though 😞

I agree with your point, there are a lot of mixed concerns on those _path_for methods - not only after_sign_in_path_for.
Sadly, it's going to be a big refactor to fix this, touching areas of the code where I don't know the reasons why they are there (like the if_navigational_format? check) and introducing a lot of breaking changes, since those methods are part of the public API.

That's why I will close this pull request. I'm sorry it didn't work out, but thank you for taking the time to tackling this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants