Fix docs for ActionController::Metal#headers

[skipkayhil]

Summary

This documentation was correct when it was written in 6e75455, however
headers has moved a few times since:

• added to ActionController::Http in 216309c as part of the new_base
• Http was renamed to Metal in 52798fd
• headers was changed from an independent hash to a delegation in
  51c7ac1 and 54becd1

Added docs for Metal#request, Metal#response, and Metal#headers that can
be linked to from Response. The recommendation to use Metal delegation
methods instead of methods on Response was also removed due to a number of
docs/guides demonstrating the opposite.

Other Information

Running CI per feedback in #45347 (comment)

I've noticed that this comment recommends using headers, status, etc. instead of response.*, but the examples in Action Controller Overview seem to use response.headers:

rails/guides/source/action_controller_overview.md

Line 907 in 4f6f3c9

response.headers["Content-Type"] = "application/pdf"

Should this comment or the examples be changed to be consistent? Updated to remove this recommendation per discussion

[jonathanhefner]

I've noticed that this comment recommends using headers, status, etc. instead of response.*, but the examples in Action Controller Overview seem to use response.headers:
Should this comment or the examples be changed to be consistent?

I also just noticed this example (added by c83e147) too. Based on the relative age of that example and the example in the Action Controller guide (present in e56b3e4), I would say the ship has sailed, and we should remove the "should never be used directly in controllers" admonition.

And if we do that, we should probably concentrate the documentation under ActionDispatch::Response#headers, and then document ActionController::Metal#headers as simply "Delegates to ActionDispatch::Response#headers." What do you think?

[skipkayhil]

I would say the ship has sailed, and we should remove the "should never be used directly in controllers" admonition.

rails/actionpack/lib/action_controller/metal.rb

Line 145 in bdc3e19

attr_internal :response, :request

After looking to link to #response, I found its not actually documented because its attr_internal... should it (and #request) be documented and/or not attr_internal?)

[jonathanhefner]

After looking to link to #response, I found its not actually documented because its attr_internal... should it (and #request) be documented and/or not attr_internal?)

I think it would make sense to document them as well. Perhaps something simple like "Returns an ActionDispatch::Response object for the current request."

As for attr_internal, it simply prefixes the ivar name with _ (see attr_internal_accessor). To the best of my knowledge, it doesn't have any semantic meaning regarding the public API.

[skipkayhil]

This is currently being documented as a method, but we could also mark it as an attribute which may be more accurate?

Suggested change

	##
	# Returns the ActionDispatch::Response object for the current request
	attr_internal :response
	##
	# :attr_accessor: response
	# Returns the ActionDispatch::Response object for the current request
	attr_internal :response

[jonathanhefner]

Yes, perhaps so. The downside is that attributes don't show up when searching in the rendered docs sidebar. Also, attributes are linkable in the sense that references to them will become links, but attributes aren't rendered with destination anchors, so the links just go to the top of the page. I'm not sure whether those are RDoc issues or SDoc issues, though.

[jonathanhefner]

After looking to link to #response, I found its not actually documented because its attr_internal... should it (and #request) be documented and/or not attr_internal?)

I hadn't actually looked, so I did not notice, but request and response are documented in ActionController::Base. Though they were documented by 2919f0d, which predates 51c7ac1. I debated whether we should prefer ActionController::Base or ActionController::Metal (or both) for documenting these things. I ended up choosing ActionController::Metal because I feel like it makes the documentation more self-consistent, but it's not a strong preference.

To get this over the finish line, I pushed a new commit. If you think it looks good, I can squash and merge. Here's what I did:

• Remove the documentation for ActionController::Base#request and ActionController::Base#response.

• Add :attr_reader: to ActionController::Metal#request and ActionController::Metal#response, as per your suggestion.

• Remove the documentation for ActionController::Metal#session. It tried to link to ActionDispatch::Request#session but session is implemented by Rack, so the link did not work.

• Some very minor tweaks to phrasing. In particular, I tried many different ways to split the documentation for ActionDispatch::Response#header and #headers (to separate the examples). But RDoc seems to ignore all directives when it sees alias_method, so I settled for the following, though I'm not sure it's actually better:

  Before	After

[skipkayhil]

• It tried to link to ActionDispatch::Request#session but session is implemented by Rack, so the link did not work.

🤦 that's what I get for scope creeping haha, I guess I forgot to check that one

• But RDoc seems to ignore all directives when it sees alias_method, so I settled for the following, though I'm not sure it's actually better:

Yep, I went through the same things. I like the specific mention of the alias, I think this is probably better

• Add :attr_reader: to ActionController::Metal#request and ActionController::Metal#response, as per your suggestion.

Just double checking, I think there is a writer, but I'm not sure we should be telling people they can use it

Other than that I think everything looks good 👍

[jonathanhefner]

Just double checking, I think there is a writer, but I'm not sure we should be telling people they can use it

Yes, that is my thinking as well.

Thank you, @skipkayhil! 👋