Avoid creating controller _helper modules until modification

[jhawthorn]

In applications which use :all helpers (the default), most controllers won't be making modifications to their _helpers module.

In CRuby this created many ICLASS objects which could cause a large increase in memory usage in applications with many controllers and helpers.

To avoid creating unnecessary modules this PR builds modules only when a modification is being made: ethier by calling helper, helper_method, or through having a default helper (one matching the controller's name) included onto it.

[jhawthorn]

I double checked that even if we build view_context_class on every controller, this provides a pretty substantial reduction in T_ICLASS (and maybe a good follow up would be investigating if we could inherit view_context_class in some cases).

[benoittgt]

Hello @jhawthorn

Is it possible to still call previous behavior explicitly?

We have an issue on rspec-rails rspec/rspec-rails#2481 that is linked to that change. We define the helper method in RSpec, but behind _views is defined here

rails/actionview/lib/action_view/test_case.rb

Lines 206 to 218 in 5720165

	# The instance of ActionView::Base that is used by +render+.
	def view
	@view ||= begin
	view = @controller.view_context
	view.singleton_class.include(_helpers)
	view.extend(Locals)
	view.rendered_views = rendered_views
	view.output_buffer = output_buffer
	view
	end
	end
	alias_method :_view, :view

I tried to play with your patch to fix the issue on the rspec-rails side but I was not able to find the issue.

[jhawthorn]

Hmm. The pattern linked in that issue looks a bit odd.

helper.controller.class.include(ConfigurationMethods), where including ConfigurationMethods calls helper method. We probably shouldn't make modifications to the controller like that, prior to #40125 (which possibly also is the cause of the observed change) there was only one TestController used by all tests so definitely not something that modifications should have been made to (the modifications would leak across all tests).

If you want the pre-6.1 behaviour I think you could achieve this include directly on ActionView::TestCase::TestController (which is what that line did before) at the start of the suite... but I really don't recommend that 😓.

I suspect the reason this pattern doesn't pass now is that the helper/view context are being instantiated before the controller includes are performed.

I don't think the example in issue is a regression (but I'm not that confident about that 😅). If there was an example which passed in Rails 6.0, didn't modify global state, and now fails on Rails 6.1 I'd love to dig into it ❤️.

[M-Sayed]

I don't think the example in issue is a regression (but I'm not that confident about that 😅). If there was an example which passed in Rails 6.0, didn't modify global state, and now fails on Rails 6.1 I'd love to dig into it ❤️.

@jhawthorn I'm using Devise for RailsAdmin with an API-only Rails app, after I upgraded to v6.1.7.3, I started getting the following error because Devise helpers are not available in the view, I assume the issue is due to the changes in this PR

error:

ActionView::Template::Error:
  undefined local variable or method `resource' for #<ActionView::Base:0x0000000003a4a8>

controller:

class Api::V1::AdminsSessionsController < Devise::SessionsController
  include ::ActionView::Layouts
  layout "admin"
end

view:

<h2>Log in</h2>

<%= form_for(resource, as: resource_name, url: session_path(resource_name)) do |f| %>
  <div class="field">
    <%= f.label :email %><br />
    <%= f.email_field :email, autofocus: true, autocomplete: "email" %>
  </div>

  <div class="field">
    <%= f.label :password %><br />
    <%= f.password_field :password, autocomplete: "current-password" %>
  </div>

  <% if devise_mapping.rememberable? %>
    <div class="field">
      <%= f.check_box :remember_me %>
      <%= f.label :remember_me %>
    </div>
  <% end %>

  <div class="actions">
    <%= f.submit "Log in" %>
  </div>
<% end %>

<%= render "devise/shared/links" %>

rspec file:

require "rails_helper"

describe "Api::V1::AdminsSessionsController", type: :request do
  describe "#new" do
    context "when format is html" do
      let(:format) { "html" }

      it "renders :ok reponse" do
        get "/admin/sign_in.#{format}"

        expect(response).to have_http_status(:ok)
      end
    end
  end
end

[skipkayhil]

Hey @M-Sayed, can you create a new issue with a full reproduction repo if you're seeing a regression? Commenting on a 2 year old PR will likely not get much attention

[denys281]

It seems that the issue is still present.