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

5.3.0 - Custom Inputs Fail to Load #1824

Closed BeatyThomas closed 3 months ago

BeatyThomas commented 10 months ago

Environment

Current behavior

This commit is a breaking change that prevents custom inputs in app/input from being loaded.

Expected behavior

Adding the removed snippit returns normal functionality.

lmrodriguezr commented 9 months ago

I seem to be experiencing this issue, but only in development, not in production. I have looked everywhere for possible configurations in the environments that could account for the difference but I can't find it anywhere. Any ideas?

mark-dce commented 8 months ago

I'm having the issue in both development and test environments. It appears to be related to how Rails lazy-loads classes in those environments vs. eager loading in production. My current quick-and-dirty work-around is to reference the custom input class names in an initializer, e.g.:

config/iniitilizers/simple_form.rb

# ...
SimpleForm.setup do |config|
    # ...setup stuff here..
end

# Added at the bottom of the file
# Explicitly reference the custom input classes
CurrencyInput
CustomInput
MultiValueInput
# End of File

Alternatively, it also works to locally override the SimpleForm::FormBuilder#attempt_mapping method with the code from v5.2.0: https://github.com/heartcombo/simple_form/blob/485175830ce36a9b5b431f3ad16730a2a5211c7b/lib/simple_form/form_builder.rb#L673-L681

This works because it always attempts to load the class and raises an error if it's not present.

As opposed to the v5.3.0 version which never executes because the lazy loaded class constant isn't loaded, and therefore isn't defined yet: https://github.com/heartcombo/simple_form/blob/8f77f598b32ed11b0d56c44481607a6e911bcecb/lib/simple_form/form_builder.rb#L673-L677

dwkoogt commented 5 months ago

I use a custom form builder that has custom fields defined in it and this change breaks it too. It seems the change will pass the specs but you won't see it break unless you actually have custom inputs or custom form builder in your dev environment.

The intention of the calling method find_mapping attempts to map a field name to an input class and it usually finds it before getting to the attempt_mapping method. The attempt_mapping method is reached when you defined some custom field hence you have to have a custom input class or have it defined in a custom form builder. These are loaded lazily in dev so the code change that broke it made the method not do what it was supposed to do.

So, for general input types attempt_mapping not being called.

If you do not want this feature, you'd simply set inputs_discovery option to false. You can also create a custom builder and map custom input classes using map_type class method. Or there may a configuration option to map as well such that attempt_mapping is not called (not sure this exists).

The try catch is used because it is attempting to find a class as the method name implies.

If the argument for the change is performance, you'd only see little improvement unless you have a ton of custom inputs. So, to me the change is not really worthwhile and should be reverted.

carlosantoniodasilva commented 3 months ago

Released v5.3.1 reverting that and trying a slightly different approach to error checking.