URL Class by MGatner · Pull Request #4647 · codeigniter4/CodeIgniter4

MGatner · 2021-05-05T01:45:53Z

Description
"Stage Three", this PR introduces a companion class to URI: URL. The URL class handles internal URIs (i.e. specific to the project) in the following manner:

A URL is always constructed with a relative path
A URL will always use the App config values from its construction (baseURL, indexPage, forceGlobalSecureRequest)
A URL is immutable, not affected by changes to $_SERVER or services once it has been created

I would like URL to become the definitive source for internal URLs within the framework (e.g. replace the logic in base_url() and current_url(), be used by RedirectResponse, etc) but for now this PR is just introducing the concept and demonstrating the tests. Please be thorough with any reviews, since URLs have been a long pain point (particularly around subfolders).

Note: This PR relies on #4644 and #4646.

Checklist:

lonnieezell · 2021-05-05T03:06:41Z

Haven't looked at the code yet but my initial thought is - this is making the framework more complex and confusing than it needs to be. Can we just start prepping for a bigger release and fix this in the existing URI class? Have two classes so closely related to each other are going to be super confusing for any devs that dig into the inner workings. Or just to those who's IDE starts to suggest both classes for them.

Additionally - isn't the purpose of site_url to be base_url but taking index.php settings into account?

MGatner · 2021-05-05T11:58:06Z

We really need both. It doesn't need to be implemented this way, but we need to be able to do two things:

Build out a URI representing any valid URI for use anywhere one would want a URI
Build "project-specific" URIs, based off the configuration in App and (potentially) any special routes

Currently this is a real problem:

CodeIgniter4/system/HTTP/URI.php

Lines 566 to 584 in 4cc1452

// If hosted in a sub-folder, we will have additional
// segments that show up prior to the URI path we just
// grabbed from the request, so add it on if necessary.
$config = config(App::class); 
$baseUri = newself($config->baseURL); 
$basePath = trim($baseUri->getPath(), '/') . '/'; 
$path = $this->getPath(); 
$trimPath = ltrim($path, '/'); 
if ($basePath !== '/' && strpos($trimPath, $basePath) !== 0) 
 { 
$path = $basePath . $trimPath; 
 } 
// force https if needed
if ($config->forceGlobalSecureRequests) 
 { 
$this->setScheme('https'); 
 } 

I've tried to mitigate it in #4646 but this "bleeds" point 2 into point 1. URI should not know or care about the framework settings because it makes it impossible to create any URI one might want because the framework settings take over.

Currently we handle "project-specific" URIs with the URL Helper functions (base_url(), site_url(), current_url()) - the problem with these is that the final project has no concept of "what is project" versus "what is path" (e.g. http://example.com/foo/bar - is the baseURLhttp://example.com/ with "foo/bar" path? or is it http://example.com/foo/ with "bar" path?) so I decided to go with a class solution so we have some way of storing and accessing that info.

I originally had URL as FrameworkURI that was an extension of URI, but there are too many setters in URI that could "violate" an internal URI so I split it out; at that point I realized it was basically a full URL so went with the name, but I'm open to changing it.

MGatner · 2021-05-05T12:56:59Z

isn't the purpose of site_url to be base_url but taking index.php settings into account

Exactly. site_url() amounts to "take base_url(), add App::$indexPage, add any given path, be smart about slashes." URL::to() tries to encompass this behavior but with a little more utility of resolving named and reverse routes.

paulbalandan · 2021-05-05T18:08:45Z

+ */
+	public static function base()
+	{
+ return static::public('');


Since the class is final, then inheritance is no more possible. So, there's no need to use the static keyword. Instead use self.
Suggested change
returnstatic::public('');
returnself::public('');

On the topic, how do you think the class should be scoped? Originally I had a normal class with final static and constructor methods (hence the leftover static references here) but I decided that nobody would go through the hassle of extending for the few allowed methods so made the whole thing final.

paulbalandan · 2021-05-05T18:09:26Z

+ $config = clone config('App');
+ $config->indexPage = '';
+
+ return new static($uri, $config);


Suggested change
returnnewstatic($uri, $config);
returnnewself($uri, $config);

paulbalandan · 2021-05-05T18:09:49Z

+ $config = clone config('App');
+ $config->indexPage = '';


Suggested change
$config = cloneconfig('App');
$config->indexPage = '';
$config = cloneconfig('App');
$config->indexPage = '';

paulbalandan · 2021-05-05T18:10:09Z

+ return self::$current;
+ }
+
+ return static::fromRequest(Services::request());


Suggested change
returnstatic::fromRequest(Services::request());
returnself::fromRequest(Services::request());

paulbalandan · 2021-05-05T18:10:24Z

+ */
+	public static function to(string $uri)
+	{
+ return new static(rtrim($uri, '/ '));


Suggested change
returnnewstatic(rtrim($uri, '/ '));
returnnewself(rtrim($uri, '/ '));

paulbalandan · 2021-05-05T18:13:57Z

+ *
+ * @param string $uri Additional URI string to include
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:12Z

+	/**
+ * Returns an instance representing the current URL.
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:33Z

+ *
+ * @param string $uri
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:44Z

+ *
+ * @param string $uri Named or reverse route
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:56Z

+ *
+ * @param IncomingRequest $request
+ *
+ * @return static


Suggested change
* @return static
* @return self

lonnieezell · 2021-05-05T18:56:51Z

I'll be honest. I'm not a fan of having both. I understand your reasoning but think it will make things more complex/confusing for the developers. I wish we could find a way to refactor the current class to work in conjunction with the url helpers.

That being said, if the rest of the team is fine with it then I'll shut up :)

MGatner · 2021-05-05T19:04:38Z

@lonnieezell I understand. Let's think of ways we could make this better. I do think the helpers could be "beefed" up to cover this class but we would lose the ability to distinguish path parts (see my comment about "what is project" versus "what is path"). That might be an acceptable tradeoff? But there are some real issues with the current setup that need addressing. Just an example, IncomingRequest is clear that it only builds out the URI it needs for itself:

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 153 to 160 in 2e0e020

// Determine the current URI
// NOTE: This WILL NOT match the actual URL in the browser since for
// everything this cares about (and the router, etc) is the portion
// AFTER the script name. So, if hosted in a sub-folder this will
// appear different than actual URL. If you need that, use current_url().
$this->uri = $uri; 
$this->detectURI($config->uriProtocol, $config->baseURL); 

If you need that, use current_url().

... but then this is the entirety of current_url():

$uri = clone Services::request()->uri;

I believe that a support class (in this case URL) gives us a lot more flexibility than the helper functions for working with framework URIs - maybe we could make it an internal class? or move it out of HTTP?

MGatner · 2021-05-05T01:45:53Z

Description
"Stage Three", this PR introduces a companion class to URI: URL. The URL class handles internal URIs (i.e. specific to the project) in the following manner:

A URL is always constructed with a relative path
A URL will always use the App config values from its construction (baseURL, indexPage, forceGlobalSecureRequest)
A URL is immutable, not affected by changes to $_SERVER or services once it has been created

I would like URL to become the definitive source for internal URLs within the framework (e.g. replace the logic in base_url() and current_url(), be used by RedirectResponse, etc) but for now this PR is just introducing the concept and demonstrating the tests. Please be thorough with any reviews, since URLs have been a long pain point (particularly around subfolders).

Note: This PR relies on #4644 and #4646.

Checklist:

lonnieezell · 2021-05-05T03:06:41Z

Haven't looked at the code yet but my initial thought is - this is making the framework more complex and confusing than it needs to be. Can we just start prepping for a bigger release and fix this in the existing URI class? Have two classes so closely related to each other are going to be super confusing for any devs that dig into the inner workings. Or just to those who's IDE starts to suggest both classes for them.

Additionally - isn't the purpose of site_url to be base_url but taking index.php settings into account?

MGatner · 2021-05-05T11:58:06Z

We really need both. It doesn't need to be implemented this way, but we need to be able to do two things:

Build out a URI representing any valid URI for use anywhere one would want a URI
Build "project-specific" URIs, based off the configuration in App and (potentially) any special routes

Currently this is a real problem:

CodeIgniter4/system/HTTP/URI.php

Lines 566 to 584 in 4cc1452

// If hosted in a sub-folder, we will have additional
// segments that show up prior to the URI path we just
// grabbed from the request, so add it on if necessary.
$config = config(App::class); 
$baseUri = newself($config->baseURL); 
$basePath = trim($baseUri->getPath(), '/') . '/'; 
$path = $this->getPath(); 
$trimPath = ltrim($path, '/'); 
if ($basePath !== '/' && strpos($trimPath, $basePath) !== 0) 
 { 
$path = $basePath . $trimPath; 
 } 
// force https if needed
if ($config->forceGlobalSecureRequests) 
 { 
$this->setScheme('https'); 
 } 

I've tried to mitigate it in #4646 but this "bleeds" point 2 into point 1. URI should not know or care about the framework settings because it makes it impossible to create any URI one might want because the framework settings take over.

Currently we handle "project-specific" URIs with the URL Helper functions (base_url(), site_url(), current_url()) - the problem with these is that the final project has no concept of "what is project" versus "what is path" (e.g. http://example.com/foo/bar - is the baseURLhttp://example.com/ with "foo/bar" path? or is it http://example.com/foo/ with "bar" path?) so I decided to go with a class solution so we have some way of storing and accessing that info.

I originally had URL as FrameworkURI that was an extension of URI, but there are too many setters in URI that could "violate" an internal URI so I split it out; at that point I realized it was basically a full URL so went with the name, but I'm open to changing it.

MGatner · 2021-05-05T12:56:59Z

isn't the purpose of site_url to be base_url but taking index.php settings into account

Exactly. site_url() amounts to "take base_url(), add App::$indexPage, add any given path, be smart about slashes." URL::to() tries to encompass this behavior but with a little more utility of resolving named and reverse routes.

paulbalandan · 2021-05-05T18:08:45Z

+ */
+	public static function base()
+	{
+ return static::public('');


Since the class is final, then inheritance is no more possible. So, there's no need to use the static keyword. Instead use self.
Suggested change
returnstatic::public('');
returnself::public('');

On the topic, how do you think the class should be scoped? Originally I had a normal class with final static and constructor methods (hence the leftover static references here) but I decided that nobody would go through the hassle of extending for the few allowed methods so made the whole thing final.

paulbalandan · 2021-05-05T18:09:26Z

+ $config = clone config('App');
+ $config->indexPage = '';
+
+ return new static($uri, $config);


Suggested change
returnnewstatic($uri, $config);
returnnewself($uri, $config);

paulbalandan · 2021-05-05T18:09:49Z

+ $config = clone config('App');
+ $config->indexPage = '';


Suggested change
$config = cloneconfig('App');
$config->indexPage = '';
$config = cloneconfig('App');
$config->indexPage = '';

paulbalandan · 2021-05-05T18:10:09Z

+ return self::$current;
+ }
+
+ return static::fromRequest(Services::request());


Suggested change
returnstatic::fromRequest(Services::request());
returnself::fromRequest(Services::request());

paulbalandan · 2021-05-05T18:10:24Z

+ */
+	public static function to(string $uri)
+	{
+ return new static(rtrim($uri, '/ '));


Suggested change
returnnewstatic(rtrim($uri, '/ '));
returnnewself(rtrim($uri, '/ '));

paulbalandan · 2021-05-05T18:13:57Z

+ *
+ * @param string $uri Additional URI string to include
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:12Z

+	/**
+ * Returns an instance representing the current URL.
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:33Z

+ *
+ * @param string $uri
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:44Z

+ *
+ * @param string $uri Named or reverse route
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:56Z

+ *
+ * @param IncomingRequest $request
+ *
+ * @return static


Suggested change
* @return static
* @return self

lonnieezell · 2021-05-05T18:56:51Z

I'll be honest. I'm not a fan of having both. I understand your reasoning but think it will make things more complex/confusing for the developers. I wish we could find a way to refactor the current class to work in conjunction with the url helpers.

That being said, if the rest of the team is fine with it then I'll shut up :)

MGatner · 2021-05-05T19:04:38Z

@lonnieezell I understand. Let's think of ways we could make this better. I do think the helpers could be "beefed" up to cover this class but we would lose the ability to distinguish path parts (see my comment about "what is project" versus "what is path"). That might be an acceptable tradeoff? But there are some real issues with the current setup that need addressing. Just an example, IncomingRequest is clear that it only builds out the URI it needs for itself:

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 153 to 160 in 2e0e020

// Determine the current URI
// NOTE: This WILL NOT match the actual URL in the browser since for
// everything this cares about (and the router, etc) is the portion
// AFTER the script name. So, if hosted in a sub-folder this will
// appear different than actual URL. If you need that, use current_url().
$this->uri = $uri; 
$this->detectURI($config->uriProtocol, $config->baseURL); 

If you need that, use current_url().

... but then this is the entirety of current_url():

$uri = clone Services::request()->uri;

I believe that a support class (in this case URL) gives us a lot more flexibility than the helper functions for working with framework URIs - maybe we could make it an internal class? or move it out of HTTP?

MGatner · 2021-05-05T01:45:53Z

Description
"Stage Three", this PR introduces a companion class to URI: URL. The URL class handles internal URIs (i.e. specific to the project) in the following manner:

A URL is always constructed with a relative path
A URL will always use the App config values from its construction (baseURL, indexPage, forceGlobalSecureRequest)
A URL is immutable, not affected by changes to $_SERVER or services once it has been created

I would like URL to become the definitive source for internal URLs within the framework (e.g. replace the logic in base_url() and current_url(), be used by RedirectResponse, etc) but for now this PR is just introducing the concept and demonstrating the tests. Please be thorough with any reviews, since URLs have been a long pain point (particularly around subfolders).

Note: This PR relies on #4644 and #4646.

Checklist:

lonnieezell · 2021-05-05T03:06:41Z

Haven't looked at the code yet but my initial thought is - this is making the framework more complex and confusing than it needs to be. Can we just start prepping for a bigger release and fix this in the existing URI class? Have two classes so closely related to each other are going to be super confusing for any devs that dig into the inner workings. Or just to those who's IDE starts to suggest both classes for them.

Additionally - isn't the purpose of site_url to be base_url but taking index.php settings into account?

MGatner · 2021-05-05T11:58:06Z

We really need both. It doesn't need to be implemented this way, but we need to be able to do two things:

Build out a URI representing any valid URI for use anywhere one would want a URI
Build "project-specific" URIs, based off the configuration in App and (potentially) any special routes

Currently this is a real problem:

CodeIgniter4/system/HTTP/URI.php

Lines 566 to 584 in 4cc1452

// If hosted in a sub-folder, we will have additional
// segments that show up prior to the URI path we just
// grabbed from the request, so add it on if necessary.
$config = config(App::class); 
$baseUri = newself($config->baseURL); 
$basePath = trim($baseUri->getPath(), '/') . '/'; 
$path = $this->getPath(); 
$trimPath = ltrim($path, '/'); 
if ($basePath !== '/' && strpos($trimPath, $basePath) !== 0) 
 { 
$path = $basePath . $trimPath; 
 } 
// force https if needed
if ($config->forceGlobalSecureRequests) 
 { 
$this->setScheme('https'); 
 } 

I've tried to mitigate it in #4646 but this "bleeds" point 2 into point 1. URI should not know or care about the framework settings because it makes it impossible to create any URI one might want because the framework settings take over.

Currently we handle "project-specific" URIs with the URL Helper functions (base_url(), site_url(), current_url()) - the problem with these is that the final project has no concept of "what is project" versus "what is path" (e.g. http://example.com/foo/bar - is the baseURLhttp://example.com/ with "foo/bar" path? or is it http://example.com/foo/ with "bar" path?) so I decided to go with a class solution so we have some way of storing and accessing that info.

I originally had URL as FrameworkURI that was an extension of URI, but there are too many setters in URI that could "violate" an internal URI so I split it out; at that point I realized it was basically a full URL so went with the name, but I'm open to changing it.

MGatner · 2021-05-05T12:56:59Z

isn't the purpose of site_url to be base_url but taking index.php settings into account

Exactly. site_url() amounts to "take base_url(), add App::$indexPage, add any given path, be smart about slashes." URL::to() tries to encompass this behavior but with a little more utility of resolving named and reverse routes.

paulbalandan · 2021-05-05T18:08:45Z

+ */
+	public static function base()
+	{
+ return static::public('');


Since the class is final, then inheritance is no more possible. So, there's no need to use the static keyword. Instead use self.
Suggested change
returnstatic::public('');
returnself::public('');

On the topic, how do you think the class should be scoped? Originally I had a normal class with final static and constructor methods (hence the leftover static references here) but I decided that nobody would go through the hassle of extending for the few allowed methods so made the whole thing final.

paulbalandan · 2021-05-05T18:09:26Z

+ $config = clone config('App');
+ $config->indexPage = '';
+
+ return new static($uri, $config);


Suggested change
returnnewstatic($uri, $config);
returnnewself($uri, $config);

paulbalandan · 2021-05-05T18:09:49Z

+ $config = clone config('App');
+ $config->indexPage = '';


Suggested change
$config = cloneconfig('App');
$config->indexPage = '';
$config = cloneconfig('App');
$config->indexPage = '';

paulbalandan · 2021-05-05T18:10:09Z

+ return self::$current;
+ }
+
+ return static::fromRequest(Services::request());


Suggested change
returnstatic::fromRequest(Services::request());
returnself::fromRequest(Services::request());

paulbalandan · 2021-05-05T18:10:24Z

+ */
+	public static function to(string $uri)
+	{
+ return new static(rtrim($uri, '/ '));


Suggested change
returnnewstatic(rtrim($uri, '/ '));
returnnewself(rtrim($uri, '/ '));

paulbalandan · 2021-05-05T18:13:57Z

+ *
+ * @param string $uri Additional URI string to include
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:12Z

+	/**
+ * Returns an instance representing the current URL.
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:33Z

+ *
+ * @param string $uri
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:44Z

+ *
+ * @param string $uri Named or reverse route
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:56Z

+ *
+ * @param IncomingRequest $request
+ *
+ * @return static


Suggested change
* @return static
* @return self

lonnieezell · 2021-05-05T18:56:51Z

I'll be honest. I'm not a fan of having both. I understand your reasoning but think it will make things more complex/confusing for the developers. I wish we could find a way to refactor the current class to work in conjunction with the url helpers.

That being said, if the rest of the team is fine with it then I'll shut up :)

MGatner · 2021-05-05T19:04:38Z

@lonnieezell I understand. Let's think of ways we could make this better. I do think the helpers could be "beefed" up to cover this class but we would lose the ability to distinguish path parts (see my comment about "what is project" versus "what is path"). That might be an acceptable tradeoff? But there are some real issues with the current setup that need addressing. Just an example, IncomingRequest is clear that it only builds out the URI it needs for itself:

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 153 to 160 in 2e0e020

// Determine the current URI
// NOTE: This WILL NOT match the actual URL in the browser since for
// everything this cares about (and the router, etc) is the portion
// AFTER the script name. So, if hosted in a sub-folder this will
// appear different than actual URL. If you need that, use current_url().
$this->uri = $uri; 
$this->detectURI($config->uriProtocol, $config->baseURL); 

If you need that, use current_url().

... but then this is the entirety of current_url():

$uri = clone Services::request()->uri;

I believe that a support class (in this case URL) gives us a lot more flexibility than the helper functions for working with framework URIs - maybe we could make it an internal class? or move it out of HTTP?

MGatner · 2021-05-05T01:45:53Z

Description
"Stage Three", this PR introduces a companion class to URI: URL. The URL class handles internal URIs (i.e. specific to the project) in the following manner:

A URL is always constructed with a relative path
A URL will always use the App config values from its construction (baseURL, indexPage, forceGlobalSecureRequest)
A URL is immutable, not affected by changes to $_SERVER or services once it has been created

I would like URL to become the definitive source for internal URLs within the framework (e.g. replace the logic in base_url() and current_url(), be used by RedirectResponse, etc) but for now this PR is just introducing the concept and demonstrating the tests. Please be thorough with any reviews, since URLs have been a long pain point (particularly around subfolders).

Note: This PR relies on #4644 and #4646.

Checklist:

lonnieezell · 2021-05-05T03:06:41Z

Haven't looked at the code yet but my initial thought is - this is making the framework more complex and confusing than it needs to be. Can we just start prepping for a bigger release and fix this in the existing URI class? Have two classes so closely related to each other are going to be super confusing for any devs that dig into the inner workings. Or just to those who's IDE starts to suggest both classes for them.

Additionally - isn't the purpose of site_url to be base_url but taking index.php settings into account?

MGatner · 2021-05-05T11:58:06Z

We really need both. It doesn't need to be implemented this way, but we need to be able to do two things:

Build out a URI representing any valid URI for use anywhere one would want a URI
Build "project-specific" URIs, based off the configuration in App and (potentially) any special routes

Currently this is a real problem:

CodeIgniter4/system/HTTP/URI.php

Lines 566 to 584 in 4cc1452

// If hosted in a sub-folder, we will have additional
// segments that show up prior to the URI path we just
// grabbed from the request, so add it on if necessary.
$config = config(App::class); 
$baseUri = newself($config->baseURL); 
$basePath = trim($baseUri->getPath(), '/') . '/'; 
$path = $this->getPath(); 
$trimPath = ltrim($path, '/'); 
if ($basePath !== '/' && strpos($trimPath, $basePath) !== 0) 
 { 
$path = $basePath . $trimPath; 
 } 
// force https if needed
if ($config->forceGlobalSecureRequests) 
 { 
$this->setScheme('https'); 
 } 

I've tried to mitigate it in #4646 but this "bleeds" point 2 into point 1. URI should not know or care about the framework settings because it makes it impossible to create any URI one might want because the framework settings take over.

Currently we handle "project-specific" URIs with the URL Helper functions (base_url(), site_url(), current_url()) - the problem with these is that the final project has no concept of "what is project" versus "what is path" (e.g. http://example.com/foo/bar - is the baseURLhttp://example.com/ with "foo/bar" path? or is it http://example.com/foo/ with "bar" path?) so I decided to go with a class solution so we have some way of storing and accessing that info.

I originally had URL as FrameworkURI that was an extension of URI, but there are too many setters in URI that could "violate" an internal URI so I split it out; at that point I realized it was basically a full URL so went with the name, but I'm open to changing it.

MGatner · 2021-05-05T12:56:59Z

isn't the purpose of site_url to be base_url but taking index.php settings into account

Exactly. site_url() amounts to "take base_url(), add App::$indexPage, add any given path, be smart about slashes." URL::to() tries to encompass this behavior but with a little more utility of resolving named and reverse routes.

paulbalandan · 2021-05-05T18:08:45Z

+ */
+	public static function base()
+	{
+ return static::public('');


Since the class is final, then inheritance is no more possible. So, there's no need to use the static keyword. Instead use self.
Suggested change
returnstatic::public('');
returnself::public('');

On the topic, how do you think the class should be scoped? Originally I had a normal class with final static and constructor methods (hence the leftover static references here) but I decided that nobody would go through the hassle of extending for the few allowed methods so made the whole thing final.

paulbalandan · 2021-05-05T18:09:26Z

+ $config = clone config('App');
+ $config->indexPage = '';
+
+ return new static($uri, $config);


Suggested change
returnnewstatic($uri, $config);
returnnewself($uri, $config);

paulbalandan · 2021-05-05T18:09:49Z

+ $config = clone config('App');
+ $config->indexPage = '';


Suggested change
$config = cloneconfig('App');
$config->indexPage = '';
$config = cloneconfig('App');
$config->indexPage = '';

paulbalandan · 2021-05-05T18:10:09Z

+ return self::$current;
+ }
+
+ return static::fromRequest(Services::request());


Suggested change
returnstatic::fromRequest(Services::request());
returnself::fromRequest(Services::request());

paulbalandan · 2021-05-05T18:10:24Z

+ */
+	public static function to(string $uri)
+	{
+ return new static(rtrim($uri, '/ '));


Suggested change
returnnewstatic(rtrim($uri, '/ '));
returnnewself(rtrim($uri, '/ '));

paulbalandan · 2021-05-05T18:13:57Z

+ *
+ * @param string $uri Additional URI string to include
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:12Z

+	/**
+ * Returns an instance representing the current URL.
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:33Z

+ *
+ * @param string $uri
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:44Z

+ *
+ * @param string $uri Named or reverse route
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:56Z

+ *
+ * @param IncomingRequest $request
+ *
+ * @return static


Suggested change
* @return static
* @return self

lonnieezell · 2021-05-05T18:56:51Z

I'll be honest. I'm not a fan of having both. I understand your reasoning but think it will make things more complex/confusing for the developers. I wish we could find a way to refactor the current class to work in conjunction with the url helpers.

That being said, if the rest of the team is fine with it then I'll shut up :)

MGatner · 2021-05-05T19:04:38Z

@lonnieezell I understand. Let's think of ways we could make this better. I do think the helpers could be "beefed" up to cover this class but we would lose the ability to distinguish path parts (see my comment about "what is project" versus "what is path"). That might be an acceptable tradeoff? But there are some real issues with the current setup that need addressing. Just an example, IncomingRequest is clear that it only builds out the URI it needs for itself:

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 153 to 160 in 2e0e020

// Determine the current URI
// NOTE: This WILL NOT match the actual URL in the browser since for
// everything this cares about (and the router, etc) is the portion
// AFTER the script name. So, if hosted in a sub-folder this will
// appear different than actual URL. If you need that, use current_url().
$this->uri = $uri; 
$this->detectURI($config->uriProtocol, $config->baseURL); 

If you need that, use current_url().

... but then this is the entirety of current_url():

$uri = clone Services::request()->uri;

I believe that a support class (in this case URL) gives us a lot more flexibility than the helper functions for working with framework URIs - maybe we could make it an internal class? or move it out of HTTP?

MGatner · 2021-05-05T01:45:53Z

Description
"Stage Three", this PR introduces a companion class to URI: URL. The URL class handles internal URIs (i.e. specific to the project) in the following manner:

A URL is always constructed with a relative path
A URL will always use the App config values from its construction (baseURL, indexPage, forceGlobalSecureRequest)
A URL is immutable, not affected by changes to $_SERVER or services once it has been created

I would like URL to become the definitive source for internal URLs within the framework (e.g. replace the logic in base_url() and current_url(), be used by RedirectResponse, etc) but for now this PR is just introducing the concept and demonstrating the tests. Please be thorough with any reviews, since URLs have been a long pain point (particularly around subfolders).

Note: This PR relies on #4644 and #4646.

Checklist:

lonnieezell · 2021-05-05T03:06:41Z

Haven't looked at the code yet but my initial thought is - this is making the framework more complex and confusing than it needs to be. Can we just start prepping for a bigger release and fix this in the existing URI class? Have two classes so closely related to each other are going to be super confusing for any devs that dig into the inner workings. Or just to those who's IDE starts to suggest both classes for them.

Additionally - isn't the purpose of site_url to be base_url but taking index.php settings into account?

MGatner · 2021-05-05T11:58:06Z

We really need both. It doesn't need to be implemented this way, but we need to be able to do two things:

Build out a URI representing any valid URI for use anywhere one would want a URI
Build "project-specific" URIs, based off the configuration in App and (potentially) any special routes

Currently this is a real problem:

CodeIgniter4/system/HTTP/URI.php

Lines 566 to 584 in 4cc1452

// If hosted in a sub-folder, we will have additional
// segments that show up prior to the URI path we just
// grabbed from the request, so add it on if necessary.
$config = config(App::class); 
$baseUri = newself($config->baseURL); 
$basePath = trim($baseUri->getPath(), '/') . '/'; 
$path = $this->getPath(); 
$trimPath = ltrim($path, '/'); 
if ($basePath !== '/' && strpos($trimPath, $basePath) !== 0) 
 { 
$path = $basePath . $trimPath; 
 } 
// force https if needed
if ($config->forceGlobalSecureRequests) 
 { 
$this->setScheme('https'); 
 } 

I've tried to mitigate it in #4646 but this "bleeds" point 2 into point 1. URI should not know or care about the framework settings because it makes it impossible to create any URI one might want because the framework settings take over.

Currently we handle "project-specific" URIs with the URL Helper functions (base_url(), site_url(), current_url()) - the problem with these is that the final project has no concept of "what is project" versus "what is path" (e.g. http://example.com/foo/bar - is the baseURLhttp://example.com/ with "foo/bar" path? or is it http://example.com/foo/ with "bar" path?) so I decided to go with a class solution so we have some way of storing and accessing that info.

I originally had URL as FrameworkURI that was an extension of URI, but there are too many setters in URI that could "violate" an internal URI so I split it out; at that point I realized it was basically a full URL so went with the name, but I'm open to changing it.

MGatner · 2021-05-05T12:56:59Z

isn't the purpose of site_url to be base_url but taking index.php settings into account

Exactly. site_url() amounts to "take base_url(), add App::$indexPage, add any given path, be smart about slashes." URL::to() tries to encompass this behavior but with a little more utility of resolving named and reverse routes.

paulbalandan · 2021-05-05T18:08:45Z

+ */
+	public static function base()
+	{
+ return static::public('');


Since the class is final, then inheritance is no more possible. So, there's no need to use the static keyword. Instead use self.
Suggested change
returnstatic::public('');
returnself::public('');

On the topic, how do you think the class should be scoped? Originally I had a normal class with final static and constructor methods (hence the leftover static references here) but I decided that nobody would go through the hassle of extending for the few allowed methods so made the whole thing final.

paulbalandan · 2021-05-05T18:09:26Z

+ $config = clone config('App');
+ $config->indexPage = '';
+
+ return new static($uri, $config);


Suggested change
returnnewstatic($uri, $config);
returnnewself($uri, $config);

paulbalandan · 2021-05-05T18:09:49Z

+ $config = clone config('App');
+ $config->indexPage = '';


Suggested change
$config = cloneconfig('App');
$config->indexPage = '';
$config = cloneconfig('App');
$config->indexPage = '';

paulbalandan · 2021-05-05T18:10:09Z

+ return self::$current;
+ }
+
+ return static::fromRequest(Services::request());


Suggested change
returnstatic::fromRequest(Services::request());
returnself::fromRequest(Services::request());

paulbalandan · 2021-05-05T18:10:24Z

+ */
+	public static function to(string $uri)
+	{
+ return new static(rtrim($uri, '/ '));


Suggested change
returnnewstatic(rtrim($uri, '/ '));
returnnewself(rtrim($uri, '/ '));

paulbalandan · 2021-05-05T18:13:57Z

+ *
+ * @param string $uri Additional URI string to include
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:12Z

+	/**
+ * Returns an instance representing the current URL.
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:33Z

+ *
+ * @param string $uri
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:44Z

+ *
+ * @param string $uri Named or reverse route
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:56Z

+ *
+ * @param IncomingRequest $request
+ *
+ * @return static


Suggested change
* @return static
* @return self

lonnieezell · 2021-05-05T18:56:51Z

I'll be honest. I'm not a fan of having both. I understand your reasoning but think it will make things more complex/confusing for the developers. I wish we could find a way to refactor the current class to work in conjunction with the url helpers.

That being said, if the rest of the team is fine with it then I'll shut up :)

MGatner · 2021-05-05T19:04:38Z

@lonnieezell I understand. Let's think of ways we could make this better. I do think the helpers could be "beefed" up to cover this class but we would lose the ability to distinguish path parts (see my comment about "what is project" versus "what is path"). That might be an acceptable tradeoff? But there are some real issues with the current setup that need addressing. Just an example, IncomingRequest is clear that it only builds out the URI it needs for itself:

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 153 to 160 in 2e0e020

// Determine the current URI
// NOTE: This WILL NOT match the actual URL in the browser since for
// everything this cares about (and the router, etc) is the portion
// AFTER the script name. So, if hosted in a sub-folder this will
// appear different than actual URL. If you need that, use current_url().
$this->uri = $uri; 
$this->detectURI($config->uriProtocol, $config->baseURL); 

If you need that, use current_url().

... but then this is the entirety of current_url():

$uri = clone Services::request()->uri;

I believe that a support class (in this case URL) gives us a lot more flexibility than the helper functions for working with framework URIs - maybe we could make it an internal class? or move it out of HTTP?

MGatner · 2021-05-05T01:45:53Z

Description
"Stage Three", this PR introduces a companion class to URI: URL. The URL class handles internal URIs (i.e. specific to the project) in the following manner:

A URL is always constructed with a relative path
A URL will always use the App config values from its construction (baseURL, indexPage, forceGlobalSecureRequest)
A URL is immutable, not affected by changes to $_SERVER or services once it has been created

I would like URL to become the definitive source for internal URLs within the framework (e.g. replace the logic in base_url() and current_url(), be used by RedirectResponse, etc) but for now this PR is just introducing the concept and demonstrating the tests. Please be thorough with any reviews, since URLs have been a long pain point (particularly around subfolders).

Note: This PR relies on #4644 and #4646.

Checklist:

lonnieezell · 2021-05-05T03:06:41Z

Haven't looked at the code yet but my initial thought is - this is making the framework more complex and confusing than it needs to be. Can we just start prepping for a bigger release and fix this in the existing URI class? Have two classes so closely related to each other are going to be super confusing for any devs that dig into the inner workings. Or just to those who's IDE starts to suggest both classes for them.

Additionally - isn't the purpose of site_url to be base_url but taking index.php settings into account?

MGatner · 2021-05-05T11:58:06Z

We really need both. It doesn't need to be implemented this way, but we need to be able to do two things:

Build out a URI representing any valid URI for use anywhere one would want a URI
Build "project-specific" URIs, based off the configuration in App and (potentially) any special routes

Currently this is a real problem:

CodeIgniter4/system/HTTP/URI.php

Lines 566 to 584 in 4cc1452

// If hosted in a sub-folder, we will have additional
// segments that show up prior to the URI path we just
// grabbed from the request, so add it on if necessary.
$config = config(App::class); 
$baseUri = newself($config->baseURL); 
$basePath = trim($baseUri->getPath(), '/') . '/'; 
$path = $this->getPath(); 
$trimPath = ltrim($path, '/'); 
if ($basePath !== '/' && strpos($trimPath, $basePath) !== 0) 
 { 
$path = $basePath . $trimPath; 
 } 
// force https if needed
if ($config->forceGlobalSecureRequests) 
 { 
$this->setScheme('https'); 
 } 

I've tried to mitigate it in #4646 but this "bleeds" point 2 into point 1. URI should not know or care about the framework settings because it makes it impossible to create any URI one might want because the framework settings take over.

Currently we handle "project-specific" URIs with the URL Helper functions (base_url(), site_url(), current_url()) - the problem with these is that the final project has no concept of "what is project" versus "what is path" (e.g. http://example.com/foo/bar - is the baseURLhttp://example.com/ with "foo/bar" path? or is it http://example.com/foo/ with "bar" path?) so I decided to go with a class solution so we have some way of storing and accessing that info.

I originally had URL as FrameworkURI that was an extension of URI, but there are too many setters in URI that could "violate" an internal URI so I split it out; at that point I realized it was basically a full URL so went with the name, but I'm open to changing it.

MGatner · 2021-05-05T12:56:59Z

isn't the purpose of site_url to be base_url but taking index.php settings into account

Exactly. site_url() amounts to "take base_url(), add App::$indexPage, add any given path, be smart about slashes." URL::to() tries to encompass this behavior but with a little more utility of resolving named and reverse routes.

paulbalandan · 2021-05-05T18:08:45Z

+ */
+	public static function base()
+	{
+ return static::public('');


Since the class is final, then inheritance is no more possible. So, there's no need to use the static keyword. Instead use self.
Suggested change
returnstatic::public('');
returnself::public('');

On the topic, how do you think the class should be scoped? Originally I had a normal class with final static and constructor methods (hence the leftover static references here) but I decided that nobody would go through the hassle of extending for the few allowed methods so made the whole thing final.

paulbalandan · 2021-05-05T18:09:26Z

+ $config = clone config('App');
+ $config->indexPage = '';
+
+ return new static($uri, $config);


Suggested change
returnnewstatic($uri, $config);
returnnewself($uri, $config);

paulbalandan · 2021-05-05T18:09:49Z

+ $config = clone config('App');
+ $config->indexPage = '';


Suggested change
$config = cloneconfig('App');
$config->indexPage = '';
$config = cloneconfig('App');
$config->indexPage = '';

paulbalandan · 2021-05-05T18:10:09Z

+ return self::$current;
+ }
+
+ return static::fromRequest(Services::request());


Suggested change
returnstatic::fromRequest(Services::request());
returnself::fromRequest(Services::request());

paulbalandan · 2021-05-05T18:10:24Z

+ */
+	public static function to(string $uri)
+	{
+ return new static(rtrim($uri, '/ '));


Suggested change
returnnewstatic(rtrim($uri, '/ '));
returnnewself(rtrim($uri, '/ '));

paulbalandan · 2021-05-05T18:13:57Z

+ *
+ * @param string $uri Additional URI string to include
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:12Z

+	/**
+ * Returns an instance representing the current URL.
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:33Z

+ *
+ * @param string $uri
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:44Z

+ *
+ * @param string $uri Named or reverse route
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:56Z

+ *
+ * @param IncomingRequest $request
+ *
+ * @return static


Suggested change
* @return static
* @return self

lonnieezell · 2021-05-05T18:56:51Z

I'll be honest. I'm not a fan of having both. I understand your reasoning but think it will make things more complex/confusing for the developers. I wish we could find a way to refactor the current class to work in conjunction with the url helpers.

That being said, if the rest of the team is fine with it then I'll shut up :)

MGatner · 2021-05-05T19:04:38Z

@lonnieezell I understand. Let's think of ways we could make this better. I do think the helpers could be "beefed" up to cover this class but we would lose the ability to distinguish path parts (see my comment about "what is project" versus "what is path"). That might be an acceptable tradeoff? But there are some real issues with the current setup that need addressing. Just an example, IncomingRequest is clear that it only builds out the URI it needs for itself:

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 153 to 160 in 2e0e020

// Determine the current URI
// NOTE: This WILL NOT match the actual URL in the browser since for
// everything this cares about (and the router, etc) is the portion
// AFTER the script name. So, if hosted in a sub-folder this will
// appear different than actual URL. If you need that, use current_url().
$this->uri = $uri; 
$this->detectURI($config->uriProtocol, $config->baseURL); 

If you need that, use current_url().

... but then this is the entirety of current_url():

$uri = clone Services::request()->uri;

I believe that a support class (in this case URL) gives us a lot more flexibility than the helper functions for working with framework URIs - maybe we could make it an internal class? or move it out of HTTP?

MGatner · 2021-05-05T01:45:53Z

Description
"Stage Three", this PR introduces a companion class to URI: URL. The URL class handles internal URIs (i.e. specific to the project) in the following manner:

A URL is always constructed with a relative path
A URL will always use the App config values from its construction (baseURL, indexPage, forceGlobalSecureRequest)
A URL is immutable, not affected by changes to $_SERVER or services once it has been created

I would like URL to become the definitive source for internal URLs within the framework (e.g. replace the logic in base_url() and current_url(), be used by RedirectResponse, etc) but for now this PR is just introducing the concept and demonstrating the tests. Please be thorough with any reviews, since URLs have been a long pain point (particularly around subfolders).

Note: This PR relies on #4644 and #4646.

Checklist:

lonnieezell · 2021-05-05T03:06:41Z

Haven't looked at the code yet but my initial thought is - this is making the framework more complex and confusing than it needs to be. Can we just start prepping for a bigger release and fix this in the existing URI class? Have two classes so closely related to each other are going to be super confusing for any devs that dig into the inner workings. Or just to those who's IDE starts to suggest both classes for them.

Additionally - isn't the purpose of site_url to be base_url but taking index.php settings into account?

MGatner · 2021-05-05T11:58:06Z

We really need both. It doesn't need to be implemented this way, but we need to be able to do two things:

Build out a URI representing any valid URI for use anywhere one would want a URI
Build "project-specific" URIs, based off the configuration in App and (potentially) any special routes

Currently this is a real problem:

CodeIgniter4/system/HTTP/URI.php

Lines 566 to 584 in 4cc1452

// If hosted in a sub-folder, we will have additional
// segments that show up prior to the URI path we just
// grabbed from the request, so add it on if necessary.
$config = config(App::class); 
$baseUri = newself($config->baseURL); 
$basePath = trim($baseUri->getPath(), '/') . '/'; 
$path = $this->getPath(); 
$trimPath = ltrim($path, '/'); 
if ($basePath !== '/' && strpos($trimPath, $basePath) !== 0) 
 { 
$path = $basePath . $trimPath; 
 } 
// force https if needed
if ($config->forceGlobalSecureRequests) 
 { 
$this->setScheme('https'); 
 } 

I've tried to mitigate it in #4646 but this "bleeds" point 2 into point 1. URI should not know or care about the framework settings because it makes it impossible to create any URI one might want because the framework settings take over.

Currently we handle "project-specific" URIs with the URL Helper functions (base_url(), site_url(), current_url()) - the problem with these is that the final project has no concept of "what is project" versus "what is path" (e.g. http://example.com/foo/bar - is the baseURLhttp://example.com/ with "foo/bar" path? or is it http://example.com/foo/ with "bar" path?) so I decided to go with a class solution so we have some way of storing and accessing that info.

I originally had URL as FrameworkURI that was an extension of URI, but there are too many setters in URI that could "violate" an internal URI so I split it out; at that point I realized it was basically a full URL so went with the name, but I'm open to changing it.

MGatner · 2021-05-05T12:56:59Z

isn't the purpose of site_url to be base_url but taking index.php settings into account

Exactly. site_url() amounts to "take base_url(), add App::$indexPage, add any given path, be smart about slashes." URL::to() tries to encompass this behavior but with a little more utility of resolving named and reverse routes.

paulbalandan · 2021-05-05T18:08:45Z

+ */
+	public static function base()
+	{
+ return static::public('');


Since the class is final, then inheritance is no more possible. So, there's no need to use the static keyword. Instead use self.
Suggested change
returnstatic::public('');
returnself::public('');

On the topic, how do you think the class should be scoped? Originally I had a normal class with final static and constructor methods (hence the leftover static references here) but I decided that nobody would go through the hassle of extending for the few allowed methods so made the whole thing final.

paulbalandan · 2021-05-05T18:09:26Z

+ $config = clone config('App');
+ $config->indexPage = '';
+
+ return new static($uri, $config);


Suggested change
returnnewstatic($uri, $config);
returnnewself($uri, $config);

paulbalandan · 2021-05-05T18:09:49Z

+ $config = clone config('App');
+ $config->indexPage = '';


Suggested change
$config = cloneconfig('App');
$config->indexPage = '';
$config = cloneconfig('App');
$config->indexPage = '';

paulbalandan · 2021-05-05T18:10:09Z

+ return self::$current;
+ }
+
+ return static::fromRequest(Services::request());


Suggested change
returnstatic::fromRequest(Services::request());
returnself::fromRequest(Services::request());

paulbalandan · 2021-05-05T18:10:24Z

+ */
+	public static function to(string $uri)
+	{
+ return new static(rtrim($uri, '/ '));


Suggested change
returnnewstatic(rtrim($uri, '/ '));
returnnewself(rtrim($uri, '/ '));

paulbalandan · 2021-05-05T18:13:57Z

+ *
+ * @param string $uri Additional URI string to include
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:12Z

+	/**
+ * Returns an instance representing the current URL.
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:33Z

+ *
+ * @param string $uri
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:44Z

+ *
+ * @param string $uri Named or reverse route
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:56Z

+ *
+ * @param IncomingRequest $request
+ *
+ * @return static


Suggested change
* @return static
* @return self

lonnieezell · 2021-05-05T18:56:51Z

I'll be honest. I'm not a fan of having both. I understand your reasoning but think it will make things more complex/confusing for the developers. I wish we could find a way to refactor the current class to work in conjunction with the url helpers.

That being said, if the rest of the team is fine with it then I'll shut up :)

MGatner · 2021-05-05T19:04:38Z

@lonnieezell I understand. Let's think of ways we could make this better. I do think the helpers could be "beefed" up to cover this class but we would lose the ability to distinguish path parts (see my comment about "what is project" versus "what is path"). That might be an acceptable tradeoff? But there are some real issues with the current setup that need addressing. Just an example, IncomingRequest is clear that it only builds out the URI it needs for itself:

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 153 to 160 in 2e0e020

// Determine the current URI
// NOTE: This WILL NOT match the actual URL in the browser since for
// everything this cares about (and the router, etc) is the portion
// AFTER the script name. So, if hosted in a sub-folder this will
// appear different than actual URL. If you need that, use current_url().
$this->uri = $uri; 
$this->detectURI($config->uriProtocol, $config->baseURL); 

If you need that, use current_url().

... but then this is the entirety of current_url():

$uri = clone Services::request()->uri;

I believe that a support class (in this case URL) gives us a lot more flexibility than the helper functions for working with framework URIs - maybe we could make it an internal class? or move it out of HTTP?

MGatner · 2021-05-05T01:45:53Z

Description
"Stage Three", this PR introduces a companion class to URI: URL. The URL class handles internal URIs (i.e. specific to the project) in the following manner:

A URL is always constructed with a relative path
A URL will always use the App config values from its construction (baseURL, indexPage, forceGlobalSecureRequest)
A URL is immutable, not affected by changes to $_SERVER or services once it has been created

I would like URL to become the definitive source for internal URLs within the framework (e.g. replace the logic in base_url() and current_url(), be used by RedirectResponse, etc) but for now this PR is just introducing the concept and demonstrating the tests. Please be thorough with any reviews, since URLs have been a long pain point (particularly around subfolders).

Note: This PR relies on #4644 and #4646.

Checklist:

lonnieezell · 2021-05-05T03:06:41Z

Haven't looked at the code yet but my initial thought is - this is making the framework more complex and confusing than it needs to be. Can we just start prepping for a bigger release and fix this in the existing URI class? Have two classes so closely related to each other are going to be super confusing for any devs that dig into the inner workings. Or just to those who's IDE starts to suggest both classes for them.

Additionally - isn't the purpose of site_url to be base_url but taking index.php settings into account?

MGatner · 2021-05-05T11:58:06Z

We really need both. It doesn't need to be implemented this way, but we need to be able to do two things:

Build out a URI representing any valid URI for use anywhere one would want a URI
Build "project-specific" URIs, based off the configuration in App and (potentially) any special routes

Currently this is a real problem:

CodeIgniter4/system/HTTP/URI.php

Lines 566 to 584 in 4cc1452

// If hosted in a sub-folder, we will have additional
// segments that show up prior to the URI path we just
// grabbed from the request, so add it on if necessary.
$config = config(App::class); 
$baseUri = newself($config->baseURL); 
$basePath = trim($baseUri->getPath(), '/') . '/'; 
$path = $this->getPath(); 
$trimPath = ltrim($path, '/'); 
if ($basePath !== '/' && strpos($trimPath, $basePath) !== 0) 
 { 
$path = $basePath . $trimPath; 
 } 
// force https if needed
if ($config->forceGlobalSecureRequests) 
 { 
$this->setScheme('https'); 
 } 

I've tried to mitigate it in #4646 but this "bleeds" point 2 into point 1. URI should not know or care about the framework settings because it makes it impossible to create any URI one might want because the framework settings take over.

Currently we handle "project-specific" URIs with the URL Helper functions (base_url(), site_url(), current_url()) - the problem with these is that the final project has no concept of "what is project" versus "what is path" (e.g. http://example.com/foo/bar - is the baseURLhttp://example.com/ with "foo/bar" path? or is it http://example.com/foo/ with "bar" path?) so I decided to go with a class solution so we have some way of storing and accessing that info.

I originally had URL as FrameworkURI that was an extension of URI, but there are too many setters in URI that could "violate" an internal URI so I split it out; at that point I realized it was basically a full URL so went with the name, but I'm open to changing it.

MGatner · 2021-05-05T12:56:59Z

isn't the purpose of site_url to be base_url but taking index.php settings into account

Exactly. site_url() amounts to "take base_url(), add App::$indexPage, add any given path, be smart about slashes." URL::to() tries to encompass this behavior but with a little more utility of resolving named and reverse routes.

paulbalandan · 2021-05-05T18:08:45Z

+ */
+	public static function base()
+	{
+ return static::public('');


Since the class is final, then inheritance is no more possible. So, there's no need to use the static keyword. Instead use self.
Suggested change
returnstatic::public('');
returnself::public('');

On the topic, how do you think the class should be scoped? Originally I had a normal class with final static and constructor methods (hence the leftover static references here) but I decided that nobody would go through the hassle of extending for the few allowed methods so made the whole thing final.

paulbalandan · 2021-05-05T18:09:26Z

+ $config = clone config('App');
+ $config->indexPage = '';
+
+ return new static($uri, $config);


Suggested change
returnnewstatic($uri, $config);
returnnewself($uri, $config);

paulbalandan · 2021-05-05T18:09:49Z

+ $config = clone config('App');
+ $config->indexPage = '';


Suggested change
$config = cloneconfig('App');
$config->indexPage = '';
$config = cloneconfig('App');
$config->indexPage = '';

paulbalandan · 2021-05-05T18:10:09Z

+ return self::$current;
+ }
+
+ return static::fromRequest(Services::request());


Suggested change
returnstatic::fromRequest(Services::request());
returnself::fromRequest(Services::request());

paulbalandan · 2021-05-05T18:10:24Z

+ */
+	public static function to(string $uri)
+	{
+ return new static(rtrim($uri, '/ '));


Suggested change
returnnewstatic(rtrim($uri, '/ '));
returnnewself(rtrim($uri, '/ '));

paulbalandan · 2021-05-05T18:13:57Z

+ *
+ * @param string $uri Additional URI string to include
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:12Z

+	/**
+ * Returns an instance representing the current URL.
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:33Z

+ *
+ * @param string $uri
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:44Z

+ *
+ * @param string $uri Named or reverse route
+ *
+ * @return static


Suggested change
* @return static
* @return self

paulbalandan · 2021-05-05T18:14:56Z

+ *
+ * @param IncomingRequest $request
+ *
+ * @return static


Suggested change
* @return static
* @return self

lonnieezell · 2021-05-05T18:56:51Z

I'll be honest. I'm not a fan of having both. I understand your reasoning but think it will make things more complex/confusing for the developers. I wish we could find a way to refactor the current class to work in conjunction with the url helpers.

That being said, if the rest of the team is fine with it then I'll shut up :)

MGatner · 2021-05-05T19:04:38Z

@lonnieezell I understand. Let's think of ways we could make this better. I do think the helpers could be "beefed" up to cover this class but we would lose the ability to distinguish path parts (see my comment about "what is project" versus "what is path"). That might be an acceptable tradeoff? But there are some real issues with the current setup that need addressing. Just an example, IncomingRequest is clear that it only builds out the URI it needs for itself:

CodeIgniter4/system/HTTP/IncomingRequest.php

Lines 153 to 160 in 2e0e020

// Determine the current URI
// NOTE: This WILL NOT match the actual URL in the browser since for
// everything this cares about (and the router, etc) is the portion
// AFTER the script name. So, if hosted in a sub-folder this will
// appear different than actual URL. If you need that, use current_url().
$this->uri = $uri; 
$this->detectURI($config->uriProtocol, $config->baseURL); 

If you need that, use current_url().

... but then this is the entirety of current_url():

$uri = clone Services::request()->uri;

I believe that a support class (in this case URL) gives us a lot more flexibility than the helper functions for working with framework URIs - maybe we could make it an internal class? or move it out of HTTP?

MGatner added 3 commits May 5, 2021 15:16

Add URL class
42a60ce

Fix base URL logic
10b605b

Refactor static methods
9fcab55

MGatner force-pushed the urls branch from 4f3ba28 to 9fcab55Compare May 5, 2021 15:34

Add spoofing, fix tests
16c364e

paulbalandan reviewed May 5, 2021
View reviewed changes

MGatner mentioned this pull request May 6, 2021
Refactor URI detection #4651
Merged
5 tasks

MGatner closed this May 11, 2021

MGatner mentioned this pull request Feb 16, 2023
feat: add SiteURI class #7252
Merged
5 tasks

MGatner added 3 commits May 5, 2021 15:16

Add URL class
42a60ce

Fix base URL logic
10b605b

Refactor static methods
9fcab55

MGatner force-pushed the urls branch from 4f3ba28 to 9fcab55Compare May 5, 2021 15:34

Add spoofing, fix tests
16c364e

paulbalandan reviewed May 5, 2021
View reviewed changes

MGatner mentioned this pull request May 6, 2021
Refactor URI detection #4651
Merged
5 tasks

MGatner closed this May 11, 2021

MGatner mentioned this pull request Feb 16, 2023
feat: add SiteURI class #7252
Merged
5 tasks

MGatner added 3 commits May 5, 2021 15:16

Add URL class
42a60ce

Fix base URL logic
10b605b

Refactor static methods
9fcab55

MGatner force-pushed the urls branch from 4f3ba28 to 9fcab55Compare May 5, 2021 15:34

Add spoofing, fix tests
16c364e

paulbalandan reviewed May 5, 2021
View reviewed changes

MGatner mentioned this pull request May 6, 2021
Refactor URI detection #4651
Merged
5 tasks

MGatner closed this May 11, 2021

MGatner mentioned this pull request Feb 16, 2023
feat: add SiteURI class #7252
Merged
5 tasks

MGatner added 3 commits May 5, 2021 15:16

Add URL class
42a60ce

Fix base URL logic
10b605b

Refactor static methods
9fcab55

MGatner force-pushed the urls branch from 4f3ba28 to 9fcab55Compare May 5, 2021 15:34

Add spoofing, fix tests
16c364e

paulbalandan reviewed May 5, 2021
View reviewed changes

MGatner mentioned this pull request May 6, 2021
Refactor URI detection #4651
Merged
5 tasks

MGatner closed this May 11, 2021

MGatner mentioned this pull request Feb 16, 2023
feat: add SiteURI class #7252
Merged
5 tasks

MGatner added 3 commits May 5, 2021 15:16

Add URL class
42a60ce

Fix base URL logic
10b605b

Refactor static methods
9fcab55

MGatner force-pushed the urls branch from 4f3ba28 to 9fcab55Compare May 5, 2021 15:34

Add spoofing, fix tests
16c364e

paulbalandan reviewed May 5, 2021
View reviewed changes

MGatner mentioned this pull request May 6, 2021
Refactor URI detection #4651
Merged
5 tasks

MGatner closed this May 11, 2021

MGatner mentioned this pull request Feb 16, 2023
feat: add SiteURI class #7252
Merged
5 tasks

MGatner added 3 commits May 5, 2021 15:16

Add URL class
42a60ce

Fix base URL logic
10b605b

Refactor static methods
9fcab55

MGatner force-pushed the urls branch from 4f3ba28 to 9fcab55Compare May 5, 2021 15:34

Add spoofing, fix tests
16c364e

paulbalandan reviewed May 5, 2021
View reviewed changes

MGatner mentioned this pull request May 6, 2021
Refactor URI detection #4651
Merged
5 tasks

MGatner closed this May 11, 2021

MGatner mentioned this pull request Feb 16, 2023
feat: add SiteURI class #7252
Merged
5 tasks

MGatner added 3 commits May 5, 2021 15:16

Add URL class
42a60ce

Fix base URL logic
10b605b

Refactor static methods
9fcab55

MGatner force-pushed the urls branch from 4f3ba28 to 9fcab55Compare May 5, 2021 15:34

Add spoofing, fix tests
16c364e

paulbalandan reviewed May 5, 2021
View reviewed changes

MGatner mentioned this pull request May 6, 2021
Refactor URI detection #4651
Merged
5 tasks

MGatner closed this May 11, 2021

MGatner mentioned this pull request Feb 16, 2023
feat: add SiteURI class #7252
Merged
5 tasks

MGatner added 3 commits May 5, 2021 15:16

Add URL class
42a60ce

Fix base URL logic
10b605b

Refactor static methods
9fcab55

MGatner force-pushed the urls branch from 4f3ba28 to 9fcab55Compare May 5, 2021 15:34

Add spoofing, fix tests
16c364e

paulbalandan reviewed May 5, 2021
View reviewed changes

MGatner mentioned this pull request May 6, 2021
Refactor URI detection #4651
Merged
5 tasks

MGatner closed this May 11, 2021

MGatner mentioned this pull request Feb 16, 2023
feat: add SiteURI class #7252
Merged
5 tasks

	returnnewstatic($uri, $config);
	returnnewself($uri, $config);

	returnstatic::fromRequest(Services::request());
	returnself::fromRequest(Services::request());

	returnnewstatic(rtrim($uri, '/ '));
	returnnewself(rtrim($uri, '/ '));

Uh oh!

Conversation

MGatner commented May 5, 2021

Uh oh!

lonnieezell commented May 5, 2021

Uh oh!

MGatner commented May 5, 2021

Uh oh!

MGatner commented May 5, 2021

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!

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!

lonnieezell commented May 5, 2021

Uh oh!

MGatner commented May 5, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

MGatner commented May 5, 2021

Uh oh!

lonnieezell commented May 5, 2021

Uh oh!

MGatner commented May 5, 2021

Uh oh!

MGatner commented May 5, 2021

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!

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!

lonnieezell commented May 5, 2021

Uh oh!

MGatner commented May 5, 2021

Uh oh!