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

support nil as model for reform #11

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

Conversation

pvmeerbe
Copy link

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
Copy link
Author

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>'

@mensfeld
Copy link

👍

@apotonick
Copy link
Member

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. 😜

@pvmeerbe
Copy link
Author

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.

@apotonick
Copy link
Member

Oh interesting, I did not see that discussion! 👍

@pvmeerbe
Copy link
Author

pvmeerbe commented Aug 3, 2016

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

@fran-worley
Copy link
Contributor

Closing and reopening to trigger travis build

@fran-worley fran-worley reopened this Sep 26, 2016
@timoschilling
Copy link

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

@fran-worley
Copy link
Contributor

@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.

@timoschilling
Copy link

ah ok, never mind

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
    )
 ...
@fran-worley
Copy link
Contributor

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 ?

@pvmeerbe
Copy link
Author

pvmeerbe commented Mar 1, 2017

@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...

@adis-io
Copy link
Contributor

adis-io commented Mar 1, 2017

Approach I'm using:

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

  def persisted?
    false
  end
end

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.

6 participants