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

Broken in Rails 7 #79

Open
sasharevzin opened this issue Aug 16, 2022 · 5 comments · May be fixed by #80
Open

Broken in Rails 7 #79

sasharevzin opened this issue Aug 16, 2022 · 5 comments · May be fixed by #80

Comments

@sasharevzin
Copy link

It looks like some of methods no longer exist in Rails 7

NoMethodError (undefined method `action_has_layout=' for
@fatkodima
Copy link
Member

@sasharevzin I am not able to reproduce this. Can your share some repro?

@sasharevzin
Copy link
Author

Hi @fatkodima,

Here the repo.
So looks like cache_configured? is always false even though I enabled it.

PS. Issue with action_has_layout might be not related and happened because I tried to debug gem.

@fatkodima
Copy link
Member

fatkodima commented Sep 8, 2022

I made some investigation.
This is not a regression in Rails 7.0, it was not working before for api_only apps (see #46, you can also find a working or semi-working solution there).

From the repro app:

class HomeController < ApplicationController
  include ActionController::Caching

  caches_action :index

  def index
    render json: { time: Time.current }
  end
end

The problem is that you included ActionController::Caching in HomeController, but that is not working properly.

After the source code was parsed, it is executed as the following:

  1. class HomeController < ApplicationController
  2. class ApplicationController < ActionController::API
  3. the rails' source file for ActionController::API kicks in and the following line is executed https://github.com/rails/rails/blob/bee6167977e601e6b690703de3be643bbdd88b38/actionpack/lib/action_controller/api.rb#L148
  4. the configs are set in https://github.com/rails/rails/blob/bee6167977e601e6b690703de3be643bbdd88b38/actionpack/lib/action_controller/railtie.rb#L67-L92, including perform_caching and cache_store, but only for ActionController::Base
  5. include ActionController::Caching is executed, but it is too late, because the step 4 was already executed

So, the proposed solution (without changing rails'):

diff --git a/lib/action_controller/action_caching.rb b/lib/action_controller/action_caching.rb
index aee19c1..5a2a243 100644
--- a/lib/action_controller/action_caching.rb
+++ b/lib/action_controller/action_caching.rb
@@ -13,3 +13,12 @@ module ActionController
 end

 ActionController::Base.send(:include, ActionController::Caching::Actions)
+
+unless ActionController::API.include?(ActionController::Caching)
+  ActionController::API.class_eval do
+    include ActionController::Caching
+
+    self.perform_caching = ActionController::Base.perform_caching
+    self.cache_store = ActionController::Base.cache_store
+  end
+end
diff --git a/lib/action_controller/caching/actions.rb b/lib/action_controller/caching/actions.rb
index a45c801..4e05556 100644
--- a/lib/action_controller/caching/actions.rb
+++ b/lib/action_controller/caching/actions.rb
@@ -159,9 +159,9 @@ module ActionController
           body = controller.read_fragment(cache_path.path, @store_options)

           unless body
-            controller.action_has_layout = false unless cache_layout
+            controller.action_has_layout = false if !cache_layout && controller.respond_to?(:action_has_layout=)
             yield
-            controller.action_has_layout = true
+            controller.action_has_layout = true if controller.respond_to?(:action_has_layout=)
             body = controller._save_fragment(cache_path.path, @store_options)
           end

Or we can include ActionController::Caching into ActionController::API in Rails itself and keep only controller.respond_to?(:action_has_layout=) changes from the diff above.

@rafaelfranca Can you give some input on this?

@rafaelfranca
Copy link
Member

I think for the first part of the diff (including ActionController::Caching) we should not do in this library. If people want to use API with this gem we should document they would need to do that.

The second part of the diff is correct, but maybe we should only check if the method exists once and if not we just yield and set the body.

@fatkodima fatkodima linked a pull request Sep 8, 2022 that will close this issue
@sasharevzin
Copy link
Author

Thank you, @fatkodima. PR looks good to me (can't merge not the owner) 😢

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

Successfully merging a pull request may close this issue.

3 participants