Check for Blank Record in form_for #7314

Merged
merged 1 commit into from Aug 11, 2012

Conversation

Projects
None yet
5 participants
Member

schneems commented Aug 10, 2012

If nil or an empty array is passed into form_for you get a horrible error message, this one is much more indicative of what the programmer needs to know to fix the problem.

ATP in actionpack

Screenshot goodness:

Owner

rafaelfranca commented Aug 10, 2012

@schneems seems good. Could you add tests to this feature?

Member

schneems commented Aug 10, 2012

I added tests, also discovered the first implementation wouldn't catch the [nil, nil] case. Let me know if you have any other problems/questions.

Owner

rafaelfranca commented Aug 10, 2012

I think the message is not good. Because in the array case [:foo, nil] should brake too, and this is not empty. Maybe "Form object in cannot be nil"

Member

steveklabnik commented Aug 10, 2012

"cannot contain nil"? [:foo, nil] is not nil either.

Owner

rafaelfranca commented Aug 10, 2012

But this is not the Form object. When you pass an Array for the form helper the object is the last element of the array.

Member

steveklabnik commented Aug 10, 2012

Hmmmm.

Whatever, I want the error message to be blue. :p

Member

schneems commented Aug 10, 2012

How about:

First argument in form cannot contain nil or be empty

It is technically correct for nil, [nil], and [:foo, nil]

Owner

rafaelfranca commented Aug 10, 2012

Seems better.

actionpack/test/template/form_helper_test.rb
+ end
+ end
+
+ assert_match /cannot be nil or empty/, error.message
@carlosantoniodasilva

carlosantoniodasilva Aug 10, 2012

Owner

I think the blank line should be swapped - assertion should be right below assert_raises block).

Seems a nice improvement 👍

Member

schneems commented Aug 11, 2012

Changed text, fixed whitespace.

@schneems thanks, can you squash? Github shows me two commits (though both have the same desc).

actionpack/test/template/form_helper_test.rb
+ form_for(nil, :html => { :id => 'create-post' }) do
+ end
+ end
+ assert_match /cannot be nil or empty/, error.message
@rafaelfranca

rafaelfranca Aug 11, 2012

Owner

I think these tests will break. The message is different. Can we assert all the message?

Member

schneems commented Aug 11, 2012

Squashed, fixed tests. ATP

check for nil or empty record in form_for
if nil or an empty array is passed into form_for you get a horrible error message, this one is much more indicative of what the programmer needs to know to fix the problem.

rafaelfranca added a commit that referenced this pull request Aug 11, 2012

@rafaelfranca rafaelfranca merged commit a6e0d8c into rails:master Aug 11, 2012

@rafaelfranca rafaelfranca merged commit 60b650b into rails:master Aug 11, 2012

rafaelfranca added a commit that referenced this pull request Aug 11, 2012

This change means that I can't use an ActiveRecord model that has a "blank" boolean attribute (if the attribute is true). A small consequence for an equally small gain.

Owner

rafaelfranca commented Sep 27, 2012

@thefool808 I din't get. I think we are checking only if the object is blank and not if the attribute of the object is blank.

rails generate model Foo blank:boolean

form_for(Foo.new(:blank => true)) => ArgumentError, "First argument in form cannot contain nil or be empty"

This patch only checks whether blank? returns true on the object, not whether or not it is suitable for form_for. Not that important, but it crossed my mind when reading schneems blog post about this patch. Food for thought. Thanks for the reply.

Owner

rafaelfranca commented Sep 27, 2012

lol. I got the error. Yeah, maybe is better to not check blank? and only check:

raise ArgumentError, "First argument in form cannot contain nil or be empty" unless object

I think you meant nil?. That's actually a really good solution, as a blank array will already be nil by that point ([].last) and a blank string will never get there. I'm just being nitpick-y for no good reason... Thanks for listening :)

Owner

rafaelfranca commented Sep 27, 2012

Oops. I forgot to change the copied code. Now it is right

That's cool too.

Member

schneems commented Sep 27, 2012

@thefool808 theoretically one could over-ride the nil? or is_a? or any other method on an object, but that doesn't make it a good idea. If someone makes a column named blank or present in their database they will have more issues than just in this one method. In this situation i believe you can pass non AR objects such as an empty string "". That would not get caught by a simple nil check

We need to support this use case too:

<%= form_for :person do |f| %>
Owner

rafaelfranca commented Sep 27, 2012

I think checking for the empty string is too defensive. I don't see why someone in the earth want to pass a string to a form_for call. We never encourage this in the documentation.

Member

schneems commented Sep 27, 2012

Agreed, was just being paranoid. Both seem to catch the majority of invalid use-cases.

A string or symbol will miss the safety check anyways. Also, I agree having a "blank" boolean attribute on a model is a terrible idea that maybe should never be considered, however I think overriding nil? is anti-ruby (i.e. most ruby developers will know not to do this, or at least understand the consequences, even if they've never used rails) whereas blank? is a rails construct.

Just checking for nil? will cover the most common case where an uninstantiated object being passed to form_for. Checking blank? does the same thing but has this tiny little (probably stupid to even point out) consequence where you could have a perfectly suitable object for form_for that gets rejected for no good reason.

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