Skip to content

Add top level rails application url - #42520

Open
aka47 wants to merge 15 commits into
rails:mainfrom
aka47:add-toplevel-rails-application-url
Open

Add top level rails application url#42520
aka47 wants to merge 15 commits into
rails:mainfrom
aka47:add-toplevel-rails-application-url

Conversation

@aka47

@aka47 aka47 commented Jun 17, 2021

Copy link
Copy Markdown
Contributor

Fixes #39566

Other Information

The http/request.rb object is a complicated one - with around 6 different modules mixed in - Rack::Helpers, Rack::Env and others. I renamed ActionDispatch::Http::URL to ActionDispatch::Http::RequestURL as it is mainly building an URL out of the request object, for all the different settings and edge cases. Relying heavily on Rack in the background. Some of the methods, especially around constructing the port - need to stay part of RequestUrl as they are need to build the URL out of the request object and can not move to the URI class, unfortunately.

Once the url is build, we parse it and can work with it as an URI object.

Also - with the next release of Rack, Rack::Request::Env will be gone and that needs to be reflected in ActionDispatch::Request Class then.

@p8

p8 commented Jun 17, 2021

Copy link
Copy Markdown
Member

@aka47 Thanks for contributing to Rails!
There also is a previous PR for this: #39629
@vipulnsward Are you still working on that one?

@aka47

aka47 commented Jun 17, 2021

Copy link
Copy Markdown
Contributor Author

Thanks @p8. It's a start and I am happy to learn and add possible and more PR to rails in the future. Some parts of the PR from @vipulnsward could be merged into this one, setting the default-url-options for one. I will do that.

It has 2 failing tests - which have the same root cause - I will fix those some time soon.

@zzak

zzak commented Jun 17, 2021

Copy link
Copy Markdown
Member

Vipul said he would look into updating his PR soon, let's give him a chance to pick up where he left off 🙏

@aka47

aka47 commented Jun 17, 2021

Copy link
Copy Markdown
Contributor Author

Vipul said he would look into updating his PR soon, let's give him a chance to pick up where he left off 🙏

I am more than happy to work with @vipulnsward on this PR. Vipul - lets not do the same work twice, lets better work together on it? How about that?

Comment thread actionpack/lib/action_dispatch/http/uri.rb Outdated
@aka47
aka47 marked this pull request as ready for review June 17, 2021 15:13
Comment thread actionpack/lib/action_controller/railtie.rb Outdated
@vipulnsward

Copy link
Copy Markdown
Member

@ak15 you can take over if you are interested. I have been caught up in work, so might not get to it soon.

Comment thread actionpack/lib/action_dispatch/http/uri.rb Outdated
Comment thread actionpack/lib/action_dispatch/http/uri.rb Outdated
Comment thread actionpack/lib/action_dispatch/http/url.rb Outdated
@aka47

aka47 commented Sep 27, 2021

Copy link
Copy Markdown
Contributor Author

What are the next steps here? @p8 / @marivaldo - please help me out here. I am happy to do some more work, tune it, but I need someone who gives me a path to its being merged. That would be wonderful.

Working on this task I got the impression this could need some more follow up refactoring - as this part is very complex and interdependent. If this should be part of this PR - I would love to have someone with whom I could share my ideas for it and come an agreement together.
It starts with moving ActionDispatch::Request outside of the HTTP folder to the action_dispatch folder. FilterParamenter could be moved to the request-folder. The content_security_policy could be moved middleware or the action_dispatch folder, but not sure. In general it would be great to declutter the http folder and bring a bit more clarity into the whole thing. ( It seems like http is such an open name - it invites everyone to put things into it.)

So - should we just bring this to a quick end and merge it? Or what is the plan here forward? .. Thanks a lot ..

@aka47
aka47 force-pushed the add-toplevel-rails-application-url branch 4 times, most recently from f191756 to 3111880 Compare October 1, 2021 08:36
@aka47
aka47 requested a review from p8 October 1, 2021 08:37
@aka47
aka47 force-pushed the add-toplevel-rails-application-url branch 2 times, most recently from 8333a91 to 8865a99 Compare October 4, 2021 16:10
Comment thread actionpack/lib/action_dispatch/http/uri.rb Outdated
Comment thread actionpack/lib/action_dispatch/http/uri.rb Outdated
Comment thread actionpack/test/dispatch/uri_test.rb Outdated
Comment thread railties/lib/rails/application/bootstrap.rb Outdated
Comment thread actionpack/lib/action_dispatch/http/uri.rb Outdated
@aka47

aka47 commented Oct 6, 2021

Copy link
Copy Markdown
Contributor Author

@p8 thanks for your feedback. I took your feedback and changed quit some parts of this PR.

  • merged the URL class with the URI class.
  • removed the builder methods build_from_faulty_string and build_from_string. The logic in inside the initialize method now.
  • optimised rails initialisation with application_url from your feedback
  • handle url exception outside the URL class, inside the url from request builder module. I still believe we should handle those exceptions instead of blowing up the whole request. Url are complex, with ipv6 being forever on the horizon, different server setups and environments.
  • a started to move some code out of the http folder and believe ActionDispatch::Http::MimeNegotiation, ActionDispatch::Http::Parameters and ActionDispatch::Http::FilterParameters should be also moved to ActionDispatch::Request::** .. they are all modules that are included inside ActionDispatch::Request - nothing more.

What you think? Any other remarks from your side? ( or Are we good to go? :)

@aka47
aka47 force-pushed the add-toplevel-rails-application-url branch 2 times, most recently from c41ce80 to 604b3b5 Compare October 6, 2021 19:21
@aka47
aka47 requested a review from p8 October 7, 2021 16:31
@dhh

dhh commented Sep 14, 2024

Copy link
Copy Markdown
Member

Hmm, @matthewd raised a good point. We don't want URLs that before just generated paths to suddenly start generating fully formed URLs. So need to sort that as well.

Comment thread railties/lib/rails/generators/rails/app/templates/config/deploy.yml.tt Outdated

@aka47 aka47 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use env APP_URL in comment

aka47 and others added 2 commits September 23, 2024 16:36
Co-authored-by: Hans Lemuet <Spone@users.noreply.github.com>
Comment on lines +12 to +14
delegate :tld_length, :tld_length=, :secure_protocol, :secure_protocol=,
:extract_domain, :extract_subdomain, :extract_subdomains, :url_for,
:full_url_for, :path_for, to: ActionDispatch::Http::URI

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just a docs/api question, but I wonder what the purpose of having both a URL and URI class in the public domain, and if we just choose one (URL) because it's already public API than we should nodoc the URI class and keep the documentation for those public methods on the URL class (you can use # :method: extract_domain style rdoc notation).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the public API, it will be less confusing to have one URL class. The idea is to move the rdoc comments for the delegated methods to the URL class from the URI class, and then make the URI class nodoc, right?

Suggested change
delegate :tld_length, :tld_length=, :secure_protocol, :secure_protocol=,
:extract_domain, :extract_subdomain, :extract_subdomains, :url_for,
:full_url_for, :path_for, to: ActionDispatch::Http::URI
# Returns the domain part of a host given the domain level.
# :method: extract_domain
# # Top-level domain example
# extract_domain('www.example.com', 1) # => "example.com"
# # Second-level domain example
# extract_domain('dev.www.example.co.uk', 2) # => "example.co.uk"
delegate :tld_length, :tld_length=, :secure_protocol, :secure_protocol=,
:extract_domain, :extract_subdomain, :extract_subdomains, :url_for,
:full_url_for, :path_for, to: ActionDispatch::Http::URI

Comment thread railties/lib/rails/generators/rails/app/templates/config/deploy.yml.tt Outdated
Rails.application.url = url
default_url_options = { host: url.host, protocol: url.scheme, port: url.port }
app.default_url_options = default_url_options if app.default_url_options.blank?
app.config.action_mailer.default_url_options = default_url_options if app.config.action_mailer.default_url_options.blank?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it matter if action_mailer is not included?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
app.config.action_mailer.default_url_options = default_url_options if app.config.action_mailer.default_url_options.blank?
if app.config.action_mailer.present? && app.config.action_mailer.default_url_options.blank?
app.config.action_mailer.default_url_options = default_url_options
end

@dhh

dhh commented Oct 16, 2024

Copy link
Copy Markdown
Member

We're going to move this to Rails 8.1. @matthewd is working on helping prepare some of the remaining work, but we're too close to the 8.0 release to get it all sorted in time.

@dhh

dhh commented Jan 7, 2025

Copy link
Copy Markdown
Member

@matthewd Have you had a chance to look at this? Would love to see this land on main well in advance of 8.1 so we can really test it out.

@rafaelfranca rafaelfranca modified the milestones: 8.1.0, 8.2.0 Oct 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add top-level Rails.application.url for canonical reference