search

state-machines / state_machines

Adds support for creating state machines for attributes on any Ruby class
https://github.com/state-machines/state_machines
MIT License
814 stars 91 forks source link

to_sym on attributes given to MachineCollection #6

Closed huoxito closed 9 years ago

huoxito commented 9 years ago

Earlier today we noticed an issue in one of our projects where the state wouldn't be assigned on initialization when provided as a string (state attributes are usually kept as a symbol by default)

This would fix the issue but probably bring some performance / security penalties for projects running on older ruby versions

Ultimately this should also highlight the project is in a tough situation. The attributes arg I had added to initialize_states was to fix issues related to ativemodel / activerecord initialization but it ends up making it very confusing if you look at this gem alone. I don't understand most of it but the root idea of adding state machines to any ruby object and on top of that work with the very specific and complicated initialization process for both activerecord and activemodel might be too much. Picking either one or another to support could makes things simpler

// @seuros what you think? any better proper suggestion to fix this? it's far from ideal but couldn't find any other solution without adding more method overrides among the state_machines gems. Mixing regular ruby objects with activerecord initialization doesn't feel like a good idea in the first place. Perhaps we should try to make this distinction more clear.

seuros commented 9 years ago

You are totally right, we should extract the Rails things to SM-AM . Did you have a look at https://github.com/state-machines/meta ? It might make thing easier for cross gem modifications.
I'm going to work on the other integrations (mongoid, datamapper) asap.

I'm going to merge and release a new version.

huoxito commented 9 years ago

hey @seuros thanks I had missed that gem, I'll try to review the initialization process again this weekend and run the tests there, I'm hoping we can revert this patch once I can figure a better fix on sm-ar and sm-am instead cheers