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

using a Hash instead of an an Array for Response#errors #303

Open
alperkokmen opened this issue Feb 11, 2016 · 2 comments
Open

using a Hash instead of an an Array for Response#errors #303

alperkokmen opened this issue Feb 11, 2016 · 2 comments

Comments

@alperkokmen
Copy link
Contributor

at the moment Response object uses an Array (initialized here) to keep track of errors encountered during validation.

at the moment, it's very human-friendly – i.e. if you wanted to display errors to users, you could just consume errors in your controller after is_valid? check and be done. however, i think it would be better for programmatically traversing errors and making sense of them if errors was a Hash.

easiest pattern i can think of is to leverage the suffix for each validate_XXX method. e.g.

def validate_num_assertion
  # validation fails...
  append_error(:num_assertion, "SAML Response must contain 1 assertion")
end

def validate_destination
  # validation fails...
  append_error(:destination, "The response was received at #{destination} instead of #{settings.assertion_consumer_service_url}")
end

obviously, this would be a breaking change; but, i think errors could be declared as deprecated and a Hash named errors_hash could be defined as a start to allow people upgrade easily. until deprecated error arrays is completely removed, errors and errors_hash could work side-by-side.

repo owners, i would appreciate if you could let me know if this makes sense. if it does, i am open to taking a first stab at it with a pull request.

@pitbulk
Copy link
Collaborator

pitbulk commented Mar 4, 2016

That makes sense but we need to implement a solution that not break current environments.
I'm not enthusiast about the deprecate solution.

@johnnyshields
Copy link
Collaborator

@alperkokmen (or anyone else), I think the right way to do this would be to make a global config option that toggles between the old way and the new way. PR would be welcome here.

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

No branches or pull requests

3 participants