search

projectblacklight / blacklight_advanced_search

Advanced search plugin for Blacklight
http://projectblacklight.org
Other
25 stars 25 forks source link

Two errors in custom merge #55

Closed atz closed 8 years ago

atz commented 8 years ago

Bad check at: https://github.com/projectblacklight/blacklight_advanced_search/blob/master/lib/blacklight_advanced_search.rb#L30

if new_value.respond_to?(:blank) && new.blank?

:blank vs. :blank? and new_value vs. new

Looks like the new/new_value error was introduced here: https://github.com/projectblacklight/blacklight_advanced_search/commit/6d3b49ace134b488c92b63e31fd622a82a5951cc#diff-4961a18d7c088f36dc4f670c73ae74a6R30

And the other in 2011 or before (so far back, I don’t care anymore). Got test coverage?

atz commented 8 years ago

Maybe time to undo that whole method now that BL doesn't have to support rails3? https://github.com/projectblacklight/blacklight/issues/827

atz commented 8 years ago

Note: when I fix this error, the behavior further diverges from Rails deep_merge because 

false.blank? ==> true
true.blank? ==> false

So any boolean false values in the overlaying hash are ignored. But when set to true the same values are enforced. This doesn't make any sense, but Rails has chosen.

mjgiarlo commented 8 years ago

:eyes: