#4057 Make ActiveModel::Errors#add_on_blank and #add_on_empty accept an options hash and make various Validators pass their options - Ruby on Rails

Type	To find
responsible:me	tickets assigned to you
tagged:"@high"	tickets tagged @high
milestone:next	tickets in the upcoming milestone
state:invalid	tickets with the state invalid
created:"last week"	tickets created last week
sort:number, importance, updated	tickets sorted by #, importance or updated
Combine keywords for powerful searching.
Use advanced searching »

This project is archived and is in readonly mode.

#4057 ✓resolved

Make ActiveModel::Errors#add_on_blank and #add_on_empty accept an options hash and make various Validators pass their options

Reported by Sven Fuchs | February 25th, 2010 @ 08:00 PM | in 3.0.2

Current ActiveModel Validators do not pass their options hash to Errors#add. This makes it hard for plugin authors to add arbitrary options to, e.g. a call like validates_presence_of :format => "some format".

Also, there's still that confusion of :default vs :message options that never could be cleaned up due to BC concerns since 2.2.x

I've tried to clean these things up and relax Validator message generation by passing their (filtered) options hash (i.e. options like :allow_nil will not be passed to the errors collection)

Here's the commit on my fork:

http://github.com/svenfuchs/rails/commit/e63e207bc12f3903e51e9c412f...

I'll also attach the commit formatted as a patch.

0001-make-activemodel-errors-add_on_blank-and-add_on_empt.patch 44.2 KB

Comments and changes to this ticket

You flagged this item as spam.
Mateo Murphy March 6th, 2010 @ 05:41 PM
+1 This would definitely be helpful for plugin development
José Valim March 27th, 2010 @ 11:54 AM
- Assigned user changed from “Yehuda Katz (wycats)” to “José Valim”
Thanks Sven, good patch! However, I don't think it's necessary to filter the options. It adds extra complexity to the code and I cannot see any reason. Wdyt?
Yehuda Katz (wycats) March 29th, 2010 @ 01:54 AM
- State changed from “new” to “incomplete”
Dan Pickett May 15th, 2010 @ 01:57 AM
- Tag changed from “activemodel, messages, validation” to “activemodel, bugmash, messages, validation”
Can some bugmashers add some feedback as it relates to option filtering? If it's deemed unnecessary, can the patch be modified to remove the filtering?
You flagged this item as spam.
Jeroen van Dijk May 15th, 2010 @ 11:49 AM
This patch does not apply cleanly anymore. I will try to extract the parts that should apply and see if the filtering is needed.
You flagged this item as spam.
Jeroen van Dijk May 15th, 2010 @ 06:53 PM
I have adapted the patch so that it applies on master, some small cleanups here and there.

The filtering is currently needed, because the tests in i18n_generate_message_validation_test.rb have a very precise expectations of how methods are being called. Here is an example:
```
  def test_errors_add_on_empty_generates_message
    @person.errors.expects(:generate_message).with(:title, :empty, {})
    @person.errors.add_on_empty :title
  end
```
I would like to make that more flexible so the code can be more flexible. This code already gives a lot more flexibility.

I have ran all tests for activemodel and activerecord. I have added Sven Fuchs to the authors.
You flagged this item as spam.
Jeroen van Dijk May 15th, 2010 @ 06:53 PM
I forgot the patch...
- active_model_errors_on_blank.diff 43.4 KB
Rizwan Reza May 15th, 2010 @ 07:02 PM
- Tag changed from “activemodel, bugmash, messages, validation” to “activemodel, bugmash, bugmash-review, messages, validation”
- State changed from “incomplete” to “open”
+1 verified

All tests pass on master.

Here's the patch without whitespace errors. Preserves authorship.
- active_model_errors_on_blank.diff 43.4 KB
Repository May 15th, 2010 @ 07:18 PM
- State changed from “open” to “committed”
(from [bc1c8d58ec45593acba614d1d0fecb49adef08ff]) Make ActiveModel::Errors#add_on_blank and #add_on_empty accept an options hash and make various Validators pass their (filtered) options.

This makes it possible to pass additional options through Validators to message
generation. E.g. plugin authors want to add validates_presence_of :foo, :format
=> "some format".

Also, cleanup the :default vs :message options confusion in ActiveModel
validation message generation.

Also, deprecate ActiveModel::Errors#add_on_blank(attributes, custom_message) in
favor of ActiveModel::Errors#add_on_blank(attributes, options).

Original patch by Sven Fuchs, some minor changes and has been changed to be applicable to master again

[#4057 state:committed]

Signed-off-by: Jeremy Kemper jeremy@bitsweat.net
http://github.com/rails/rails/commit/bc1c8d58ec45593acba614d1d0fecb...
Rizwan Reza May 15th, 2010 @ 07:43 PM
- Tag changed from “activemodel, bugmash, bugmash-review, messages, validation” to “activemodel, messages, validation”
José Valim May 15th, 2010 @ 08:58 PM
- State changed from “committed” to “incomplete”
Sorry, but I had to revert. Please check the comments in d6cbb27e7b260c970bf7 ( http://github.com/rails/rails/commit/d6cbb27e7b260c970bf7d07dc0b059... ).
José Valim May 15th, 2010 @ 09:00 PM
- Tag changed from “activemodel, messages, validation” to “activemodel, bugmash, messages, validation”
You flagged this item as spam.
Jeroen van Dijk May 16th, 2010 @ 03:43 PM
The goal of the commit was to make the validations more flexible. The whitelist approach was an intermediate solution to satisfy the strict tests. My plan was to see how I could make the tests more flexible to make the code more flexible and have the whitelist removed. I thought that would be easier when the whitelist was in one spot.

Anyhow, I think I know how to approach this problem better now. I'll work on a new patch.
Rizwan Reza May 16th, 2010 @ 03:45 PM
- Tag changed from “activemodel, bugmash, messages, validation” to “activemodel, messages, validation”
- State changed from “incomplete” to “wontfix”
Rizwan Reza May 16th, 2010 @ 03:46 PM
Jeroen, open the ticket when you attach the fix.
Rizwan Reza May 16th, 2010 @ 03:55 PM
- State changed from “wontfix” to “incomplete”
You flagged this item as spam.
Jeroen van Dijk May 16th, 2010 @ 09:23 PM
- Tag changed from “activemodel, messages, validation” to “activemodel, bugmash-review, messages, validation”
Here is my latest patch. I have done a lot of refactoring in the tests to make sure things aren't repeated all over the place.

Each validators cleans it on own options now. The Errors object only filters :if, :unless and :on.
- make_validations_pass_options.diff 63.7 KB
José Valim May 16th, 2010 @ 09:57 PM
- State changed from “incomplete” to “open”
Rizwan Reza May 19th, 2010 @ 11:59 AM
- no changes were found...
You flagged this item as spam.
Simon Jefford May 19th, 2010 @ 03:28 PM
+1 Patch applies and tests run.

I have, however, taken the liberty of removing whitespace errors - patch preserves authorship.
- make_validations_pass_options_wsfix.diff 64.1 KB
José Valim June 7th, 2010 @ 09:44 AM
Sorry, but patch does not apply anymore. Would anyone mind to rebase? Also, there is a TODO: in the tests, would be nice if we could apply the tests without the TODO.
Repository June 21st, 2010 @ 11:39 AM
- State changed from “open” to “resolved”
(from [0421fb7a913c1c8a7e07a395106bbc65e75e9d45]) Refactor previous commit a bit [#4057 state:resolved] http://github.com/rails/rails/commit/0421fb7a913c1c8a7e07a395106bbc...
Jeremy Kemper October 15th, 2010 @ 11:01 PM
- Milestone set to “3.0.2”
- Importance changed from “” to “Medium”
[bulk edit]