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

Switch to Railtie, add config.paper_trail.enabled #695

Conversation

Hermanverschooten
Copy link
Contributor

This PR does 2 things.

@jaredbeck
Copy link
Member

This PR does 2 things.

Would it be awkward to separate this into two PRs? Thanks.

@batter
Copy link
Collaborator

batter commented Jan 18, 2016

Also, the Railtie class should be housed within lib/paper_trail/frameworks/rails

@batter
Copy link
Collaborator

batter commented Jan 18, 2016

And it should be loaded in by the PaperTrail::Rails module most likely

@Hermanverschooten
Copy link
Contributor Author

I am not familiar with the codebase, I just replaced it in the spot where active record was loaded before.
But I'll have a got at it.

@Hermanverschooten
Copy link
Contributor Author

Ok, I moved the code to their respective frameworks.
About splitting the PR, I don't know where/how to access the config variables outside of the RailTie.
So the second PR would still be dependent on this one.

@@ -34,9 +35,13 @@ def track_associations
end
alias_method :track_associations?, :track_associations

def disable!
@disabled = true
end
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method, and the @disabled ivar, seem unnecessary, given that we already have enabled=.

@jaredbeck
Copy link
Member

There is another problem with the proposed @disabled ivar, that I didn't see before. PaperTrail::Config is a singleton, so its instance variables are not thread-safe. That's why we started using RequestStore.

As I stated in #635, we have already decided that the flag which enables PT should be thread-safe. It would be very confusing to have two flags controlling whether PT were enabled, and even more confusing if one is thread-safe and the other (@disabled) is not.

I'm going to continue exploring this PR, because I want to learn more about railties in hopes that will fix #635. As it stands, we can't merge this, but let me see what I can come up with.

@jaredbeck
Copy link
Member

Unfortunately, simply removing the non-thread-safe @disabled ivar and keeping the rest of the changes re: railties fails to fix #635. Herman, do you have any other ideas?

@jaredbeck
Copy link
Member

In #635, we are discussing making the main flag "global" again (ie. moving it out of the RequestStore)

@jaredbeck
Copy link
Member

Closing because this PR is continued by #717, which keeps @Hermanverschooten's idea and authorship (thank you!) but renames certain methods to clarify which are thread-safe and which are not.

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.

3 participants