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

3.5.1 breaks simple_fields_for for custom form objects #1549

Closed
lacco opened this issue Feb 19, 2018 · 12 comments
Closed

3.5.1 breaks simple_fields_for for custom form objects #1549

lacco opened this issue Feb 19, 2018 · 12 comments

Comments

@lacco
Copy link

lacco commented Feb 19, 2018

Environment

  • Ruby 2.5.0
  • Rails 5.1.4
  • Simple Form 3.5.1

Current/ expected behavior

Prior to 3.5.1, I was able to build my own form objects using http://dry-rb.org/gems/dry-types/ . It is looking something like

class CategoryForm < DryTypes::Model
  attribute :names, DryTypes::Strict::Array.member(DryTypes::Translation)
end

and

  = simple_form_for category_form do |f|
    = f.simple_fields_for :names do |name_fields|
      - name = name_fields.object
      = name_fields.input :value, label: "Name (#{name.language.name})"

After the update, name_fields.object returns nil and breaks my code. Does somebody have a hint which change could have caused the issue?

@hosiawak
Copy link

Probably because object gets converted into to_model in 3.5.1 My SimpleDelegator decorators are broken too in 3.5.1 so freezing to 3.5.0

@krisleech
Copy link

krisleech commented Feb 19, 2018

It seems we have a similar problem, if we have a wrapped model (presenter using SimpleDelegator) then object.form returns the model, not the wrapped model.

StudyAssignmentPresenter.new(StudyAssignment.new).class # => StudyAssignmentPresenter
StudyAssignmentPresenter.new(StudyAssignment.new).to_model.class # => StudyAssignment

You can hack around this as such:

class StudyAssignmentPresenter < SimpleDelegator
  def to_model; self; end
end

@krisleech
Copy link

The PR introducing the call to to_model is here: #1256

@lacco
Copy link
Author

lacco commented Feb 22, 2018

I can confirm that reverting the change from #1256 fixes the problem for me!

@krisleech
Copy link

@lacco does adding def to_model; self; end to CategoryForm work? You could also consider include ActiveModel::Conversion.

/cc @timurvafin

@lacco
Copy link
Author

lacco commented Feb 22, 2018

Actually I am already providing to_model to make use of Rails automatisms to guess URL, HTTP method, ... : def to_model; category; end. I am sure that def to_model; self; end should solve my problems with simple_form, but I would rewrite the other code.

@feliperenan
Copy link
Collaborator

feliperenan commented Feb 26, 2018

You are right, now from version 3.5.1 we have to implement #to_model if we are using a PORO or Object that doesn't respond to it. BTW, it is the same behavior of form_for.

This information should be included in the releases notes, sorry about that 😥 . I will add some documentation about it.

Thank you folks.

@feliperenan
Copy link
Collaborator

@lacco just for curiosity. Does DryTypes::Model implements #to_model and other Active Model methods or you are expliciting it in the form?

@lacco
Copy link
Author

lacco commented Feb 28, 2018

@felipegimenesptec You are correct, my DryTypes::Model implementation is aware of ActiveModel features, the implementation looks roughly like this:

class DryTypes::Model < Dry::Struct
  include ActiveModel::Conversion
  include ActiveModel::Validations
  extend ActiveModel::Naming

  # This is actually overwritten to def to_model; category; end in my case
  def to_model
    self
  end

  def persisted?
    to_model == self ? false : to_model.persisted?
  end
end

@jrochkind
Copy link

I don't understand why this issue is closed, can you provide any context? Apps that break in a patch-level upgrade from 3.5.0 to 3.5.1 seems like it must be a bug? I would consider it a bug for patch-level release to ever include breaking changes...

@feliperenan
Copy link
Collaborator

I consider it a bug fix since Simple Form should work as a Rails Form like form_for do and it wasn't.

@krisleech
Copy link

I guess it isn't a breaking change since it doesn't break any features described in the README. The fact it supported non-ActiveModel compliant objects was an unintentional feature and strictly speaking a bug since that meant it did not have parity with form_for, which is what the patch fixed.

It is just a bit unfortunate.

no-reply pushed a commit to samvera/hyrax that referenced this issue Mar 2, 2018
We previously supported `~> 3.2`. The regression introduced in v3.5.1 is
described at heartcombo/simple_form#1549. We'll
need to fix our presenters to be `ActiveModel` compliant to make `simple_form`
work correctly past that version.

In the meanwhile, we shouldn't require any simple_form upgrade as part of an
upcoming Hyrax release.
no-reply pushed a commit to samvera/hyrax that referenced this issue Mar 2, 2018
We previously supported `~> 3.2`. The regression introduced in v3.5.1 is
described at heartcombo/simple_form#1549. We'll
need to fix our presenters to be `ActiveModel` compliant to make `simple_form`
work correctly past that version.

In the meanwhile, we shouldn't require any simple_form upgrade as part of an
upcoming Hyrax release.
no-reply pushed a commit to samvera/hydra-editor that referenced this issue Mar 2, 2018
Beginning with `v3.5.1`, Simple Form now requires models passed to it to conform
to the `ActiveModel` interface; particularly, they must implement
`#to_model`. We will restrict to `<= 3.5.0` to prevent unexpected breakage for
downstream apps.

Support for `v3.5.1` and up should be readded (this may just require
testing/documentation fixes) and a major version released to get us back to
tracking current `simple_form` development.

See also: samvera/hyrax#2758 and
heartcombo/simple_form#1549.
no-reply pushed a commit to samvera/hydra-editor that referenced this issue Mar 5, 2018
Beginning with `v3.5.1`, Simple Form now requires models passed to it to conform
to the `ActiveModel` interface; particularly, they must implement
`#to_model`. We will restrict to `<= 3.5.0` to prevent unexpected breakage for
downstream apps.

Support for `v3.5.1` and up should be readded (this may just require
testing/documentation fixes) and a major version released to get us back to
tracking current `simple_form` development.

See also: samvera/hyrax#2758 and
heartcombo/simple_form#1549.
badlamer pushed a commit to foxford/actionform that referenced this issue Dec 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

5 participants