adzap / validates_timeliness

Date and time validation plugin for ActiveModel and Rails. Supports multiple ORMs and allows custom date/time formats.
MIT License
1.59k stars 227 forks source link

Fix a wrong monkey patch on overriding ActiveModel::Type::Date #179

Closed jasl closed 6 years ago

jasl commented 6 years ago
ActiveModel::Type::Date.class_eval do
  # Module.new do |m|
    def cast_value(value)
      # the `super` is point to `Type::Value#cast_value`, not Type::Date's
      return super unless ValidatesTimeliness.use_plugin_parser

      # ....
    end
  # end.tap { |mod| include mod }
end

In addition, sqlite3-ruby is no use now, so I remove it.

adzap commented 6 years ago

Ah yes thanks for this. You can see I had attempted a module override to preserve super but forgot to change after using class_eval.

adzap commented 6 years ago

I think I will review to see if I can it get working with the module to avoid aliasing methods. If not I will use your PR.

jasl commented 6 years ago

I think just alias the origin one is enough, or you may need to using prepend

adzap commented 6 years ago

Yes we can alias but I'd like to avoid that. I have removed as many aliases as I can from the plugin.

I think the module will work in Date but it didn't work in Time and DateTime due to some funky delegation AR uses to inject timezone aware casting.

jasl commented 6 years ago

Or maybe you can create a subclass of Type::Date, Type::Datetime, and overrides ActiveModel::Type lookup table, see https://github.com/rails/rails/blob/master/activemodel/lib/active_model/type.rb#L44

But that's a bad design of Rails, AR reimplement this, so you also need to overrides here https://github.com/rails/rails/blob/master/activerecord/lib/active_record/type.rb#L69