Engine classes not loaded in dev mode if class with same name was already loaded in main app #6413

Closed
ocha opened this Issue May 20, 2012 · 9 comments

Projects

None yet

4 participants

@ocha
ocha commented May 20, 2012

Engine classes/modules (constants) are not loaded in development environment if constant with same name already exists in main app and was accessed. For example ApplicationController. In production everything works fine because of cache_classes (I think), but in dev mode if Engine constants are not preloaded, then ruby resolves main app constant instead of having rails preloaded engine constant.

Example to reproduce:
Create a new rails 3.2 app TestApp and add mounted engine TestEngine.
Add TestController to TestEngine with method index that inherits from ApplicationController. This is how rails generator for engine would set it up for you by default:

module TestEngine
  class TestController < ApplicationController
    def index
    end
  end
end

If for any reason ::ApplicationController already was accessed prior to getting in TestController, TestController will inherit from main app ApplicationController instead of TestEngine::ApplicationController...

This is an issue with any module/class names that are same but seperated with engine namespace like ::ApplicationController and ::TestEngine::ApplicationController...

@drogus drogus added a commit that closed this issue May 20, 2012
@drogus drogus Fix generators to help with ambiguous `ApplicationController` issue
In development mode, dependencies are loaded dynamically at runtime,
using `const_missing`. Because of that, when one of the constants is
already loaded and `const_missing` is not triggered, user can end up
with unexpected results.

Given such file in an Engine:

```ruby
module Blog
  class PostsController < ApplicationController
  end
end
```

If you load it first, before loading any application files, it will
correctly load `Blog::ApplicationController`, because second line will
hit `const_missing`. However if you load `ApplicationController` first,
the constant will be loaded already, `const_missing` hook will not be
fired and in result `PostsController` will inherit from
`ApplicationController` instead of `Blog::ApplicationController`.

Since it can't be fixed in `AS::Dependencies`, the easiest fix is to
just explicitly load application controller.

closes #6413
7c95be5
@drogus drogus closed this in 7c95be5 May 20, 2012
@drogus drogus reopened this May 20, 2012
@drogus
Member
drogus commented May 20, 2012

I explained why is this happening here: http://stackoverflow.com/questions/10042348/rails-3-2-engine-layouts

Unfortunately this can't be easily fixed in AS::Dependencies, but we should generate proper controllers with explicit application_controller require`. I pushed a fix that addresses that, I will push it to master and 3-2-stable if tests pass.

@drogus
Member
drogus commented May 21, 2012

Ok, fix is in master (here: 7c95be5) and in 3-2-stable (here b0f8355).

@pwnall pwnall pushed a commit to pwnall/rails that referenced this issue May 21, 2012
@drogus drogus Fix generators to help with ambiguous `ApplicationController` issue
In development mode, dependencies are loaded dynamically at runtime,
using `const_missing`. Because of that, when one of the constants is
already loaded and `const_missing` is not triggered, user can end up
with unexpected results.

Given such file in an Engine:

```ruby
module Blog
  class PostsController < ApplicationController
  end
end
```

If you load it first, before loading any application files, it will
correctly load `Blog::ApplicationController`, because second line will
hit `const_missing`. However if you load `ApplicationController` first,
the constant will be loaded already, `const_missing` hook will not be
fired and in result `PostsController` will inherit from
`ApplicationController` instead of `Blog::ApplicationController`.

Since it can't be fixed in `AS::Dependencies`, the easiest fix is to
just explicitly load application controller.

closes #6413
b0f8355
@pixeltrix
Member

This can't be fixed in AS::Dependencies since that depends on const_missing being called which it never is in this case because Ruby finds the top-level ApplicationController first before calling const_missing.

@drogus you may want to change the require to require_dependency as the latter will reload in development mode.

@ocha
ocha commented May 21, 2012

Yes. I understand whats going on and why it is hard to fix. I think guide for engines should have a word about using same constant names in engines and main apps and maybe a recommendation for best practices not to re-use same names for this reason... basically a little warning.

@drogus
Member
drogus commented May 21, 2012

@pixeltrix that's really werid. I was sure that it will work. When you require files after initialization it should not matter if you use require_dependency or not. I've just checked it and it works correctly in rails 3.1.0 and breaks on 3.2.3 (breaks, meaning that it won't reload required file). I would consider it a bug and a regression between 3.1 and 3.2.

I will look into it and try to provide fix.

@drogus
Member
drogus commented May 21, 2012

@igagnidz sure, I agree with that, I wanted to add documentation, but had limited time yesterday, so I just provided a fix (unfortunately it will help only for new apps)

@drogus
Member
drogus commented May 22, 2012

@pixeltrix I'm wrong. I'm not sure why I thought that it should work and I actually messed up testing it. It worked in 3.1 only because I changed the wrong file.

After consideration, I realized that it can't work like that, as ActiveSupport::Dependencies still uses require under the hood, all it does is wrapping it with constants watching code: https://github.com/rails/rails/blob/master/activesupport/lib/active_support/dependencies.rb#L247

I'll push a fix, thanks for pointing that out!

@schneems
Member

@drogus what's the state of your fix, is it in the codebase? Can we close this issue?

@drogus
Member
drogus commented Aug 24, 2012

@schneems yes, we can close this.

For reference: the fix is to add require_dependency to explicitly require correct file, it will be generated for new files, but if you have existing engine, you need to add it by yourself.

@drogus drogus closed this Aug 24, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment