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

avoiding the use of the StandardError exception #331

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

Conversation

MaximilianoGarciaRoe
Copy link

In general, it is better to rescue and display the specific error rather than showing a generic message in the view when working with Ruby. This is because a generic message may not provide enough information for the user to understand the problem and take steps to solve it.

By rescuing and displaying the specific error, more detailed information about the nature of the problem can be provided, which can help the user understand what went wrong and how to fix it.

I hope you consider this PR please, it would be my first contribution.

In general, it is better to rescue and display the specific error rather than showing a generic message in the view when working with Ruby. This is because a generic message may not provide enough information for the user to understand the problem and take steps to solve it.
@andyw8
Copy link
Contributor

andyw8 commented Mar 16, 2023

I agree with the premise, but I wonder if it's too general. In any language, it's usually wise to avoid directly exposing users to errors in the underlying framework.

@MaximilianoGarciaRoe
Copy link
Author

I agree with the premise, but I wonder if it's too general. In any language, it's usually wise to avoid directly exposing users to errors in the underlying framework.

Yes, but the explicit warning is not present in the code, StandarError. In the specific case of Rails, it would be good to have this warning explicitly included to promote this good practice.

@MaximilianoGarciaRoe
Copy link
Author

Hi @andyw8 Could you help me understand how you would suggest implementing this change? Would you suggest making the change in a different location, or perhaps adding more explanation to the proposal?"

@pirj
Copy link
Member

pirj commented Apr 5, 2023

What is wrong with the following approach?
What I'm trying to do is to take the focus off StandardError class itself.

begin
  raise StandardError.new("No access")
rescue => e
  @message = e.message
  render :error
end

There certainly are cases when advanced handling is needed, and there's even a whole Exceptional Ruby book about this.
But what is the problem for the basic case?

@MaximilianoGarciaRoe
Copy link
Author

MaximilianoGarciaRoe commented Apr 5, 2023

What is wrong with the following approach?
What I'm trying to do is to take the focus off StandardError class itself.

begin
  raise StandardError.new("No access")
rescue => e
  @message = e.message
  render :error
end

There certainly are cases when advanced handling is needed, and there's even a whole Exceptional Ruby book about this.
But what is the problem for the basic case?

In this particular case, the current implementation of the code is appropriate. However, in more complex applications or broader contexts, it is important to consider which parts of the code would benefit from specific error handling. That's why I suggested adding it to the Controllers, where vulnerabilities are more likely. What do you think? Do you agree with this proposal or do you have another suggestion for where to implement this change?

@pirj

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.

None yet

3 participants