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

Don't use prepend_before_action when loading/authorizing resources #730

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

Conversation

jagthedrummer
Copy link
Contributor

Using prepend_before_action causes problems because developers lose the ability to control the order in which callbacks happen.

Maybe fixes #531? (Hard to be sure since I don't know how to reproduce whatever problem people are running into.)

Supersedes #716

Using `prepend_before_action` causes problems because developers lose the ability to control the order in which callbacks happen.

Maybe fixes #531? (Hard to be sure since I don't know how to reproduce whatever problem people are running into.)

Supersedes #716
Copy link
Contributor

@kaspth kaspth left a comment

Choose a reason for hiding this comment

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

This seems alright to me, though I don't know how to reason about whether there'd be other unforeseen repercussions to this.

I wonder if we can explain this further in the eventual release notes? Or can we support passing prepend: true to get the old behavior?

@jagthedrummer
Copy link
Contributor Author

Yeah, this definitely strikes me as the kind of change that could definitely have unforeseen consequences. Especially because I don't really understand whatever problem people were running into.

Maybe a prepend param, or a global config option would be a good idea... 🤔

@kaspth
Copy link
Contributor

kaspth commented Jan 16, 2024

Yeah, this definitely strikes me as the kind of change that could definitely have unforeseen consequences. Especially because I don't really understand whatever problem people were running into.

Yeah, true. Sometimes these things can be a bit finicky.

I suppose we could do we something like this for a prepend param:

before_actions = [
  -> { before_action() },
  -> { before_action() },
]
before_actions.reverse! if options[:prepend]
before_actions.each(&:call)

I think this would be a relatively easy way to have backwardscompatibility — although it's a bit of do we want to carry this forward, versus just cutting it off and deal with the aftermath?

@jagthedrummer
Copy link
Contributor Author

although it's a bit of do we want to carry this forward, versus just cutting it off and deal with the aftermath?

🤔 Interesting proposal on using an array. But I don't fully understand how it would work.

Honestly, I'm a little hesitant to merge either until we hear from someone who's experienced the problem that we're trying to fix. I still feel like I'm flying blind on this one.

@jagthedrummer
Copy link
Contributor Author

@seattlecyclist @aaricpittman Can y'all give this branch a try and see if it fixes the problem you mentioned in #531?

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.

Execution order issue with load_and_authorize_resource?
2 participants