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

Implement helpers proxy in controller instance level #24866

Merged
merged 2 commits into from May 5, 2016

Conversation

Projects
None yet
6 participants
@rafaelfranca
Member

rafaelfranca commented May 5, 2016

It is a common pattern in the Rails community that when people want to
use any kind of helper that is defined inside app/helpers they includes
the helper module inside the controller like:

module UserHelper
  def my_user_helper
    # ...
  end
end

class UsersController < ApplicationController
  include UserHelper

  def index
    render inline: my_user_helper
  end
end

This has problem because the helper can't access anything that is
defined in the view level context class.

Also all public methods of the helper become available in the controller
what can lead to undesirable methods being routed and behaving as
actions.

Also if you helper depends on other helpers or even Action View helpers
you need to include each one of these dependencies in your controller
otherwise your helper is not going to work.

We already have a helpers proxy at controller class level but that proxy
doesn't have access to the instance variables defined in the
controller.

With this new instance level helper proxy users can reuse helpers in the
controller without having to include the modules and with access to
instance variables defined in the controller.

class UsersController < ApplicationController
  def index
    render inline: helpers.my_user_helper
  end
end
Move protected instance variable to the right place
There were a lot of protected instance variables in
AbsctractController::Rendering that were related to Action Controller
and Action View.

Moving to ActionController::Base's protected instance list we make it
closer to where they are really defined.
@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 5, 2016

Member

@jeremy @matthewd @sgrif do you have opinions about this?

Member

rafaelfranca commented May 5, 2016

@jeremy @matthewd @sgrif do you have opinions about this?

@kaspth

View changes

Show outdated Hide outdated actionpack/test/controller/helper_test.rb
@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth May 5, 2016

Member

LGTM and like the idea.

Should we make this available for Action Mailer too?

Member

kaspth commented May 5, 2016

LGTM and like the idea.

Should we make this available for Action Mailer too?

@maclover7 maclover7 added the actionpack label May 5, 2016

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 5, 2016

Member

Should we make this available for Action Mailer too?

Yeah, that makes sense. Neither of them are available in mailers.

Member

rafaelfranca commented May 5, 2016

Should we make this available for Action Mailer too?

Yeah, that makes sense. Neither of them are available in mailers.

@rafaelfranca

This comment has been minimized.

Show comment
Hide comment
@rafaelfranca

rafaelfranca May 5, 2016

Member

So, after checking mailers helpers don't use app/helpers at all, only if they are asked. That said, I'll implement only in the controller.

Member

rafaelfranca commented May 5, 2016

So, after checking mailers helpers don't use app/helpers at all, only if they are asked. That said, I'll implement only in the controller.

Implement helpers proxy in controller instance level
It is a common pattern in the Rails community that when people want to
:xa
use any kind of helper that is defined inside app/helpers they includes
the helper module inside the controller like:

    module UserHelper
      def my_user_helper
        # ...
      end
    end

    class UsersController < ApplicationController
      include UserHelper

      def index
        render inline: my_user_helper
      end
    end

This has problem because the helper can't access anything that is
defined in the view level context class.

Also all public methods of the helper become available in the controller
what can lead to undesirable methods being routed and behaving as
actions.

Also if you helper depends on other helpers or even Action View helpers
you need to include each one of these dependencies in your controller
otherwise your helper is not going to work.

We already have a helpers proxy at controller class level but that proxy
doesn't have access to the instance variables defined in the
controller.

With this new instance level helper proxy users can reuse helpers in the
controller without having to include the modules and with access to
instance variables defined in the controller.

    class UsersController < ApplicationController
      def index
        render inline: helpers.my_user_helper
      end
    end

@arthurnn arthurnn merged commit 58a2212 into rails:master May 5, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@arthurnn

This comment has been minimized.

Show comment
Hide comment
@arthurnn

arthurnn May 5, 2016

Member

❤️

Member

arthurnn commented May 5, 2016

❤️

prathamesh-sonpatki added a commit to prathamesh-sonpatki/rails that referenced this pull request May 6, 2016

@kaspth

This comment has been minimized.

Show comment
Hide comment
@kaspth

kaspth May 6, 2016

Member

Sweet ❤️

Member

kaspth commented May 6, 2016

Sweet ❤️

kaspth added a commit that referenced this pull request May 6, 2016

Merge pull request #24881 from prathamesh-sonpatki/add-24866-to-relae…
…se-notes

[ci skip] Release notes: Add PR #24866 to release notes
@diegorv

This comment has been minimized.

Show comment
Hide comment
@diegorv

diegorv commented May 11, 2016

❤️

# Provides a proxy to access helpers methods from outside the view.
def helpers
@_helper_proxy ||= view_context

This comment has been minimized.

@Sporky023

Sporky023 Oct 24, 2016

Just curious - why not use view_context.some_helper_method in the controller instead of helpers.some_helper_method?

@Sporky023

Sporky023 Oct 24, 2016

Just curious - why not use view_context.some_helper_method in the controller instead of helpers.some_helper_method?

This comment has been minimized.

@rafaelfranca

rafaelfranca Oct 25, 2016

Member

Because view_context is not cached and it working view_context is just an implementation detail that I plan to change some point in the future.

@rafaelfranca

rafaelfranca Oct 25, 2016

Member

Because view_context is not cached and it working view_context is just an implementation detail that I plan to change some point in the future.

@rafaelfranca rafaelfranca deleted the rafaelfranca:actionview-helpers branch Oct 25, 2016

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