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

Undefined method 'has_attribute?' from simple form #145

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@bookwyrm

bookwyrm commented Sep 29, 2014

SimpleForm expects a model to respond to has_attribute? while building a form in the view. I just updated from Reform v1.0.4 to v1.1.1 and SimpleForm started throwing and undefined method error.

This PR adds the behavior back in to appease SimpleForm.

bookwyrm added some commits Sep 29, 2014

Delegate has_attribute? to model for simple_form
SimpleForm uses has_attribute? to build form fields in the view. At some point between Reform v1.0.4 and v1.1.1 the form object stopped responding to has_attribute? and SimpleForm now throws an exception.
This adds the delegation back in.
@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Sep 29, 2014

Collaborator

Nice, @bookwyrm thanks!

This endless adding-arbitrary-methods-to-ActiveModel has to end! We should encourage simple_form to define an interface what is required from a "model", otherwise we'll never stop adding these bullshit methods to Form::ActiveModel (which is already full of weird delegations).

Collaborator

apotonick commented Sep 29, 2014

Nice, @bookwyrm thanks!

This endless adding-arbitrary-methods-to-ActiveModel has to end! We should encourage simple_form to define an interface what is required from a "model", otherwise we'll never stop adding these bullshit methods to Form::ActiveModel (which is already full of weird delegations).

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Sep 29, 2014

Collaborator

Other than that, I love simple_form ❤️ !!!

Collaborator

apotonick commented Sep 29, 2014

Other than that, I love simple_form ❤️ !!!

@bookwyrm

This comment has been minimized.

Show comment
Hide comment
@bookwyrm

bookwyrm Sep 30, 2014

Yeah, simple_form is great. It was a bummer when the reform update broke it. FYI - the tests all pass against the v1.1.1 code - it's only master where they fail.

bookwyrm commented Sep 30, 2014

Yeah, simple_form is great. It was a bummer when the reform update broke it. FYI - the tests all pass against the v1.1.1 code - it's only master where they fail.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca Sep 30, 2014

Good point. Right now Simple Form is highly coupled to Active Record, not to Active Model.

We have a wish to support more database adapters and to make so we need to define an API that can be implemented in the adapters.

This is a really old wish, it precedes the date I started to contribute with Simple Form 4 years ago. Maybe it is time to define it.

rafaelfranca commented Sep 30, 2014

Good point. Right now Simple Form is highly coupled to Active Record, not to Active Model.

We have a wish to support more database adapters and to make so we need to define an API that can be implemented in the adapters.

This is a really old wish, it precedes the date I started to contribute with Simple Form 4 years ago. Maybe it is time to define it.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Sep 30, 2014

Collaborator

@rafaelfranca It will be easier to define this list with a 3rd-party extension like Reform (the same with Cells and ActionView, BTW). We need a reflection API that can be implemented by all ORM projects, then we're good to go.

@bookwyrm Ok, good to know. I will check out what I changed. Too many beers in the meantime to remember what I did... 🍻

Collaborator

apotonick commented Sep 30, 2014

@rafaelfranca It will be easier to define this list with a 3rd-party extension like Reform (the same with Cells and ActionView, BTW). We need a reflection API that can be implemented by all ORM projects, then we're good to go.

@bookwyrm Ok, good to know. I will check out what I changed. Too many beers in the meantime to remember what I did... 🍻

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Sep 30, 2014

Collaborator

BTW, @rafaelfranca The same goes out to validators, they must use the same reflection API. It's such a pain in the ass ATM to support all validators in Reform.

Collaborator

apotonick commented Sep 30, 2014

BTW, @rafaelfranca The same goes out to validators, they must use the same reflection API. It's such a pain in the ass ATM to support all validators in Reform.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Sep 30, 2014

Collaborator

@bookwyrm Are you saying it broke after 1.1.1? Please paste your simple_form view and form class here.

Also, we need to include functional tests with simple_form. That will help working out this reflection API with @rafaelfranca.

Collaborator

apotonick commented Sep 30, 2014

@bookwyrm Are you saying it broke after 1.1.1? Please paste your simple_form view and form class here.

Also, we need to include functional tests with simple_form. That will help working out this reflection API with @rafaelfranca.

@bookwyrm

This comment has been minimized.

Show comment
Hide comment
@bookwyrm

bookwyrm Sep 30, 2014

@apotonick I started getting the "Undefined method 'has_attribute?'" error when I upgraded Reform from v1.0.4 to v1.1.1 (I skipped v1.1.0 so I'm not sure what the behavior is there). When I first tried to fix the issue, I simply created a feature branch from master, but the test suite was failing when I ran it (I think this was against 99a8875). When I saw the test failing, I changed my branch to be based on the https://github.com/apotonick/reform/tree/v1.1.1 tag so that I could have passing tests before I started my additions.

Regarding functional tests with simple_form, I considered that but I wasn't sure if you wanted to introduce simple_form as a dev dependency. I can work on that in the coming weeks. Unfortunately, I can't paste my code as it's for a private project but I can work up a failing integration test.

bookwyrm commented Sep 30, 2014

@apotonick I started getting the "Undefined method 'has_attribute?'" error when I upgraded Reform from v1.0.4 to v1.1.1 (I skipped v1.1.0 so I'm not sure what the behavior is there). When I first tried to fix the issue, I simply created a feature branch from master, but the test suite was failing when I ran it (I think this was against 99a8875). When I saw the test failing, I changed my branch to be based on the https://github.com/apotonick/reform/tree/v1.1.1 tag so that I could have passing tests before I started my additions.

Regarding functional tests with simple_form, I considered that but I wasn't sure if you wanted to introduce simple_form as a dev dependency. I can work on that in the coming weeks. Unfortunately, I can't paste my code as it's for a private project but I can work up a failing integration test.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Sep 30, 2014

Collaborator

@bookwyrm This is weird, as no AM/AR code was changed or added from 1.1.1 till master?! Please, isolate the simple_form view and the form to the minimal code to provoke this error, so I can investigate.

Collaborator

apotonick commented Sep 30, 2014

@bookwyrm This is weird, as no AM/AR code was changed or added from 1.1.1 till master?! Please, isolate the simple_form view and the form to the minimal code to provoke this error, so I can investigate.

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Oct 1, 2014

Collaborator

What simple_form version is that, @bookwyrm ?

Collaborator

apotonick commented Oct 1, 2014

What simple_form version is that, @bookwyrm ?

@bookwyrm

This comment has been minimized.

Show comment
Hide comment
@bookwyrm

bookwyrm Oct 2, 2014

@apotonick, When I first started getting the error, I was using https://github.com/plataformatec/simple_form/tree/v3.1.0.rc1 - after I patched reform and was using my branch from GH (https://github.com/bookwyrm/reform/tree/1.1.1.simpleformfix) I updated to https://github.com/plataformatec/simple_form/tree/v3.1.0.rc2 and everything continued to work as expected.

Regarding failing tests, my master branch (bookwyrm@99a8875) currently includes an AM/AR failure.

  1) Failure:
test_0004_has errors on title when title is taken for the same artist and album(ActiveRecordTest) [/Users/matt/Projects/reform/test/active_record_test.rb:57]:
Expected [] to not be empty.

When I saw this, I based my feature branch on the v1.1.1 tag (https://github.com/bookwyrm/reform/tree/v1.1.1) because that had no test failures and I figured was safer to develop against.

I can see about setting up some simple_form tests but it probably won't be for a couple weeks. Is there a particular way you would like me to structure the tests (apart from putting them in test/rails/)?

bookwyrm commented Oct 2, 2014

@apotonick, When I first started getting the error, I was using https://github.com/plataformatec/simple_form/tree/v3.1.0.rc1 - after I patched reform and was using my branch from GH (https://github.com/bookwyrm/reform/tree/1.1.1.simpleformfix) I updated to https://github.com/plataformatec/simple_form/tree/v3.1.0.rc2 and everything continued to work as expected.

Regarding failing tests, my master branch (bookwyrm@99a8875) currently includes an AM/AR failure.

  1) Failure:
test_0004_has errors on title when title is taken for the same artist and album(ActiveRecordTest) [/Users/matt/Projects/reform/test/active_record_test.rb:57]:
Expected [] to not be empty.

When I saw this, I based my feature branch on the v1.1.1 tag (https://github.com/bookwyrm/reform/tree/v1.1.1) because that had no test failures and I figured was safer to develop against.

I can see about setting up some simple_form tests but it probably won't be for a couple weeks. Is there a particular way you would like me to structure the tests (apart from putting them in test/rails/)?

@apotonick

This comment has been minimized.

Show comment
Hide comment
@apotonick

apotonick Oct 2, 2014

Collaborator

Thanks so much, @bookwyrm.

The currently failing test is a deliberate error to that we need to write our own uniqueness validator instead of using the AR one which is too coupled to ActiveRecord's extra-magic API.

If it takes a couple of weeks I'll probably start doing it, but you know what would be fantastic? Once I set up tests, you could quickly add some quirky simple_form calls in the view since you most probably know its API better than me (e.g. using association and so on). Could you do that? I'll ping ya!

Collaborator

apotonick commented Oct 2, 2014

Thanks so much, @bookwyrm.

The currently failing test is a deliberate error to that we need to write our own uniqueness validator instead of using the AR one which is too coupled to ActiveRecord's extra-magic API.

If it takes a couple of weeks I'll probably start doing it, but you know what would be fantastic? Once I set up tests, you could quickly add some quirky simple_form calls in the view since you most probably know its API better than me (e.g. using association and so on). Could you do that? I'll ping ya!

@bookwyrm

This comment has been minimized.

Show comment
Hide comment
@bookwyrm

bookwyrm Oct 6, 2014

@apotonick, sure, once you get some tests setup, I can add some simple_form calls.

bookwyrm commented Oct 6, 2014

@apotonick, sure, once you get some tests setup, I can add some simple_form calls.

@fran-worley

This comment has been minimized.

Show comment
Hide comment
@fran-worley

fran-worley Jul 5, 2016

Collaborator

Closing in favour of #176

Collaborator

fran-worley commented Jul 5, 2016

Closing in favour of #176

@fran-worley fran-worley closed this Jul 5, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment