dragonfly icon indicating copy to clipboard operation
dragonfly copied to clipboard

[Feature Request] Optionally disable attribute setters for UID and "magic" attributes

Open mvastola opened this issue 9 years ago • 2 comments

Hi there, First off, thanks for maintaining a fantastic gem!

I'd like to recommend this as a change and/or inquire as to whether a proper PR with this modification would be merged:

I would ask you consider implementing (perhaps optionally -- by default or otherwise) overrides to the writer methods for the UID (i.e. #{attribute}_uid=) and magic attributes (i.e. #{attribute}_#{property}=) on AR records (and perhaps the equivalent public methods in Dragonfly::Model::Attachment instances) to throw warnings or errors and prevent them from being (likely unintentionally) directly overwritten with (likely incorrect) values not computed by the routines in this gem.

The only repercussion in the code I can see so far is that it would require switching to using the slightly lower-level AR::Base public instance methods read_attribute and write_attribute when altering AR attributes from Dragonfly::Model::Attachment.

mvastola avatar Mar 17 '16 04:03 mvastola

hi - thanks for the request - to be honest I'd unlikely consider this because

  • the dragonfly model mixin actually doesn't know it's dealing with activerecord - it's designed to work with any model that has #{attribute}_uid defined http://markevans.github.io/dragonfly/models/#defining-accessors, and optionally the corresponding magic attributes, so I don't want to have to bring in AR-specific stuff like read_attribute
  • the user may want to override it for some reason (so a warning would be better than an error)
  • it hasn't seemed to have been an issue up till now

Has it been the source of particular issues for you?

markevans avatar Mar 17 '16 11:03 markevans

Ah. It didn't occur to me that this gem wasn't just for Rails (or otherwise had a more in depth abstraction layer).

It's not really a big issue, but it did just come up while I was coding for me:

I had to think a moment about which value to nil -- #{attribute} or #{attribute}_uid to ensure the underlying file is unlinked and if I had to do the same to the property columns (I goofed and forgot about the magicness).

Once I figured it out, I realized it was less intuitive than it should ideally be, so my general habit in these cases (with my code especially) is to try to dummy-proof them, since my having the issue means someone else will, and if they're less fluent in debugging ruby, they won't manage to fix it.

Anywho, I was going to override or remove the attribute writer on my end and request the small switch to write_attribute here, but upon noticing you used the attribute writer directly in your code, this became too big an ordeal. Thus this post.

What if we were to implement this solution by defining a Dragonfly::Model::Attachment#read_model_attribute and a Dragonfly::Model::Attachment#write_model_attribute method that uses the superclass of the model to determine how to access the variable, and uses the current access method (or instance_attribute_set) as a fallback?

mvastola avatar Mar 17 '16 13:03 mvastola