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

Do not TSort Finisher and Bootstrap initializers #1602

Closed
wants to merge 1 commit into from

Conversation

cameel
Copy link
Contributor

@cameel cameel commented Jun 9, 2011

  • Use sorting in topological order (TSort) on all initializers found by Railtie but not on the ones from Rails::Application::Bootstrap and Rails::Application::Finisher. Bootstrap should always be the first and Finisher the last.
  • Move Rails::Initializable::Collection to Rails::Application. Now it's only needed there.

This fixes issue 1587 reported by me yesterday.

- Use sorting in topological order (TSort) on all initializers found by Railtie but not on the ones from Rails::Application::Bootstrap and Rails::Application::Finisher. Bootstrap should always be the first and Finisher the last.
- Move Rails::Initializable::Collection to Rails::Application. Now it's only needed there.
@vijaydev
Copy link
Member

vijaydev commented Jun 9, 2011

Suggestion: It will be useful to mention the issue number itself as the link text in "This fixes <issue-num>" :-)

@cameel
Copy link
Contributor Author

cameel commented Jun 9, 2011

ok. link fixed.

@josevalim
Copy link
Contributor

I am afraid that, after this change, people won't be able to use before/after using bootstrap and finisher as references. Also, I don't agree with the TSort responsibility moving to the application object. Maybe cells should be using the finisher hooks and/or passing both before/after instead of just one?

@cameel
Copy link
Contributor Author

cameel commented Jun 9, 2011

Right, it might be better for Cells to use :after_initialize hook rather than initializers. Even though it explicitly states in its Railtie that it wants to run after Finisher's :set_routes_reloader initializer, it seems to work fine with my patch that always places it before the Finisher. I'll report it in their repo.

Still, the issue with :after_initialize remains. There should be more constraints on :finisher_hook so that other initializers can't push it down.

And is :after_initialize really a good name for this hook? It's misleading since it's not guaranteed to run after all initializers. At very least :set_routes_reloader and :disable_dependency_loading initializers from Finisher run after the hooks. Moreover, it's possible for an initializer to request to be run after :finisher_hook. This can be a source or errors (as this issue clearly shows).

Or maybe :after_initialize would better not be executed by an initializer?

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.

after_initialize hooks sometimes get executed during initialization
3 participants