reform-rails icon indicating copy to clipboard operation
reform-rails copied to clipboard

support nil as model for reform

Open pvmeerbe opened this issue 8 years ago • 13 comments

In some cases reform is just used for coercion & validation without any persistence done. In these cases it was advised to use nil as reform model & to mark all properties as virtual.

this works fine, however fails as .persisted? is called on nil upon rendering in actionpack-3.2.22.2/lib/action_view/helpers/form_helper.rb:388

...
  as = options[:as]
    action, method = object.respond_to?(:persisted?) && object.persisted? ? [:edit, :put] : [:new, :post]
    options[:html].reverse_merge!(
      :class  => as ? "#{action}_#{as}" : dom_class(object, action),
      :id     => as ? "#{action}_#{as}" : [options[:namespace], dom_id(object, action)].compact.join("_").presence,
      :method => method
    )
 ...

pvmeerbe avatar Jul 15 '16 13:07 pvmeerbe

got two tests failing but they also fail before my change:

$ bundle exec ruby -Ilib:test test/model_reflections_test.rb 
 config.eager_load is set to nil. Please update your config/environments/*.rb files accordingly:

  * development - set it to false
  * test - set it to false (unless you use a tool that preloads your test environment)
  * production - set it to true

   DEPRECATION WARNING: Using a dynamic :controller segment in a route is deprecated and will be removed in Rails 5.1. (called from block in <top (required)> at  reform_rails/test/rails-app/config/routes.rb:2)
   DEPRECATION WARNING: Using a dynamic :action segment in a route is deprecated and will be removed in Rails 5.1. (called from block in <top (required)> at reform_rails/test/rails-app/config/routes.rb:2)
   test/model_reflections_test.rb:6: warning: toplevel constant ActiveRecord referenced by    Reform::Form::ActiveRecord
   test/model_reflections_test.rb:7:in `<class:SongForm>': uninitialized constant  Reform::Form::ActiveModel::ModelReflections (NameError)
   from test/model_reflections_test.rb:5:in `<class:ModelReflectionTest>'
   from test/model_reflections_test.rb:4:in `<main>'

and

$ bundle exec ruby -Ilib:test test/mongoid_test.rb 
 config.eager_load is set to nil. Please update your config/environments/*.rb files accordingly:

   * development - set it to false
   * test - set it to false (unless you use a tool that preloads your test environment)
   * production - set it to true

 DEPRECATION WARNING: Using a dynamic :controller segment in a route is deprecated and will be removed in Rails 5.1. (called from block in <top (required)> at /reform_rails/test/rails-app/config/routes.rb:2)
 DEPRECATION WARNING: Using a dynamic :action segment in a route is deprecated and will be removed in Rails 5.1. (called from block in <top (required)> at /reform_rails/test/rails-app/config/routes.rb:2)
 /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:293:in `require': /.rvm/gems/ruby-2.2.3@reform_rails/gems/mongoid-1.0.6/lib/mongoid/associations/has_many.rb:79: syntax error, unexpected keyword_do_cond, expecting ':' (SyntaxError)
         @documents = attributes ? attributes.collect do |attrs|
                                                   ^
 /.rvm/gems/ruby-2.2.3@reform_rails/gems/mongoid-1.0.6/lib/mongoid/associations/has_many.rb:84: syntax error, unexpected ':', expecting keyword_end
    end : []
          ^
 /.rvm/gems/ruby-2.2.3@reform_rails/gems/mongoid-1.0.6/lib/mongoid/associations/has_many.rb:99: syntax error, unexpected keyword_do_cond, expecting keyword_end
    attributes.values.each do |attrs|
                             ^
 /.rvm/gems/ruby-2.2.3@reform_rails/gems/mongoid-1.0.6/lib/mongoid/associations/has_many.rb:139: syntax error, unexpected keyword_end, expecting end-of-input
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:293:in `block in require'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:259:in `load_dependency'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:293:in `require'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/mongoid-1.0.6/lib/mongoid/associations.rb:5:in `<top (required)>'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:293:in `require'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:293:in `block in require'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:259:in `load_dependency'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:293:in `require'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/mongoid-1.0.6/lib/mongoid.rb:40:in `<top (required)>'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:293:in `require'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:293:in `block in require'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:259:in `load_dependency'
from /.rvm/gems/ruby-2.2.3@reform_rails/gems/activesupport-5.0.0/lib/active_support/dependencies.rb:293:in `require'
from test/mongoid_test.rb:3:in `mongoid_present?'
from test/mongoid_test.rb:12:in `<main>'

pvmeerbe avatar Jul 15 '16 13:07 pvmeerbe

:+1:

mensfeld avatar Jul 18 '16 11:07 mensfeld

I would rather implement that as a module and then make people include it, e.g. Modelless or something. That way, we can save all the ifs because I don't like ifs in particular. :stuck_out_tongue_winking_eye:

apotonick avatar Jul 18 '16 13:07 apotonick

Make sense ;). However have you seen the related discussion on gitter :

  Ievgen Kuzminov @iJackUA jul. 15 11:50 
  @pvmeerbe @apotonick I would agree that there is a confusion when you try to have   
  form/contract without a Model object. Maybe there should be an additional “feature”/mixin like 
  NotPersisted to prevent form to syncronisation with underlying model ? That is also helpful with 
  Operations that do not persist anything, but just calculate smth. or do other thing not directly 
  related to Model (but still require input params handling and validations).

I think it make sense to also add such a feature @ reform level directly, in which case the new reform-rails Modelless module could be called automatically.

pvmeerbe avatar Jul 18 '16 14:07 pvmeerbe

Oh interesting, I did not see that discussion! :+1:

apotonick avatar Jul 18 '16 14:07 apotonick

changed it to a separate module. Is this what you had in mind?

I didn't completely split it from the active_model code as I didn't dive into the meaning/pupose of all the code in there, so you have to include both ActiveModel & NotPersisted (see related test).

I guess it could be possible to split it more when using dry-validation, however the used Form builder could still request several active_model API calls...

Perhaps a feature 'NotPersisted' can be added to reform which will automatically

  • include this new module
  • mark all fields as virtual

pvmeerbe avatar Aug 03 '16 09:08 pvmeerbe

Closing and reopening to trigger travis build

fran-worley avatar Sep 26 '16 13:09 fran-worley

@fran-worley as a Repo Member you can start a rebuild via the Travis UI build__14_-trailblazer_reform-rails-_travis_ci

timoschilling avatar Sep 26 '16 14:09 timoschilling

@timoschilling that only works if a build existed in the first place. I setup travis after these PRs were open and this seemed to be the only way to trigger a build.

fran-worley avatar Sep 26 '16 14:09 fran-worley

ah ok, never mind

timoschilling avatar Sep 26 '16 14:09 timoschilling

What model/ class are you providing the form on initialisation? I'd like to find a more elegant way of doing this if possible, but would need to know a little more about how you would intend to use it.

Can you provide an example form ?

fran-worley avatar Feb 21 '17 22:02 fran-worley

@fran-worley The goal was to use the validation and coercion logic from Reform, but not the persistence functionality. Perhaps in addition a representer could be inferred from the Reform contract

The cases I encountered:

  • a form on a customer platform which required to execute some business logic/actions + some REST calls to other internal systems not available to the end customer. No real model is available in the Rails app
  • an incoming REST api call requiring some business actions + the result is created by using a Roar representer

The last case is now solved using dry-validation instead of a Reform contract. The first case is currently solved using an OpenStruct as model.

Example of Reform of first case : http://pastebin.com/Fjz3SLwq. This form is feeded with OpenStruct.new as model. The form result is used to launch several REST api calls to an internal system. Each call produces a result object, which is forwarded to the user and not stored locally. I tried to implement this with dry-validation, but encountered problems with mapping possible dry errors on the form. I forgot the exact details...

pvmeerbe avatar Mar 01 '17 13:03 pvmeerbe

Approach I'm using:

class BaseForm < Reform::Form
  def initialize
    super(nil)
  end

  def persisted?
    false
  end
end

adis-io avatar Mar 01 '17 14:03 adis-io