pluginaweek / state_machine

Adds support for creating state machines for attributes on any Ruby class
http://www.pluginaweek.org
MIT License
3.74k stars 507 forks source link

Mongo Mapper master not compatible #268

Open plusplus opened 11 years ago

plusplus commented 11 years ago

The master branch of mongo mapper isn't happy with the state_machine latest gem. I'm just putting this here to remind me to look at it later. I've raised an issue there as well.

This repo demonstrates the issue: https://github.com/jnunemaker/mongomapper/issues/531

The mongo mapper issue is here: https://github.com/jnunemaker/mongomapper/issues/531

cheald commented 11 years ago

Hi there, I'm responsible for the changes in MM that have surfaced this. I think that strictly speaking, this is a bug in state_machine that was exposed by recent changes to MM's internals.

In lib/state_machine/integrations/mongo_mapper.rb:

def define_state_initializer
  define_helper :instance, <<-end_eval, __FILE__, __LINE__ + 1
    def initialize(*args)
      self.class.state_machines.initialize_states(self, :static => :force) { super }
    end
  end_eval
end

state_machine is doing its initialization before MongoMapper's #initialize gets to run. One of the things we're doing to improve performance is to do initial setup on initialize, and then relying on that to be there rather than checking that every time we try to write a key. Previously, this worked because MM would automagically define keys on-the-fly and didn't rely on instance state to do so. This is great, except it's also really slow.

I'm going to see if I can get this working in MM in a not-terribly-slow fashion, but if it'd be possible to call super and then call initialize_states, that would be ideal.

plusplus commented 11 years ago

FWIW: I can work around this by converting:

state_machine initial: :shopping  do
  # ...

to:

def initialize(*args)
  super
  self.class.state_machines.initialize_states(self, static: :force)
end

state_machine initial: :shopping, initialize: false do
  # ...

Which would suit me fine if it avoids slow downs in MM and/or nasty code and doesn't do BAD things to my business logic (to the tests!)

cheald commented 11 years ago

I think that would be more correct since the state machine setup relies on MM's internals, so it's only fair to let it do its setup first.

That being said, I've fixed the issue in a way that shouldn't be too awful performance wise, so it's really just an abstract exercise at this point. Master should be working without any more changes necessary. On Jul 9, 2013 6:50 PM, "Julian Russell" notifications@github.com wrote:

FWIW: I can work around this by converting:

state_machine initial: :shopping do

...

to:

def initialize(*args) super self.class.state_machines.initialize_states(self, static: :force) end

state_machine initial: :shopping, initialize: false do

...

Which would suit me fine if it avoids slow downs in MM and/or nasty code and doesn't do BAD things to my business logic (to the tests!)

— Reply to this email directly or view it on GitHubhttps://github.com/pluginaweek/state_machine/issues/268#issuecomment-20717205 .