feat: improve multiple domain support by kenjis · Pull Request #6759 · codeigniter4/CodeIgniter4

kenjis · 2022-10-26T04:38:04Z

Description
Fixes #5807
Related #6746

add Config\App::$allowedHostnames
update Config\App::$baseURL in IncomingRequest if HTTP_HOST is an allowed hostname

Changes:

current_url(), base_url(), site_url() will return the URL with allowed HTTP_HOST
previous_url() will return the site URL with allowed HTTP_HOST when cannot get it from session nor HTTP_REFERER

The following functions/methods are affected (There may be others):

Using site_url()
- form_open()
- audio()
- embed()
- img()
- link_tag()
- object()
- script_tag()
- source()
- video()
- RedirectResponse::route()
- RedirectResponse::to()
- Toolbar::prepare()
- Toolbar::respond()
- anchor()
- anchor_popup()
- base_url()
- previous_url()
- url_to()
Using base_url()
- RedirectException in CodeIgnter.php L377
- UserAgent::isReferral()
Using current_url()
- ChromeLoggerHandler::__construct()
- CodeIgniter::storePreviousURL()
- Pager::ensureGroup()
- Toolbar::run()

Ref #4651

Checklist:

Securely signed commits
Component(s) with PHPDoc blocks, only if necessary or adds value
Unit testing, with >80% coverage
User guide updated
Conforms to style guide

MGatner · 2022-10-26T11:05:20Z

system/HTTP/IncomingRequest.php

    protected function detectURI(string $protocol, string $baseURL)
    {
        // Passing the config is unnecessary but left for legacy purposes
+        // @TODO For what? Why clone?


The clone prevents URI testing from changing the App Config globally.

The reason for clone is only for testing, I would like to remove it.

--- a/system/HTTP/IncomingRequest.php +++ b/system/HTTP/IncomingRequest.php @@ -195,12 +195,7 @@ class IncomingRequest extends Request */ protected function detectURI(string $protocol, string $baseURL) { - // Passing the config is unnecessary but left for legacy purposes - // @TODO For what? Why clone? - $config = clone $this->config; - $config->baseURL = $baseURL; - - $this->setPath($this->detectPath($protocol), $config); + $this->setPath($this->detectPath($this->config->uriProtocol), $this->config); } /**

And deprecate the baseURL input? Is that really only for testing?

Both $protocol and $baseURL of detectURI() are not needed.

__construct():

CodeIgniter4/system/HTTP/IncomingRequest.php

Line 172 in 11680d6

$this->detectURI($config->uriProtocol, $config->baseURL);

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 198 to 205 in 11680d6

protected function detectURI(string $protocol, string $baseURL)

{

// Passing the config is unnecessary but left for legacy purposes

$config = clone $this->config;

$config->baseURL = $baseURL;

$this->setPath($this->detectPath($protocol), $config);

}

michalsn

This may be very useful feature.

At some point we probably should consider providing some easy function (helper) to generate a URL to a different host than this defined in baseURL.

system/HTTP/IncomingRequest.php

The URI object is updated in IncomingRequest constructor.

kenjis · 2022-10-27T05:40:36Z

Is this a breaking change?

MGatner

I think this is mostly okay. I want to hear more about the write-back to App Config - that seems like a pretty big possibility for issues to creep in, and ones that would be difficult to trace. As far as I can tell the logic here is all self-contained, and since current_url() uses this class we shouldn't need to touch the Config.

MGatner · 2022-10-27T09:58:01Z

system/HTTP/IncomingRequest.php


+        if ($config->baseURL === '') {
+            // @codeCoverageIgnoreStart
+            exit('You have an empty or invalid base URL. The baseURL value must be set in Config\App.php, or through the .env file.');


This used to be conditional on !is_cli() - what is the justification or precedent for requiring CLI to have this set?

MGatner · 2022-10-27T09:59:34Z

system/HTTP/IncomingRequest.php

+        // Update Config\App::$baseURL
+        $uri = new URI($baseURL);
+        $uri->setHost($host);
+        $this->config->baseURL = $uri;


I really don't like components updating global Config values. This seems like a bad idea. What does this fix?

Without this, all helpers will return "old" URLs, not one of those defined in allowedHostnames. That's basically the whole point of all these changes.

I have an app where subdomains and custom domains are used, but I use a more dynamic approach and handle it via Registrars. I would recommend this way, but the only downside is that we can't make it work with one config variable - it's a bit more complicated.

We need the current real baseURL with allowedHostnames in somewhere.
I think Config\App::$baseURL in shared Config object is best,
because the baseURL is only one in a request and never changes. I don't see any problems.

But if something wrong that I miss, we need to save the current baseURL somewhere else.
Where?

I would rather see anything that needs to rely on "URL for the incoming request" to use a component that represents that (IncomingRequest, current_url(), etc). It seems very counterintuitive that our Config values are dependent on infrastructure context.

I get that this is the most straightforward approach to a fix, but I think it pokes a new hole in our architecture that is liable to leak, grow bigger, etc.

I'm not a fan of the proliferation of global URL helpers we already have but I would prefer something like current_base_url() to this.

Re: Registrars... I think that is a great solution at the project level, and would encourage it for individuals who need this currently.

Indeed, it may seem counter-intuitive that the value of one Config object changes dynamically.
I think it is better that the URI object in IncomingRequest has all the URL related data including the current baseURL.

system/HTTP/IncomingRequest.php

kenjis · 2022-10-29T23:21:29Z

Go to #6785

kenjis added enhancement PRs that improve existing functionalities 4.3 labels Oct 26, 2022

MGatner reviewed Oct 26, 2022

View reviewed changes

michalsn reviewed Oct 26, 2022

View reviewed changes

system/HTTP/IncomingRequest.php Show resolved Hide resolved

system/HTTP/IncomingRequest.php Outdated Show resolved Hide resolved

kenjis force-pushed the improve-subdomain-support branch from b984b8f to 52a8589 Compare October 27, 2022 04:53

kenjis added 17 commits October 27, 2022 14:07

docs: fix @param

1cf2cf5

refactor: early exit()

b94e0c7

feat: add Config\App::$allowedHostnames and use it for $baseURL

00a930a

docs: remove inaccurate @param

903c233

docs: @todo

99a8768

test: fix global state before test

2237a24

test: add tests for Config\App::$AllowedHostname

a240942

refactor: remove unneeded if

5591103

refactor: early return

0d02e7c

refactor: use array destructuring

608ddd4

style: break long line

ef93c75

test: move line up and add empty lines

04f1691

refactor: replace Services::request()

e727dae

style: break long lines

4946343

fix: remove uneeded URI object

10a628c

The URI object is updated in IncomingRequest constructor.

test: add tests for base_url() and site_url() with $allowedHostnames

3906bea

docs: break long lines

b6e0976

kenjis force-pushed the improve-subdomain-support branch from 52a8589 to b6e0976 Compare October 27, 2022 05:08

MGatner requested changes Oct 27, 2022

View reviewed changes

kenjis added 4.4 and removed 4.3 labels Oct 28, 2022

kenjis mentioned this pull request Oct 29, 2022

feat: add $allowedHostnames for multiple domain support #6785

Merged

5 tasks

kenjis closed this Oct 29, 2022

kenjis deleted the improve-subdomain-support branch October 29, 2022 23:23

	protected function detectURI(string $protocol, string $baseURL)
	{
	// Passing the config is unnecessary but left for legacy purposes
	$config = clone $this->config;
	$config->baseURL = $baseURL;

	$this->setPath($this->detectPath($protocol), $config);
	}

Conversation

kenjis commented Oct 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kenjis Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MGatner Oct 27, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michalsn left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kenjis commented Oct 27, 2022

Uh oh!

MGatner left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kenjis commented Oct 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

kenjis commented Oct 26, 2022 •

edited

Loading

kenjis Oct 27, 2022 •

edited

Loading

MGatner Oct 27, 2022 •

edited

Loading