heartcombo / simple_form

Forms made easy for Rails! It's tied to a simple DSL, with no opinion on markup.
http://blog.plataformatec.com.br/tag/simple_form
MIT License
8.21k stars 1.31k forks source link

The custom `error:` on all inputs does not work if there is an object present #1832

Closed matt-whiteley closed 2 months ago

matt-whiteley commented 6 months ago

f.input :title, error: "error" should set a custom error message, but the error is completely ignored unless f.object is nil, which is very unlikely for the vast majority of model backed forms.

The bug was introduced in this commit over 6 years ago https://github.com/heartcombo/simple_form/commit/09c928f1c0b20ec19b82c52840e24204e5ab4ff4 which was added to support errors working when there is no f.object

https://github.com/heartcombo/simple_form/blob/main/lib/simple_form/components/errors.rb#L13

# lib/simple_form/components/errors.rb#L13
def has_errors?
    object_with_errors? || object.nil? && has_custom_error?
end

The issue is the logic then never reaches has_custom_error? if f.object is present, because it either only looks in the object's errors, or fails the object.nil? check when you have an object.

I propose the fix is to just change this line to

object_with_errors? || has_custom_error?

as the custom error is used if it exists and the object.nil? check was not adding any actual benefit. This will find an error on the object if an object exists, or will fallback to the custom error regardless of object being present or not.

nashby commented 2 months ago

Hey @matt-whiteley! If we change it to

object_with_errors? || has_custom_error?

as you propose, it will show a custom error on every field without an error for regular AR models as well, and we don't want that. Could you please elaborate on your use case? What do you mean by model-backed forms? ActiveRecord models or your custom ones?

If you want to show an error even when the object has no errors, maybe you're looking for hints?

I'm closing this for now, but I'll reopen it if it makes sense to change the current behavior.

matt-whiteley commented 2 months ago

has_custom_error? only returns true if an error has been passed in to the input's options (e.g. f.input :title, error: "This field is wrong!"), so I don't see why it would show on all inputs?

By model backed I mean the common use case simple_form_for @user do |f| where there is now a value set for f.object.

The use case was some complex form with multi level accepts_nested_attributes_for which was difficult to get a decent error message via Rails model validations directly, so I wanted to use options[:error] and pass the message I wanted in directly.

f.input :deep_nested_field, error: calculate_complex_nested_error_for(f.object.deep_nested_field)

This option is mentioned in the docs but does not work at all currently if f.object is present.

I think you should be able to set a custom error on an input whenever you want via this option, no matter what kind of form you are building.

The previous change was not intended to break this functionality, but did so by accident while attempting to fix something else.

nashby commented 2 months ago

When I said it would show on all inputs I meant that if you have @user as regular AR model and it doesn't have any validation errors we won't show any error in this example below because we only show custom error if there's actual validation error (custom error is just a way to override default error message)

<%= simple_form_for @user do |f| %>
  <%= f.input :username, label: 'Your username please', error: 'Username is mandatory, please specify one' %>
  <%= f.button :submit %>
<% end %>

I see what you're trying to achieve in your example but it's not clear to me why you're providing this error calculate_complex_nested_error_for(f.object.deep_nested_field) to the input itself and not using validations? Your custom model could implement errors so it's compatible with SimpleForm