Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fully qualify the Version class to prevent load-errors #183

Merged

Conversation

senny
Copy link
Contributor

@senny senny commented Nov 27, 2012

We use paper_trail inside engines and this causes load-errors because Rails will load the wrong file for the Version class. The exact error is:

     LoadError:
       Expected /project/lib/project/version.rb to define Project::Version

I patched paper_trail to fully qualify the constant name "::Version" this should have no effect on existing applications but fixed the problem for me.

@batter
Copy link
Collaborator

batter commented Nov 27, 2012

This seems to be the same issue noted in #165. Not certain, but I think a more thorough solution is called for, likely namespacing the Version class itself.

I'm curious as to why the Travis build failed for your pull request, because I cloned your code down and ran the test suite and all the tests passed for me. Any ideas?

@senny
Copy link
Contributor Author

senny commented Nov 27, 2012

@fullbridge-batkins the PR failed to build because of the latest version of capybara. There is a known issue, that the dependencies are not loaded correctly for 2.0.1. A call to bundle update capybara fixes the problem but just on the machine where you call it. More on this here teamcapybara/capybara#882

I submitted the PR because this solution is simple and fixes broken cases without compromising backwards compatibility. Of course this does not help in the case where a Version class is already present.

If the other PR is about to be merged and released, I'm fine and this PR can be closed. Otherwise it would be nice to have this one in for the mean time.

@bjoernbur
Copy link

👍

@batter
Copy link
Collaborator

batter commented Nov 28, 2012

@senny - Thanks for the response and the pull request. I think what you're saying makes sense. Can you elaborate a bit more on the use case where you're encountering this issue? When you say engines, are you referring to a Rails::Engine class or another gem?

I'm just trying to understand for my own knowledge why this change makes it work in a scenario like this. ::Version makes ruby go up the call chain into the global scope before it attempts to resolve which class it should use for Version, correct? Are you attempting to bundle paper_trail into a non-rails project or?

@senny
Copy link
Contributor Author

senny commented Nov 28, 2012

We use rails engines heavily. Engines are parts of a rails app, which is namespaced and distributed like a gem. The default generated scaffold of an engine rails plugin new my_cool_engine --full creates a file called lib/my_cool_engine/version.rb. This file holds the MyCoolEngine::VERSION constant, which is common in gem development.

Now imagine the following situation:

module MyCoolEngine
  class ExampleContext
    def run
      SomeARModel.create!
    end
  end
end

Let's assume SomeARModel is configured to use PaperTrail. A call to the run method would den trigger the creation of a new Version. Because we are inside the MyCoolEngine context, the rails autoloading trips and thinks the Version class is located in lib/my_cool_engine/version.rb. Since it can't find the class, this results in the exception above.

If we prefix the Version class name with :: we tell the rails autoloading code to ignore the context and to look in the root namespace.

@fullbridge-batkins I hope this explanation is understandable. 😄

@stmichael
Copy link

👍

@batter
Copy link
Collaborator

batter commented Nov 28, 2012

@senny - Thanks a lot for the explanation. That's very helpful and informative! What you're saying makes a lot of sense, I just wanted to understand exactly what the inner workings of this was, so cheers for that! And thanks again for the pull request. 😀

@batter batter merged commit 3a72dac into paper-trail-gem:master Nov 28, 2012
@senny senny deleted the full_qualified_reference_to_version branch December 28, 2012 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants