Navigation Menu

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

Autoloading helpers fails initially in Rails 7.x, but succeeds on the second attempt #43205

Closed
bradgessler opened this issue Sep 10, 2021 · 20 comments
Assignees

Comments

@bradgessler
Copy link

bradgessler commented Sep 10, 2021

Steps to reproduce

When I try loading a Rails Application in Ruby 7.0, the app/helpers autoload path does not appear to be working from my gems engine.

Here's the script to reproduce the issue NOT working under 7.0:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "sitepress", github: "sitepress/sitepress", branch: "rails-7"
  gem "rails", github: "rails/rails", branch: "main"
end

require "sitepress/server"  # Load all the stuff needed setup the configuration below.

# Setup defaults for stand-alone Sitepress server in the current path. This
# can, and should, be over-ridden by the end-user in the `config/site.rb` file.
Sitepress.configure do |config|
  config.routes = false
  config.site = Sitepress::Site.new root_path: "."
end

# Create the module that should autoload without an issues.
require 'fileutils'
FileUtils.mkdir_p 'helpers'
File.write "helpers/foo_helper.rb", "module FooHelper; end;"

# Boot the Rails app
app = Sitepress::Server
app.initialize!

puts "Loading ::SiteController fails in Rails 7.0"
# See https://github.com/sitepress/sitepress/blob/rails-7/sitepress-rails/lib/sitepress/engine.rb#L15 for helper path configuration
# See https://github.com/sitepress/sitepress/blob/rails-7/sitepress-server/lib/sitepress/server.rb for the configuration of the Rails app this boots
p ::SiteController # This will raise an exception

Now for code that shows this WORKING under Rails 6:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "sitepress", github: "sitepress/sitepress", branch: "rails-7"
  gem "rails"
end

require "sitepress/server"  # Load all the stuff needed setup the configuration below.

# Setup defaults for stand-alone Sitepress server in the current path. This
# can, and should, be over-ridden by the end-user in the `config/site.rb` file.
Sitepress.configure do |config|
  config.routes = false
  config.site = Sitepress::Site.new root_path: "."
end

# Create the module that should autoload without an issues.
require 'fileutils'
FileUtils.mkdir_p 'helpers'
File.write "helpers/foo_helper.rb", "module FooHelper; end;"

# Boot the Rails app
app = Sitepress::Server
app.initialize!

puts "Loading ::SiteController succeeds in Rails 6.x"
p ::SiteController # This will not raise an exception

Finally, in a third example under Rails 7.0, I demonstrate that this is WORKING when I attempt to load the constant a second time:

require "bundler/inline"

gemfile(true) do
  source "https://rubygems.org"

  git_source(:github) { |repo| "https://github.com/#{repo}.git" }

  # Activate the gem you are reporting the issue against.
  gem "sitepress", github: "sitepress/sitepress", branch: "rails-7"
  gem "rails", github: "rails/rails", branch: "main"
end

# require "sitepress/boot"
require "sitepress/server"  # Load all the stuff needed setup the configuration below.

# Setup defaults for stand-alone Sitepress server in the current path. This
# can, and should, be over-ridden by the end-user in the `config/site.rb` file.
Sitepress.configure do |config|
  config.routes = false
  config.site = Sitepress::Site.new root_path: "."
end

# Create the module that should autoload without an issues.
require 'fileutils'
FileUtils.mkdir_p 'helpers'
File.write "helpers/foo_helper.rb", "module FooHelper; end;"

# Boot the Rails app
app = Sitepress::Server
app.initialize!

puts "Loading ::SiteController fails in Rails 7.0 on the first attempt"
begin
  p ::SiteController
rescue => e
  puts "Failed with #{e.inspect}"
end

puts "Loading ::SiteController succeeds in Rails 7.0 on the second attempt"
p ::SiteController

The following source files may be helpful to help diagnose the issue:

Expected behavior

I expect the FooHelper to be resolved in Rails 7.x, as it does in Rails 6.x. Here's what a succesful script run looks like from above:

SiteController

Actual behavior

The FooHelper module is not resolved when Rails attempts to find it. Here's what the error looks like from the first Rails 7.x script above:

/Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/rails-a692e63bf400/activesupport/lib/active_support/inflector/methods.rb:280:in `constantize': uninitialized constant FooHelper (NameError)
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/rails-a692e63bf400/activesupport/lib/active_support/core_ext/string/inflections.rb:74:in `constantize'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/rails-a692e63bf400/actionpack/lib/abstract_controller/helpers.rb:177:in `block in modules_for_helpers'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/rails-a692e63bf400/actionpack/lib/abstract_controller/helpers.rb:170:in `map!'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/rails-a692e63bf400/actionpack/lib/abstract_controller/helpers.rb:170:in `modules_for_helpers'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/rails-a692e63bf400/actionpack/lib/action_controller/metal/helpers.rb:104:in `modules_for_helpers'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/rails-a692e63bf400/actionpack/lib/abstract_controller/helpers.rb:148:in `helper'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/rails-a692e63bf400/actionpack/lib/action_controller/railties/helpers.rb:19:in `inherited'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/sitepress-cd078ab7255c/sitepress-server/rails/app/controllers/application_controller.rb:1:in `<top (required)>'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/zeitwerk-2.5.0.beta3/lib/zeitwerk/kernel.rb:27:in `require'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/zeitwerk-2.5.0.beta3/lib/zeitwerk/kernel.rb:27:in `require'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/bundler/gems/sitepress-cd078ab7255c/sitepress-server/rails/app/controllers/site_controller.rb:1:in `<top (required)>'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/zeitwerk-2.5.0.beta3/lib/zeitwerk/kernel.rb:27:in `require'
	from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/zeitwerk-2.5.0.beta3/lib/zeitwerk/kernel.rb:27:in `require'
	from bug.rb:40:in `<main>'

System configuration

Rails version: 7.x

Ruby version: 3.x

@bradgessler bradgessler changed the title Autoloading helpers fails in Rails 7.x Autoloading helpers fails initially in Rails 7.x, but succeeds on the second attempt Sep 10, 2021
@rafaelfranca
Copy link
Member

This could be an issue on sitepress. Are you able to reproduce in a regular Rails application without using Sitepress?

@bradgessler
Copy link
Author

bradgessler commented Sep 10, 2021

Short answer is no, I haven't tried reproducing this in a vanilla Rails 7.x app. I doubt its an issue there since helpers are so heavily tested.

The longer answer is that Sitepress itself is a stripped-down Rails application (see https://github.com/sitepress/sitepress/blob/rails-7/sitepress-server/lib/sitepress/server.rb). The paths that are not loading are set in an engine (see https://github.com/sitepress/sitepress/blob/rails-7/sitepress-rails/lib/sitepress/engine.rb#L21-L26). While its not a full-blown Rails application in a rails new foo_bar sense, it should serve as a simpler use case since it is only loading a subset of Rails.

The reason I think this is of interest is that if I'm seeing this strange behavior in what should be a "simple use case", it will appear in more complicated rails apps and/or gems.

If this turns out to not be a bug in the Rails 7.x autoloading features, I'd guess that whatever I've found is worth documenting in the autoloading change log, autoloading docs, or the Rails engine docs.

@fxn
Copy link
Member

fxn commented Sep 16, 2021

I've played around for a few minutes. Some details I see:

First, let's eliminate case (3) from the equation. In Ruby, you know that if you have this

# foo.rb
class Foo
  raise

  def x
  end
end

and then

begin
  require "foo"
rescue
  # ignored
end

Foo

That works, because the Foo constant was actually defined. You are not loading again and succeeding, it was the first load the succeeded partially. Of course, instances of Foo won't respond to #x, that is expected.

That is in essence what is happening in case (3).

There's one difference between case (1) and case (2), if you

pp ActiveSupport::Dependencies.autoload_paths

after initialization, you'll see tmp/helpers is present in (2), and missing in (1).

I don't know why is that yet, just reached this point by now, but looks like the problem is the configurations, for some reason, are different between both versions.

I'll try to dig into it, thanks a lot for testing the alpha ❤️.

@fxn fxn self-assigned this Sep 16, 2021
@bradgessler
Copy link
Author

I tested against alpha2 and dug into some of the docs in there to see if any of the new info in the README (https://github.com/rails/rails/blob/main/guides/source/autoloading_and_reloading_constants.md) would help out.

I discovered Rails.autoloaders.log! and dropped that into the loading. Here's what I get on the first attempt to load ::SiteController

[1] pry(#<Sitepress::CLI>)> pp ActiveSupport::Dependencies.autoload_paths
["/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/helpers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns"]
=> ["/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/helpers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns"]
[2] pry(#<Sitepress::CLI>)> ::SiteController
Zeitwerk@rails.main: constant ApplicationHelper loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/helpers/application_helper.rb
NameError: uninitialized constant PageHelper
from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activesupport-7.0.0.alpha2/lib/active_support/inflector/methods.rb:280:in `constantize'

Then on the second attempt:

[3] pry(#<Sitepress::CLI>)> pp ActiveSupport::Dependencies.autoload_paths
["/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/helpers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns"]
=> ["/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/helpers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers",
 "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns"]
[4] pry(#<Sitepress::CLI>)> ::SiteController
Zeitwerk@rails.main: constant ApplicationController loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/application_controller.rb
Zeitwerk@rails.main: constant Sitepress::SitePages loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns/sitepress/site_pages.rb
Zeitwerk@rails.main: constant SiteController loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/site_controller.rb
=> SiteController

From what I observe, the ActiveSupport::Dependencies.autoload_paths are the same between both attempts, but the Zeitwork logger is kicking in on the second attempt to reference that controller.


@fxn people don't say this enough in OSS projects, but much 🤟and 🤗 for digging into this issue. I'm going to keep working at it too and see if I can understand the problem better.

@bradgessler
Copy link
Author

I found another surprising thing: the Rails.autoloaders.main.autoloads paths changes after ::SiteController successfully loads on the second attempt.

[1] pry(#<Sitepress::CLI>)> Rails.autoloaders.main.autoloads
=> #<Zeitwerk::Autoloads:0x00007fef91204f30
 @a2c=
  {"/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/application_controller.rb"=>[Object, :ApplicationController],
   "/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/site_controller.rb"=>[Object, :SiteController],
   "/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/helpers/application_helper.rb"=>[Object, :ApplicationHelper],
   "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/sitepress/site_controller.rb"=>[Sitepress, :SiteController],
   "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns/sitepress/site_pages.rb"=>[Sitepress, :SitePages]},
 @c2a=
  {[Object, :ApplicationController]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/application_controller.rb",
   [Object, :SiteController]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/site_controller.rb",
   [Object, :ApplicationHelper]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/helpers/application_helper.rb",
   [Sitepress, :SiteController]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/sitepress/site_controller.rb",
   [Sitepress, :SitePages]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns/sitepress/site_pages.rb"}>
[2] pry(#<Sitepress::CLI>)> ::SiteController
Zeitwerk@rails.main: constant ApplicationHelper loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/helpers/application_helper.rb
NameError: uninitialized constant PageHelper
from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activesupport-7.0.0.alpha2/lib/active_support/inflector/methods.rb:280:in `constantize'

Then I try it again:

[3] pry(#<Sitepress::CLI>)> Rails.autoloaders.main.autoloads
=> #<Zeitwerk::Autoloads:0x00007fef91204f30
 @a2c=
  {"/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/application_controller.rb"=>[Object, :ApplicationController],
   "/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/site_controller.rb"=>[Object, :SiteController],
   "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/sitepress/site_controller.rb"=>[Sitepress, :SiteController],
   "/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns/sitepress/site_pages.rb"=>[Sitepress, :SitePages]},
 @c2a=
  {[Object, :ApplicationController]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/application_controller.rb",
   [Object, :SiteController]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/site_controller.rb",
   [Sitepress, :SiteController]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/sitepress/site_controller.rb",
   [Sitepress, :SitePages]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns/sitepress/site_pages.rb"}>
[4] pry(#<Sitepress::CLI>)> ::SiteController
Zeitwerk@rails.main: constant ApplicationController loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/application_controller.rb
Zeitwerk@rails.main: constant Sitepress::SitePages loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns/sitepress/site_pages.rb
Zeitwerk@rails.main: constant SiteController loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/site_controller.rb

Finally when I run Rails.autoloaders.main.autoloads after the controller is successfully loaded, I get a different set of autoload paths:

[5] pry(#<Sitepress::CLI>)> Rails.autoloaders.main.autoloads
=> #<Zeitwerk::Autoloads:0x00007fef91204f30
 @a2c={"/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/sitepress/site_controller.rb"=>[Sitepress, :SiteController]},
 @c2a={[Sitepress, :SiteController]=>"/Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/sitepress/site_controller.rb"}>

I'd expect these paths to not change after the application & Rails boot successfully (assuming no mutations after that)

@fxn
Copy link
Member

fxn commented Sep 16, 2021

Regarding the first comment, in the first attempt this tries to load FooHelper, and fails. Because the configuration does not have the directory. In the second attempt, you move on because of the partial evaluation, that code is not executed again, and you are able to move on.

Regarding the second comment. That is internal, private state, and it is mutated in place.

I believe our best hint so far is that the configuration is different. In Rails 6, the directory that contains FooHelper is not in the autoload paths. That is the root of everything (I believe!), FooHelper cannot be autoloaded in the Rails 7 example, and that squares because the configuration does not have it. However, to debug that I'll need to understand your engine, I suspect.

Something that we could do is bisect rails/main.

@bradgessler
Copy link
Author

I'm narrowing down the problem to eager loading. First attempt:

[1] pry(#<Sitepress::CLI>)> Rails.autoloaders.main.eager_load
Zeitwerk@rails.main: eager load start
Zeitwerk@rails.main: constant ApplicationHelper loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/helpers/application_helper.rb
NameError: uninitialized constant PageHelper
from /Users/bradgessler/.rbenv/versions/3.0.0/lib/ruby/gems/3.0.0/gems/activesupport-7.0.0.alpha2/lib/active_support/inflector/methods.rb:280:in `constantize'

Second attempt:

[2] pry(#<Sitepress::CLI>)> Rails.autoloaders.main.eager_load
Zeitwerk@rails.main: eager load start
Zeitwerk@rails.main: constant ApplicationController loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/application_controller.rb
Zeitwerk@rails.main: constant Sitepress::SitePages loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/concerns/sitepress/site_pages.rb
Zeitwerk@rails.main: constant SiteController loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-server/rails/app/controllers/site_controller.rb
Zeitwerk@rails.main: constant Sitepress::SiteController loaded from file /Users/bradgessler/Projects/sitepress/gem/sitepress-rails/rails/app/controllers/sitepress/site_controller.rb
Zeitwerk@rails.main: eager load end
=> nil

Regarding the second comment. That is internal, private state, and it is mutated in place.

I see that now. Thanks for pointing this out.

I believe our best hint so far is that the configuration is different

When you say configuration do you mean my application's configuration or a change to the overall Rails configuration?

@fxn
Copy link
Member

fxn commented Sep 16, 2021

When you say configuration do you mean my application's configuration or a change to the overall Rails configuration?

Rails configures Zeitwerk with the contents of ActiveSupport::Dependencies.autoload_paths by the end of application boot, in what we call the finisher.

In your examples, after initialization, ActiveSupport::Dependencies.autoload_paths in Rails 6 contains the directory where FooHelper lives, and in Rails 7, the directory is not there. Hence, FooHelper is not autoloaded (this is consistent with the autoload paths configured). So, what happens after attempting to load FooHelper is not that much important, those are side-effects of partially evaluated code.

What we have to understand is why are the autoload paths different.

@fxn
Copy link
Member

fxn commented Sep 16, 2021

# ... original script ...

# Boot the Rails app
app = Sitepress::Server
app.initialize!

pp ActiveSupport::Dependencies.autoload_paths

see what that prints in Rails 7 and Rails 6.

@bradgessler
Copy link
Author

bradgessler commented Sep 16, 2021

I did a git bisect.

$ git bisect good v6.1.4.1
$ git bisect bad v7.0.0.alpha2

Here's what I got as far as "the first bad commit":

2306a8e645627a7913e161b76eeeb7ee6e6724a9 is the first bad commit
commit 2306a8e645627a7913e161b76eeeb7ee6e6724a9
Author: Xavier Noria <fxn@hashref.com>
Date:   Tue Aug 17 05:23:51 2021 +0200

    Setup the once autoloader on bootstrap

 actionpack/test/controller/helper_test.rb          |  4 +-
 actionview/test/actionpack/abstract/helper_test.rb |  8 ++--
 activesupport/lib/active_support/dependencies.rb   |  4 ++
 .../dependencies/zeitwerk_integration.rb           | 50 +-------------------
 railties/lib/rails/application/bootstrap.rb        | 17 +++++++
 railties/lib/rails/application/finisher.rb         | 24 +++++++++-
 railties/lib/rails/engine.rb                       | 29 +++++++-----
 railties/test/application/configuration_test.rb    | 54 ----------------------
 .../test/application/zeitwerk_integration_test.rb  | 48 +++++++++++++++++++
 9 files changed, 116 insertions(+), 122 deletions(-)

Link: 2306a8e

@fxn
Copy link
Member

fxn commented Sep 16, 2021

That is awesome, and indeed that commit looks like could be related. It's almost midnight here, I'll continue in another moment, but I believe we have moved forward understanding the issue. Thanks a lot!!!

@bradgessler
Copy link
Author

I think I found the problem: 2306a8e#diff-c72ea90e9c6b7c60285006ea4c34548ae9257dffcda0c1b2f41fffd75a14363bR578-R580.

Any rails engine that depends on the before: :set_autoload_paths would break from this change. If I change that to before: :setup_main_autoloader it works.

In my engine when I change:

    # Load paths from `Sitepress#site` into rails so it can render views, helpers, etc. properly.
    initializer :set_sitepress_paths, before: :set_autoload_paths do |app|
      app.paths["app/helpers"].push site.helpers_path.expand_path
      app.paths["app/assets"].push site.assets_path.expand_path
      app.paths["app/views"].push site.root_path.expand_path
      app.paths["app/views"].push site.pages_path.expand_path
    end

to:

    # Load paths from `Sitepress#site` into rails so it can render views, helpers, etc. properly.
    initializer :set_sitepress_paths, before: :setup_main_autoloader do |app|
      app.paths["app/helpers"].push site.helpers_path.expand_path
      app.paths["app/assets"].push site.assets_path.expand_path
      app.paths["app/views"].push site.root_path.expand_path
      app.paths["app/views"].push site.pages_path.expand_path
    end

it works.

@bradgessler
Copy link
Author

bradgessler commented Sep 16, 2021

I know that Rails engines are not that well documented and the API, particularly the callbacks, are brittle. I'm OK with that since Rails engine developers are more technically inclined and can release updates, but I would call this a regression.

I should add that its entirely possible I'm not hooking into the right initializer callback at https://github.com/sitepress/sitepress/blob/rails-7/sitepress-rails/lib/sitepress/engine.rb#L21 in 6.x, but it just happened to work.

Assuming I hooked into the right callback, the ideal fix would be for the 7.x codebase to setup the initializer callback behaviors to fire roughly in the order of 6.x. If that's not possible then I'd propose adding this change to the docs at https://github.com/rails/rails/blob/main/guides/source/autoloading_and_reloading_constants.md.

If updating docs are the way to go, I'm happy to open a PR with those changes.

@bradgessler
Copy link
Author

I should add that its entirely possible I'm not hooking into the right initializer callback at https://github.com/sitepress/sitepress/blob/rails-7/sitepress-rails/lib/sitepress/engine.rb#L21 in 6.x, but it just happened to work.

As I suspected, if I change my code from before: :set_autoload_paths to after: :set_autoload_paths it boots just fine in Rails 7.x and 6.x.

IMO this is the call of a Rails maintainer to make as to wether or not this is a regression or simply "undocumented behavior".

@fxn
Copy link
Member

fxn commented Sep 17, 2021

Fantastic @bradgessler! We have it narrowed down. I'll think about it and will write back.

@bradgessler
Copy link
Author

bradgessler commented Sep 18, 2021

FWIW when I change before: :set_autoload_paths to after: :set_autoload_paths it creates a bunch of other issues unrelated to the autoloader that I'd have to figure out. I'm going to hold off on diving into that raft of issues until I hear back on how ya'll are thinking about this issue.

@fxn
Copy link
Member

fxn commented Sep 19, 2021

The idea of that move was, "we should not need the autoload paths until we setup the main autoloader". But that breaks that initialization logic related to paths, and the fact that set_autoload_paths runs before bootstrap_hook is documented in the configuration guide.

I believe this has to reverted, could you please try this branch?

@bradgessler
Copy link
Author

https://github.com/rails/rails/tree/issue-43205 works! No modification needed in the Sitepress gem for the upgrade from Rails 6.x to 7.x with the updated branch.

@fxn
Copy link
Member

fxn commented Sep 20, 2021

Fantastic @bradgessler! It was great that you caught this one. Thanks for testing the alpha, the isolated reproduction scripts, and the bisect.

I'll polish that patch soon and merge.

@bradgessler
Copy link
Author

Thanks for patching it and a huge thanks for maintaining Zeitwerk and all of your contributions to Rails.

fxn added a commit that referenced this issue Oct 4, 2021
@fxn fxn closed this as completed in fe43770 Oct 4, 2021
DerekCrosson pushed a commit to DerekCrosson/rails that referenced this issue Oct 8, 2021
…s url or fallback

refactor: add port number

refactor: add default fallback if default options has no host

test(rails console): session host is correct

refactor: set default url in correct file

refactor: remove code that is not needed

Remove unused instrumentation hooks from action_controller

Instrumentation hooks for `write_page.action_controller` and
`expire_page.action_controller` seem to be removed
in rails#7833.
So, subscribers for them are no longer necessary.

Fix new method name in DatabaseConfig#config deprecation message

Replace "ActionText" with "Action Text" [ci skip]

http://guides.rubyonrails.org/api_documentation_guidelines.html#wording

Remove unnecessary if in ExtendedDeterministicUniquenessValidator

Specify ORDER BY enumsortorder for postgres enums

Co-authored-by: Daniel Colson <danieljamescolson@gmail.com>

Restore set_autoload_path triggering before bootstrap

Fixes rails#43205.

Automatically infer inverse_of with scopes

Background
---

I recently noticed we had a number of associations in GitHub that would
benefit from having `inverse_of` set, and so I began adding them. I
ended up adding them to virtually every association with a scope, at
which point I wondered whether Rails might be able to automatically find
these inverses for us.

For GitHub, the changes in this commit end up automatically adding
`inverse_of` to 171 of associations that were missing it.

My understanding is that we do not automatically detect `inverse_of` for
associations with scopes because the scopes could exclude the records we
are trying to inverse from. But I think that should only matter if there
is a scope on the inverse side, not on the association itself.

For example:

Scope on has_many
----

```rb
class Post < ActiveRecord::Base
  has_many :comments, -> { visible }
end

class Comment < ActiveRecord::Base
  belongs_to :post

  scope :visible, -> { where(visible: true) }
  scope :hidden, -> { where(visible: false) }
end

post = Post.create!
comment = post.comments.hidden.create!
assert comment.post
```

This code leaves `post.comments` in sort of a weird state, since it
includes a comment that the association would filter out. But that's
true regardless of the changes in this commit.

Regardless of whether the comments association has an inverse,
`comment.post` will return the post. The difference is that when
`inverse_of` is set we use the existing post we have in memory, rather
than loading it again. If there is a downside to having the `inverse_of`
automatically set here I'm not seeing it.

Scope on belongs_to
----

```rb
class Post < ActiveRecord::Base
  has_many :comments

  scope :visible, -> { where(visible: true) }
  scope :hidden, -> { where(visible: false) }
end

class Comment < ActiveRecord::Base
  belongs_to :post, -> { visible }
end

post = Post.hidden.create!
comment = post.comments.create!
assert_nil comment.post
```

This example is a different story. We don't want to automatically infer
the inverse here because that would change the behavior of
`comment.post`. It should return `nil`, since it's scoped to visible
posts while this one is hidden.

This behavior was not well tested, so this commit adds a test to
ensure we haven't changed it.

Changes
---

This commit changes `can_find_inverse_of_automatically` to allow us to
automatically detect `inverse_of` when there is a scope on the
association, but not when there is a scope on the potential inverse
association. (`can_find_inverse_of_automatically` gets called first with
the association's reflection, then if it returns true we attempt to find
the inverse reflection, and finally we call the method again with the
inverse reflection to ensure we can really use it.)

Since this is a breaking change—specifically in places where code may
have relied on a missing `inverse_of` causing fresh copies of a record
to be loaded—we've placed it behind the `automatic_scope_inversing` flag
(whose name was inspired by `has_many_inversing`). It is set to true for
new applications via framework defaults.

Testing
---

In addition to the inverse association tests, this commit also adds some
cases to a few tests related to preloading. They are basically
duplicates of existing tests, but with lower query counts.

Testing this change with GitHub, the bulk of the failing tests were
related to lower query counts. There were additionally 3 places (2 in
tests and one in application code) where we relied on missing
`inverse_of` causing fresh copies of a record to be loaded.

There's still one Rails test that wouldn't pass if we ran the whole
suite with `automatic_scope_inversing = true`. It's related to
`TaggedPost`, which changes the polymorphic type from the base class
`Post` to the subclass `TaggedPost`.

```rb
class TaggedPost < Post
  has_many :taggings, -> { rewhere(taggable_type: "TaggedPost") }, as: :taggable
end
```

Setting the inverse doesn't work because it ends up changing the type
back to `Post`, something like this:

```rb
post = TaggedPost.new
tagging = post.taggings.new
puts tagging.taggable_type
=> TaggedPost

tagging.taggable = post
puts tagging.taggable_type
=> Post
```

I think this is an acceptable change, given that it's a fairly specific
scenario, and is sort of at odds with the way polymorphic associations
are meant to work (they are meant to contain the base class, not the
subclass). If someone is relying on this specific behavior they can
still either keep `automatic_scope_inversing` set to false, or they can
add `inverse_of: false` to the association.

I haven't found any other cases where having the `inverse_of` would
cause problems like this.

Co-authored-by: Chris Bloom <chrisbloom7@gmail.com>

Remove message from ActiveRecord::Rollback example

Do not suggest that raising `ActiveRecord::Rollback` exception should have a message.

There's no point of adding a message to `ActiveRecord::Rollback` exception because it will be rescued by transaction block and the message will be lost.

Use queue_classic branch which works on psql 14

This fixes the failing CI for ActiveJob.

This uses my fork, we can switch back when the following PR is merged:

    QueueClassic/queue_classic#334

Add ability to lazily load the schema cache on connection

This adds a configuration option and code to enable lazy loading of the
schema cache on the connection. If
`config.active_record.lazily_load_schema_cache` is set to `true` then
the Railtie will be completely skipped and the schema cache will be
loaded when the connection is established rather than on boot.

We use this method at GitHub currently in our custom adapter. It enables
us to load schema caches lazily. It also is a solution for schema caches
with multiple databases. The Railtie doesn't know about the other
connections _before_ boot so it can only load the cache for
`ActiveRecord::Base.connection`. Since this loads the cache on
`establish_connection` Rails doesn't need to know anything special about
the connections.

Applications can continue dumping the cache like normal, the change is
only to how the schema cache is loaded. To use the lazy loaded schema
cache a `schema_cache_path` must be set in the database configuration,
otherwise `db/schema_cache.yml` will be used.

Followup questions:

1) Should we deprecate the Railtie?
2) The Railtie does more work than we're doing here because it checks
the version against the current version. I'm not sure we really want
to do this in Rails - we don't do it in ours at GitHub.

Replace ableist language

The word "Crazy" has long been associated with mental illness. While
there may be other dictionary definitions, it's difficult for some of us
to separate the word from the stigmatization, gaslighting, and bullying
that often comes along with it.

This commit replaces instances of the word with various alternatives. I
find most of these more focused and descriptive than what we had before.

Remove "matches?" from AS::N subscriber classes

This was not being used anymore

Move AllMessages behaviour into Matcher

This avoids needing to delegate all methods to the actual subscriber
class and we can deal just with the message name matching behaviour.

Don't require role when passing shard to connected_to

Originally we required `role` when switching `shard`'s because I felt it
made it less confusing. However now that we're exploring some more
multi-tenancy work I agree we need to move this condition. Otherwise if
you're using one middleware to switch roles and another to switch
shards, you may be forced to pass the role around in places you're only
concerned with shard. This also simplifies the call for applications
that don't use roles and only have writer shards.

chore: squash commits

refactor: include url helpers

Replace more ableist language

Along the same lines as ccb3cb5, this commit removes unnecessary
references to mental health.

As in that commit, I think many of these are more descriptive than what
we had before.

The commit changes only tests and documentation.

Remove refenrence to destroy_all_in_batches config

This feature was reverted.

refactor: fix test

fix(rails console): change session host to  application default url options host
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

No branches or pull requests

3 participants