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

Only load Railtie integration if Rails::Railtie is defined #319

Merged
merged 1 commit into from Jun 4, 2022

Conversation

Morriar
Copy link
Member

@Morriar Morriar commented Jun 1, 2022

As pointed by the Rails documentation, creating Railties should explicitly ensure that Rails::Railtie is defined.

There could be non-rails application use-cases where Rails is defined, yet Rails::Railtie is not loaded.

We looked into how to add a new test for this use-case but couldn't figure a simple way to load Rails without Railtie. Any idea how to do it? Or are we happy with the change without test?

As pointed by the [Rails documentation](https://api.rubyonrails.org/classes/Rails/Railtie.html#class-Rails::Railtie-label-Creating+a+Railtie),
creating Railties should explicitly ensure that `Rails::Railtie` is defined.

There could be non-rails application use-cases where `Rails` is defined, yet
`Rails::Railtie` is not loaded.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Co-authored-by: Ufuk Kayserilioglu <ufuk.kayserilioglu@shopify.com>
@pkuczynski pkuczynski added this to the 3.0.0 milestone Jun 1, 2022
@pkuczynski pkuczynski modified the milestones: 4.0.0, 4.0.1 Jun 1, 2022
@pkuczynski
Copy link
Member

@cjlarose what do you think? I think we can merge it without the test...

@Morriar Morriar mentioned this pull request Jun 3, 2022
@pkuczynski pkuczynski merged commit 740fe1f into rubyconfig:master Jun 4, 2022
danilogco pushed a commit to dcotecnologia/confset that referenced this pull request Jun 6, 2022
…onfig#319)

Co-authored-by: Ufuk Kayserilioglu <ufuk.kayserilioglu@shopify.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants