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

Fix autoloading issue with ActiveStorage::Downloader #34253

Merged
merged 1 commit into from Oct 28, 2018

Conversation

Projects
None yet
6 participants
@haberbyte
Contributor

haberbyte commented Oct 18, 2018

There is currently an issue in local development that cause the following error:

  NameError (uninitialized constant ActiveStorage::Downloader):

  bootsnap (1.3.2) lib/bootsnap/load_path_cache/core_ext/active_support.rb:74:in `block in load_missing_constant'
  bootsnap (1.3.2) lib/bootsnap/load_path_cache/core_ext/active_support.rb:8:in `without_bootsnap_cache'
  bootsnap (1.3.2) lib/bootsnap/load_path_cache/core_ext/active_support.rb:74:in `rescue in load_missing_constant'
  bootsnap (1.3.2) lib/bootsnap/load_path_cache/core_ext/active_support.rb:56:in `load_missing_constant'
  rails (134dab46e4e9) activestorage/app/models/active_storage/blob.rb:196:in `open'

This seems to happen whenever I change some models in my app that make use of ActiveStorage and try to download a new representation of a file.

Not sure, maybe it is related to this gotcha: https://edgeguides.rubyonrails.org/autoloading_and_reloading_constants.html#when-constants-aren-t-missed

In any case, I looked at where the ActiveStorage::Downloader class is used, and it is only used in "app/models/active_storage/blob.rb". Since the usage is in app/, moving it also into "app/models/" made sense to me.

This fixed the issue for me, and the Blob model now does not need to manually require "active_storage/downloader" anymore.

@rails-bot

This comment has been minimized.

rails-bot commented Oct 18, 2018

Thanks for the pull request, and welcome! The Rails team is excited to review your changes, and you should hear from @rafaelfranca (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

This repository is being automatically checked for code quality issues using Code Climate. You can see results for this analysis in the PR status below. Newly introduced issues should be fixed before a Pull Request is considered ready to review.

Please see the contribution instructions for more information.

@fxn

This comment has been minimized.

Member

fxn commented Oct 18, 2018

The funny thing is that classes in the framework are not autoloaded like classes in the application
do. (They may be technically lazy loaded via autoload, but not by AS::Dependencies).

EDIT: This remark is incorrect. Part of Active Storage does use Rails autoloading because it is an engine with models and other autoloadable stuff.

Could you come with a minimal example to reproduce?

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 18, 2018

Didn't know how to come up with something really minimal like a small script, but i created a minimal demo app here: https://github.com/haberbyte/activestorage_downloader_error

Steps to reproduce:

  1. clone the app, bundle install, db:create / db:migrate etc...
  2. rails s (should start puma in development)
  3. Go to http://localhost:3000/posts (i provided further instructions there)
  4. Create new post with an attached image
  5. Change app/models/hello_world.rb in the meantime
  6. Edit the post and re-upload a new image

=> Now i see the error whenever a view tries to get the image. Stops only after killing and restarting the server.

I hope you can follow the steps and make more sense of this than me :-D

Here is a full example stacktrace:

Started GET "/rails/active_storage/representations/eyJfcmFpbHMiOnsibWVzc2FnZSI6IkJBaHBFUT09IiwiZXhwIjpudWxsLCJwdXIiOiJibG9iX2lkIn19--77369ee7b0bd61cccc1c7a6e9ca3a128ab04f8a7/eyJfcmFpbHMiOnsibWVzc2FnZSI6IkJBaDdCam9TY21WemFYcGxYM1J2WDJacGRGc0hhUUhJYVFISSIsImV4cCI6bnVsbCwicHVyIjoidmFyaWF0aW9uIn19--514df84c2a014552d7b9fffa2206e68ee3f919be/Screenshot%202018-08-19%20at%2014.07.17.png" for 127.0.0.1 at 2018-10-18 20:08:31 +0200
Processing by ActiveStorage::RepresentationsController#show as PNG
  Parameters: {"signed_blob_id"=>"eyJfcmFpbHMiOnsibWVzc2FnZSI6IkJBaHBFUT09IiwiZXhwIjpudWxsLCJwdXIiOiJibG9iX2lkIn19--77369ee7b0bd61cccc1c7a6e9ca3a128ab04f8a7", "variation_key"=>"eyJfcmFpbHMiOnsibWVzc2FnZSI6IkJBaDdCam9TY21WemFYcGxYM1J2WDJacGRGc0hhUUhJYVFISSIsImV4cCI6bnVsbCwicHVyIjoidmFyaWF0aW9uIn19--514df84c2a014552d7b9fffa2206e68ee3f919be", "filename"=>"Screenshot 2018-08-19 at 14.07.17"}
  ActiveStorage::Blob Load (0.1ms)  SELECT "active_storage_blobs".* FROM "active_storage_blobs" WHERE "active_storage_blobs"."id" = ? LIMIT ?  [["id", 12], ["LIMIT", 1]]
  Disk Storage (0.0ms) Checked if file exists at key: variants/JKP87mMHvdiinNAq3VA2wHhM/46af080130400126400e76e7af8d921b13a8722d7d11677f19b338aa982c7c04 (no)
Completed 500 Internal Server Error in 4ms (ActiveRecord: 0.1ms | Allocations: 3704)



NameError (uninitialized constant ActiveStorage::Downloader):

bootsnap (1.3.2) lib/bootsnap/load_path_cache/core_ext/active_support.rb:74:in `block in load_missing_constant'
bootsnap (1.3.2) lib/bootsnap/load_path_cache/core_ext/active_support.rb:8:in `without_bootsnap_cache'
bootsnap (1.3.2) lib/bootsnap/load_path_cache/core_ext/active_support.rb:74:in `rescue in load_missing_constant'
bootsnap (1.3.2) lib/bootsnap/load_path_cache/core_ext/active_support.rb:56:in `load_missing_constant'
rails (885ab065b50e) activestorage/app/models/active_storage/blob.rb:196:in `open'
rails (885ab065b50e) activestorage/app/models/active_storage/variant.rb:99:in `process'
rails (885ab065b50e) activestorage/app/models/active_storage/variant.rb:67:in `processed'
rails (885ab065b50e) activestorage/app/controllers/active_storage/representations_controller.rb:12:in `show'
rails (885ab065b50e) actionpack/lib/action_controller/metal/basic_implicit_render.rb:6:in `send_action'
rails (885ab065b50e) actionpack/lib/abstract_controller/base.rb:196:in `process_action'
rails (885ab065b50e) actionpack/lib/action_controller/metal/rendering.rb:30:in `process_action'
rails (885ab065b50e) actionpack/lib/abstract_controller/callbacks.rb:42:in `block in process_action'
rails (885ab065b50e) activesupport/lib/active_support/callbacks.rb:132:in `run_callbacks'
rails (885ab065b50e) actionpack/lib/abstract_controller/callbacks.rb:41:in `process_action'
rails (885ab065b50e) actionpack/lib/action_controller/metal/rescue.rb:22:in `process_action'
rails (885ab065b50e) actionpack/lib/action_controller/metal/instrumentation.rb:34:in `block in process_action'
rails (885ab065b50e) activesupport/lib/active_support/notifications.rb:168:in `block in instrument'
rails (885ab065b50e) activesupport/lib/active_support/notifications/instrumenter.rb:23:in `instrument'
rails (885ab065b50e) activesupport/lib/active_support/notifications.rb:168:in `instrument'
rails (885ab065b50e) actionpack/lib/action_controller/metal/instrumentation.rb:32:in `process_action'
rails (885ab065b50e) actionpack/lib/action_controller/metal/params_wrapper.rb:256:in `process_action'
rails (885ab065b50e) activerecord/lib/active_record/railties/controller_runtime.rb:27:in `process_action'
rails (885ab065b50e) actionpack/lib/abstract_controller/base.rb:136:in `process'
rails (885ab065b50e) actionview/lib/action_view/rendering.rb:32:in `process'
rails (885ab065b50e) actionpack/lib/action_controller/metal.rb:191:in `dispatch'
rails (885ab065b50e) actionpack/lib/action_controller/metal.rb:252:in `dispatch'
rails (885ab065b50e) actionpack/lib/action_dispatch/routing/route_set.rb:51:in `dispatch'
rails (885ab065b50e) actionpack/lib/action_dispatch/routing/route_set.rb:33:in `serve'
rails (885ab065b50e) actionpack/lib/action_dispatch/journey/router.rb:52:in `block in serve'
rails (885ab065b50e) actionpack/lib/action_dispatch/journey/router.rb:35:in `each'
rails (885ab065b50e) actionpack/lib/action_dispatch/journey/router.rb:35:in `serve'
rails (885ab065b50e) actionpack/lib/action_dispatch/routing/route_set.rb:835:in `call'
rack (2.0.5) lib/rack/tempfile_reaper.rb:15:in `call'
rack (2.0.5) lib/rack/etag.rb:25:in `call'
rack (2.0.5) lib/rack/conditional_get.rb:25:in `call'
rack (2.0.5) lib/rack/head.rb:12:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/http/content_security_policy.rb:18:in `call'
rack (2.0.5) lib/rack/session/abstract/id.rb:232:in `context'
rack (2.0.5) lib/rack/session/abstract/id.rb:226:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/cookies.rb:682:in `call'
rails (885ab065b50e) activerecord/lib/active_record/migration.rb:560:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/callbacks.rb:28:in `block in call'
rails (885ab065b50e) activesupport/lib/active_support/callbacks.rb:98:in `run_callbacks'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/callbacks.rb:26:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/executor.rb:14:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/debug_exceptions.rb:69:in `call'
web-console (3.7.0) lib/web_console/middleware.rb:135:in `call_app'
web-console (3.7.0) lib/web_console/middleware.rb:30:in `block in call'
web-console (3.7.0) lib/web_console/middleware.rb:20:in `catch'
web-console (3.7.0) lib/web_console/middleware.rb:20:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/show_exceptions.rb:33:in `call'
rails (885ab065b50e) railties/lib/rails/rack/logger.rb:38:in `call_app'
rails (885ab065b50e) railties/lib/rails/rack/logger.rb:26:in `block in call'
rails (885ab065b50e) activesupport/lib/active_support/tagged_logging.rb:80:in `block in tagged'
rails (885ab065b50e) activesupport/lib/active_support/tagged_logging.rb:28:in `tagged'
rails (885ab065b50e) activesupport/lib/active_support/tagged_logging.rb:80:in `tagged'
rails (885ab065b50e) railties/lib/rails/rack/logger.rb:26:in `call'
sprockets-rails (3.2.1) lib/sprockets/rails/quiet_assets.rb:13:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/remote_ip.rb:81:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/request_id.rb:27:in `call'
rack (2.0.5) lib/rack/method_override.rb:22:in `call'
rack (2.0.5) lib/rack/runtime.rb:22:in `call'
rails (885ab065b50e) activesupport/lib/active_support/cache/strategy/local_cache_middleware.rb:29:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/executor.rb:14:in `call'
rails (885ab065b50e) actionpack/lib/action_dispatch/middleware/static.rb:126:in `call'
rack (2.0.5) lib/rack/sendfile.rb:111:in `call'
webpacker (3.5.5) lib/webpacker/dev_server_proxy.rb:22:in `perform_request'
rack-proxy (0.6.5) lib/rack/proxy.rb:57:in `call'
rails (885ab065b50e) railties/lib/rails/engine.rb:524:in `call'
puma (3.12.0) lib/puma/configuration.rb:225:in `call'
puma (3.12.0) lib/puma/server.rb:658:in `handle_request'
puma (3.12.0) lib/puma/server.rb:472:in `process_client'
puma (3.12.0) lib/puma/server.rb:332:in `block in run'
puma (3.12.0) lib/puma/thread_pool.rb:133:in `block in spawn_thread'
^C- Gracefully stopping, waiting for requests to finish
=== puma shutdown: 2018-10-18 20:08:36 +0200 ===
- Goodbye!
Exiting
@fxn

This comment has been minimized.

Member

fxn commented Oct 19, 2018

@haberbyte perfect mini application, could reproduce.

@fxn

This comment has been minimized.

Member

fxn commented Oct 19, 2018

I believe this is a bug in dependencies. Let me explain.

That require is correct. ActiveStorage::Downloader is implemented in activestorage/lib/active_storage/downloader.rb. The lib directory of an engine or Rails application does not belong to autoload_paths, and to load code from that directory you use require.

The problem I notice is that although that constant is not autoloaded, dependencies believes the constant has been autoloaded. You can throw

before_action -> { logger.debug(ActiveSupport::Dependencies.autoloaded_constants) }

in PostsController, you'll see it listed. Since dependencies believes it was autoloaded, it wipes the constant when files change. But since the constant is not autoloadable and the require won't execute twice for the same file, that class is gone.

The bug is that dependencies believes the constant was autoloaded, but it wasn't.

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 20, 2018

Oh wow, thanks for that explanation, makes sense.

I am struggling to understand how ActiveSupport::Dependencies works.
The WatchStack just returns newly added constants, so when ActiveStorage::Blob gets autoloaded it would require the Downloader, which then is a new constant on the ActiveStorage module.

When Dependencies loads a file:

# lib/active_support/dependencies.rb
    def load_file(path, const_paths = loadable_constants_for_path(path))
      const_paths = [const_paths].compact unless const_paths.is_a? Array
      parent_paths = const_paths.collect { |const_path| const_path[/.*(?=::)/] || ::Object }

      result = nil
      newly_defined_paths = new_constants_in(*parent_paths) do
        result = Kernel.load path
      end
      autoloaded_constants.concat newly_defined_paths unless load_once_path?(path)
      autoloaded_constants.uniq!
      result
    end

Kernel.load will load "/activestorage/app/models/active_storage/blob.rb", which requires stuff from lib/ and therefore adds a new constant to ActiveStorage.

Newly defined paths now gives: newly_defined_paths: ["ActiveStorage::LogSubscriber", "ActiveStorage::Downloader", "ActiveStorage::Blob"]

But Downloader (and actually LogSubscriber as well) is in a path outside of the autoload paths, so this should be filtered out before adding it to autoloaded_constants I guess.

@fxn

This comment has been minimized.

Member

fxn commented Oct 20, 2018

Exactly, good analysis.

This works well routinely, for example, you can require "nokogiri" in an autoloaded file, and Nokogiri is correctly not flagged as being autoloaded.

Without looking at the code, I suspect the watch stack manager or someone involved in deciding what has been autoloaded might be incorrectly assuming something about being in a different namespace.

@fxn

This comment has been minimized.

Member

fxn commented Oct 20, 2018

I have reproduced with an even smaller application that doesn't use Active Storage.

There, we have app/models/admin/user.rb, which requires lib/admin/utils.rb. The former is autoloadable, the latter is not.

To demonstrate the problem we have foo.rb in the root directory:

Admin::User

p ActiveSupport::Dependencies.autoloaded_constants

that you need to execute with bin/rails runner. The ouput is

["Admin", "Admin::User", "Admin::Utils"]

Admin::Utils should not be there.

@fxn

This comment has been minimized.

Member

fxn commented Oct 20, 2018

Just realized, though, that Admin is autoloaded. Therefore, should be deleted on reload, and that is going to delete the child Admin::Utils no matter what.

Perhaps that is saying this is an inherent limitation of Rails autoload more than a bug. In which case, the approach to solve it could be different, like documenting it, rewriting Active Storage directories, etc.

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 20, 2018

I just realized that even things like the AS errors, e.g. ActiveStorage::UnpreviewableError that is raised in app/ should then be affected by the same autoloading issues.
So moving all of that into app/ seems weird.

With rewriting directories, do you mean having a different module name in lib than in app?

Just thinking out loud, but couldn't rails autoloader be extended to keep track of what constants have been added by an autoload, but are not part of an autoload path?
Then a reload needs to check that it does not delete those.

Wow this gets complicated ;-)

@georgeclaghorn

This comment has been minimized.

Member

georgeclaghorn commented Oct 20, 2018

Just realized, though, that Admin is autoloaded. Therefore, should be deleted on reload, and that is going to delete the child Admin::Utils no matter what.

Is this true for ActiveStorage, which is defined in lib/active_storage.rb?

Perhaps that is saying this is an inherent limitation of Rails autoload more than a bug. In which case, the approach to solve it could be different, like documenting it, rewriting Active Storage directories, etc.

Would require_dependency instead of require fix, since the former reloads as necessary?

@geoffgarside

This comment has been minimized.

Contributor

geoffgarside commented Oct 21, 2018

@fxn dude why me?

@fxn

This comment has been minimized.

Member

fxn commented Oct 21, 2018

@fxn dude why me?

Bad completion selection, sorry! I have deleted the comment.

@fxn

This comment has been minimized.

Member

fxn commented Oct 21, 2018

@georgeclaghorn Active Storage is indeed not affected by that gotcha with the root namespace and it is doing things correctly, because the ActiveStorage module gets defined when active_storage.rb is loaded. In the example above, I have pushed a change to foo.rb so that the Admin module is defined instead of autoloaded, and then the autoloaded constants are

["Admin::User", "Admin::Utils"]

There it is, Admin::Utils should not be marked as autoloaded.

I believe require_dependency would not be a good idea. You use that when you want to force loading something autoloadable. But in this case, ActiveStorage::Downloader is not something we want to autoload. The require is correct. Dependencies is the want to be fixed I believe.

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 21, 2018

The docs on top of the WatchStack class in Dependencies states:

...
# If child.rb is being autoloaded, its constants will be added to
# autoloaded_constants. If it was being required, they will be discarded.

Well, the discard is not really working in this case. And I don't see how this should work.

When the WatchStack is called to watch the "Admin" namespace, it keeps track of the original constants, so that it can know which ones are new. (In this case, original constants is empty since Admin is an empty module so far).

But require_or_load does not update those original constants, so the WatchStack has no way to discard the Admin::Utils class if the require happens during the load of the autoloaded constant Admin::User.

@rails-bot rails-bot bot added the activesupport label Oct 21, 2018

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 21, 2018

@fxn I quite honestly have little idea what I am doing, but after a long run of puts-debugging I pushed a possible fix.

If you try your demo-app with this branch of rails, then you should get:

$ rails runner foo.rb
["Admin::User"]
@fxn

This comment has been minimized.

Member

fxn commented Oct 21, 2018

@haberbyte Bravo! Digging into dependencies.rb demonstrates courage 😄.

I need to do some work to validate the patch, but my first impression is that it could be correct. Would you like to provide test coverage?

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 22, 2018

I can try to add a test for this, might take a me a while, but I guess there are enough other tests to take a look how it should be done ;)

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 23, 2018

@fxn I added a test that fails without my fix and passes with it. Hope the implementation is somehow ok, happy to improve as needed.

@fxn

This comment has been minimized.

Member

fxn commented Oct 23, 2018

Awesome @haberbyte. I'll write back!

@fxn

This comment has been minimized.

Member

fxn commented Oct 27, 2018

Hey! I believe the patch is good, only don't quite see a way in which

Dependencies.constant_watch_stack.watching.last

could be nil, given that code only runs if

Dependencies.constant_watch_stack.watching?

which means that watching is not empty, and as far as I can tell that array cannot contain nils.

Did you add || Object just in case to play safe?

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 27, 2018

Yes. I added that just to play safe and since it was used before this patch by default.

@fxn

This comment has been minimized.

Member

fxn commented Oct 27, 2018

OK, I believe it is not needed. Could you please remove it? In the case I missed something I prefer to be proved wrong with a regression and understand why.

@fxn

This comment has been minimized.

Member

fxn commented Oct 28, 2018

Nothing like sleeping on a problem. Unfortunately, I have found a simple case that is broken by this change:

$ cat app/models/post.rb 
class Post
  Admin::User
end

$ cat app/models/admin/user.rb 
require "const_from_lib"

module Admin
  class User
  end
end

$ cat lib/const_from_lib.rb 
ConstFromLib = 1

Now:

$ bin/rails r 'Post; p ActiveSupport::Dependencies.autoloaded_constants'
["Admin", "Admin::User", "Post", "ConstFromLib"]

As you see, ConstFromLib is incorrectly marked as autoloaded.

The problem is that when you are autoloading Post, the code stores the constants existing in its parent namespace: Object. By adding one indirection, we make dependencies discard new constants in Admin, but when going up the stack ConstFromLib appears to be autoloaded.

OK, this matches my intuition above: there is code assuming things about namespaces. My hypothesis is that when Object was hard-coded, the author was thinking about a use case in which the constants being autoloaded were top-level constants.

I believe the correct patch is

descs = Dependencies.constant_watch_stack.watching.flatten.uniq

That is, ignore anything loaded in any of the watched namespaces.

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 28, 2018

Ah wow, nice catch. Should I try to add your example as another test case and try your suggested patch with it?

I still need to take some time to truely understand this, my first patch attempt was just my best guess. Thanks for taking the time to look into this, I'm learning alot :-)

@fxn

This comment has been minimized.

Member

fxn commented Oct 28, 2018

@haberbyte that would be great, let's add yours and mine use cases to the test suite, and let's merge.

Not only is autoloading tricky and difficult to follow when you go pass the golden path and get into the details the actual implementation has to account for, but in addition dependencies.rb needs a deep refactor. The current code is showing its age and it is difficult to follow. I believe naming could be a lot better for starters.

So congrats on digging into it, the original patch was maybe not the final solution, but you identified the suspicious spot, which is awesome. So congrats on the effort ❤️.

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 28, 2018

OK i again added a failing test and made sure it fails with the current implementation, and is fixed by the change you suggested.

I find it incredibly hard to even name the test-case.
And if this is to be merged, how to name the commit xD

@fxn

This comment has been minimized.

Member

fxn commented Oct 28, 2018

The commit message could be something like this:

Improve the logic that detects non-autoloaded constants

If you require `nokogiri` from `app/models/user.rb`, dependencies.rb does not
mark `Nokogiri` as an autoloaded constant, as expected. But the logic to detect
these non-autoloaded constants is incomplete. See the tests defined in the patch
for some cases incorrectly handled.

What do you think?

@fxn

This comment has been minimized.

Member

fxn commented Oct 28, 2018

Oh, didn't say it before, but I guess we could rebase, and leave one single commit with (for example) that commit message.

@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 28, 2018

Yup, will do that once I’m back home. I like that message, thank you!

Improve the logic that detects non-autoloaded constants
If you require `nokogiri` from `app/models/user.rb`, dependencies.rb
does not mark `Nokogiri` as an autoloaded constant, as expected.
But the logic to detect these non-autoloaded constants is incomplete.
See the tests defined in the patch for some cases incorrectly handled.
@haberbyte

This comment has been minimized.

Contributor

haberbyte commented Oct 28, 2018

Rebased and just used your message. I am wondering if the sentence is true though, does it really work for nokogiri, or is that just because nokogiri tends to be required before autoloading kicks in?
It doesn't make sense to me that nokogiri is an exception to this?

@fxn

This comment has been minimized.

Member

fxn commented Oct 28, 2018

It has worked for Nokogiri since forever (Nokogiri being just an example of a 3rd party file).

require has always done that, Dependecies knows it has entered a stack level in which new constants should not be considered to be autoloaded, and apps use require regularly.

But the examples we’ve seen say the support was not sufficient.

The message wants to explain which is the feature we are talking about.

I am going to merge, later at home I’ll add an entry in the CHANGELOG and backport to 5.2.

@fxn fxn merged commit 7d7372c into rails:master Oct 28, 2018

2 checks passed

codeclimate All good!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@haberbyte haberbyte deleted the haberbyte:fix_autoload_on_activestorage_downloader branch Oct 28, 2018

fxn added a commit that referenced this pull request Oct 28, 2018

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Oct 30, 2018

@fxn is this safe to backport?

@fxn

This comment has been minimized.

Member

fxn commented Oct 30, 2018

I think so, the logic is the same as the one in master.

I backported because anyone using Active Storage with 5.2 may be affected by this bug (that was the issue that originated this fix).

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Oct 30, 2018

Ok. I didn't see the backport commit here so this is why I asked. I Agree we should backport.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment