feat: add SiteURI class

[kenjis]

Description
This PR is needed for #7123

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

• add SiteURI class
  • The $routePath never starts with /.
  • If the path ends with /, the URI retains the trailing /.
  • getPath() returns the full URI path.
  • getRoutePath() returns the path relative to baseURL.

Related #5930, #7239, #7249, #7251

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

[michalsn]

Maybe this is a silly question, but... as I understand, this new class will bring full PSR-7 compatibility, right?

I don't see any related changes in the framework so we won't rely on it. The question is, who needs this class and for what?

Like... I saw your description:

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

But I'm afraid I don't fully understand the use cases.

[kenjis]

This PR only adds a new SiteURI class, and does not change the current framework behavior yet.

In the next PR(s), I will do:

• add a factory to create the SiteURI instance for the current URL: feat: add SiteURIFactory #7256
• set the SiteURI instance in the uri property of the Request
• use the SiteURI instance when getting the current URL like current_url()

About compatibility with PSR-7, the URI class will be cleaner because site-related functions like baseURL, will be separated into the SiteURI class. And the URI class will be easier to change.

Finally, I think URI and SiteURL can be made fully compatible (and having some extended functions) with PSR-7.

[MGatner]

I am obviously in favor of this but I want to be sure we get reviews from @lonnieezell and @iRedds and anyone else who was opposed to the idea originally.

[lonnieezell]

I was purposefully not commenting since my position was already known and it seemed I was overruled :)

I'm confused why we need a second class that does 99% of the same things as the URI class. The URI class is just for working with URIs in general, crafted to meet the RFC as best we could at the time. It seems like all other issues should be fixed where it's used, or the base class should be changed to support our needs.

The implementation itself is solid enough. Though if we're going to include it, we need docs and guidance on when to use which one.

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers. What is the point of this other then to throw confusion about which class people should use when? I figured it was to fix issues in how the framework was returning answers, but it doesn't seem like we're actually doing that.

[michalsn]

I agree with @lonnieezell. I don't understand why we need a second class to handle URLs. Can somebody give me a clear example of where it will be useful?

AFAIK the only issue that we have now with the URI class is that it does not follow PSR-7 (it's something path related).

Also, if the goal for both classes is to follow the PSR-7, what will be the difference? Wouldn't it be better to see some additional methods added to the current class instead?

[kenjis]

I don't understand why we need a second class to handle URLs.

Because we have already two kinds of concept URI,
and if we have two classes, It is easier to understand for us.

1. URI: a URI
2. SiteURI: a URI for a CI4 app site.

SiteURI has its own properties like baseURL, indexPage and we cannot change the URI path
inside baseURL (subfolders). Only the URI path after indexPage does matter.

Now we are already confusing 1. and 2. So we have two kinds of URI segments for one SiteURL.
See #7123 (comment)

[kenjis]

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers.

This fix (and refactoring) will be long story. This PR is just the first one.
We will use the new class in all places after all. Please wait for incoming PRs.

[MGatner]

I won't repeat my arguments, the original PR is here with discussion: #4647

I would like to point out that after doing a deep dive on URI handling and bug fixes both @kenjis and I came to the same conclusion, independently. I think the problem is larger than credited and may indeed merit the potential confusion/clutter of an additional class.

[lonnieezell]

After giving this a bit more thought, I think I'm ok with this on 2 conditions:

1. It's designed for internal use within the request class and URL helper functions. Not intended for public use or really even documented. This avoids the confusion to end users, while still providing a "URI in Request is the source of truth" that @MGatner was looking for in the previous PR.
2. It's saved for 5.0 (which we should be working on now....) since it's too big of a potential BC break.

[iRedds]

$uri = $this->request->getUri();
echo (string) $uri; // "http://localhost:8888/ci431/public/test?a=b" → Okayecho$uri->getPath(); // "test" → NG. It should be "/ci431/public/test" when following PSR-7

And I think that such behavior corresponds to PSR.

// for http://localhost:8888/ci431/public/test?a=bURI::getPath(); // => /ci431/public/test// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)// Accordingly, the leading slash will determine whether the path is absolute or rootless// for http://localhost:8888/{base_path}/test?a=bURI::withPath('/xxx'); // http://localhost:8888/xxx?a=b// for http://localhost:8888/{base_path}/test?a=bURI::withPath('xxx'); // http://localhost:8888/{base_path}/xxx?a=b

php-fig/fig-standards#503

[kenjis]

What is {base_path} in this case? How do you define it?

// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)

[iRedds]

@kenjis app.baseURL = 'http://localhost:8888{/ci431/public/}' <- base path

[kenjis]

I don't get it.
How do you set it in PSR-7 URI object?

[iRedds]

There are no objects in PSR-7. Interfaces only. That is, mandatory public methods for PSR compliance.
A class implementing UriInterface should not be limited to UriInterface methods and may contain a method that sets base_path or base_url or whatever is needed for it to work properly.

[kenjis]

Many frameworks provide the ability to get the "base path," usually considered the path up to and including the front controller. As an example, if the application is served at http://example.com/b2b/index.php, and the current URI used to request it is http://example.com/b2b/index.php/customer/register, the functionality to retrieve the base path would return /b2b/index.php. This value can then be used by routers to strip that path segment prior to attempting a match.
https://www.php-fig.org/psr/psr-7/meta/#why-is-no-functionality-included-for-retrieving-the-base-path

 * If an HTTP path is intended to be host-relative rather than path-relative
* then it must begin with a slash ("/"). HTTP paths not starting with a slash
* are assumed to be relative to some base path known to the application or
* consumer.
...
*/
public function withPath($path);

https://www.php-fig.org/psr/psr-7/

This means like this?

• if we call URI::withPath('foo/bar'), the path foo/bar is relative to some base path known to the application.
• the new URI instance getPath() will return foo/bar?

[kenjis]

<?phprequire__DIR__ . '/vendor/autoload.php';
useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
echo$uri . PHP_EOL; // http://localhost/ci4/foo/barecho$uri->getPath() . PHP_EOL; // /ci4/foo/bar

https://uri.thephpleague.com/uri/6.0/psr7/

[kenjis]

useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');

PHP Fatal error: Uncaught League\Uri\Exceptions\SyntaxError: If an authority is present the path must be empty or start with a /. in /.../league-uri/vendor/league/uri/src/Uri.php:921

[kenjis]

useSlim\Psr7\Factory\UriFactory;
$factory = newUriFactory();
$uri = $factory->createUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[kenjis]

useNyholm\Psr7\Uri;
$uri = newUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[iRedds]

• the new URI instance getPath() will return foo/bar?

empty string

[MGatner]

Looks great! I feel validated in the need for this given how much code is in SiteURI on top of URI. I appreciate your verbosity in naming variables and methods - it is easy to overlook pieces which has led to many bugs in the past. I think this will be a good step forward.

I assume the URL Helper methods get changed in the next PR? I haven't looked at that one yet...

[kenjis]

@TimexPeachtree@MGatner Thank you for approvals.

[kenjis]

Description
This PR is needed for #7123

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

• add SiteURI class
  • The $routePath never starts with /.
  • If the path ends with /, the URI retains the trailing /.
  • getPath() returns the full URI path.
  • getRoutePath() returns the path relative to baseURL.

Related #5930, #7239, #7249, #7251

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

[michalsn]

Maybe this is a silly question, but... as I understand, this new class will bring full PSR-7 compatibility, right?

I don't see any related changes in the framework so we won't rely on it. The question is, who needs this class and for what?

Like... I saw your description:

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

But I'm afraid I don't fully understand the use cases.

[kenjis]

This PR only adds a new SiteURI class, and does not change the current framework behavior yet.

In the next PR(s), I will do:

• add a factory to create the SiteURI instance for the current URL: feat: add SiteURIFactory #7256
• set the SiteURI instance in the uri property of the Request
• use the SiteURI instance when getting the current URL like current_url()

About compatibility with PSR-7, the URI class will be cleaner because site-related functions like baseURL, will be separated into the SiteURI class. And the URI class will be easier to change.

Finally, I think URI and SiteURL can be made fully compatible (and having some extended functions) with PSR-7.

[MGatner]

I am obviously in favor of this but I want to be sure we get reviews from @lonnieezell and @iRedds and anyone else who was opposed to the idea originally.

[lonnieezell]

I was purposefully not commenting since my position was already known and it seemed I was overruled :)

I'm confused why we need a second class that does 99% of the same things as the URI class. The URI class is just for working with URIs in general, crafted to meet the RFC as best we could at the time. It seems like all other issues should be fixed where it's used, or the base class should be changed to support our needs.

The implementation itself is solid enough. Though if we're going to include it, we need docs and guidance on when to use which one.

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers. What is the point of this other then to throw confusion about which class people should use when? I figured it was to fix issues in how the framework was returning answers, but it doesn't seem like we're actually doing that.

[michalsn]

I agree with @lonnieezell. I don't understand why we need a second class to handle URLs. Can somebody give me a clear example of where it will be useful?

AFAIK the only issue that we have now with the URI class is that it does not follow PSR-7 (it's something path related).

Also, if the goal for both classes is to follow the PSR-7, what will be the difference? Wouldn't it be better to see some additional methods added to the current class instead?

[kenjis]

I don't understand why we need a second class to handle URLs.

Because we have already two kinds of concept URI,
and if we have two classes, It is easier to understand for us.

1. URI: a URI
2. SiteURI: a URI for a CI4 app site.

SiteURI has its own properties like baseURL, indexPage and we cannot change the URI path
inside baseURL (subfolders). Only the URI path after indexPage does matter.

Now we are already confusing 1. and 2. So we have two kinds of URI segments for one SiteURL.
See #7123 (comment)

[kenjis]

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers.

This fix (and refactoring) will be long story. This PR is just the first one.
We will use the new class in all places after all. Please wait for incoming PRs.

[MGatner]

I won't repeat my arguments, the original PR is here with discussion: #4647

I would like to point out that after doing a deep dive on URI handling and bug fixes both @kenjis and I came to the same conclusion, independently. I think the problem is larger than credited and may indeed merit the potential confusion/clutter of an additional class.

[lonnieezell]

After giving this a bit more thought, I think I'm ok with this on 2 conditions:

1. It's designed for internal use within the request class and URL helper functions. Not intended for public use or really even documented. This avoids the confusion to end users, while still providing a "URI in Request is the source of truth" that @MGatner was looking for in the previous PR.
2. It's saved for 5.0 (which we should be working on now....) since it's too big of a potential BC break.

[iRedds]

$uri = $this->request->getUri();
echo (string) $uri; // "http://localhost:8888/ci431/public/test?a=b" → Okayecho$uri->getPath(); // "test" → NG. It should be "/ci431/public/test" when following PSR-7

And I think that such behavior corresponds to PSR.

// for http://localhost:8888/ci431/public/test?a=bURI::getPath(); // => /ci431/public/test// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)// Accordingly, the leading slash will determine whether the path is absolute or rootless// for http://localhost:8888/{base_path}/test?a=bURI::withPath('/xxx'); // http://localhost:8888/xxx?a=b// for http://localhost:8888/{base_path}/test?a=bURI::withPath('xxx'); // http://localhost:8888/{base_path}/xxx?a=b

php-fig/fig-standards#503

[kenjis]

What is {base_path} in this case? How do you define it?

// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)

[iRedds]

@kenjis app.baseURL = 'http://localhost:8888{/ci431/public/}' <- base path

[kenjis]

I don't get it.
How do you set it in PSR-7 URI object?

[iRedds]

There are no objects in PSR-7. Interfaces only. That is, mandatory public methods for PSR compliance.
A class implementing UriInterface should not be limited to UriInterface methods and may contain a method that sets base_path or base_url or whatever is needed for it to work properly.

[kenjis]

Many frameworks provide the ability to get the "base path," usually considered the path up to and including the front controller. As an example, if the application is served at http://example.com/b2b/index.php, and the current URI used to request it is http://example.com/b2b/index.php/customer/register, the functionality to retrieve the base path would return /b2b/index.php. This value can then be used by routers to strip that path segment prior to attempting a match.
https://www.php-fig.org/psr/psr-7/meta/#why-is-no-functionality-included-for-retrieving-the-base-path

 * If an HTTP path is intended to be host-relative rather than path-relative
* then it must begin with a slash ("/"). HTTP paths not starting with a slash
* are assumed to be relative to some base path known to the application or
* consumer.
...
*/
public function withPath($path);

https://www.php-fig.org/psr/psr-7/

This means like this?

• if we call URI::withPath('foo/bar'), the path foo/bar is relative to some base path known to the application.
• the new URI instance getPath() will return foo/bar?

[kenjis]

<?phprequire__DIR__ . '/vendor/autoload.php';
useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
echo$uri . PHP_EOL; // http://localhost/ci4/foo/barecho$uri->getPath() . PHP_EOL; // /ci4/foo/bar

https://uri.thephpleague.com/uri/6.0/psr7/

[kenjis]

useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');

PHP Fatal error: Uncaught League\Uri\Exceptions\SyntaxError: If an authority is present the path must be empty or start with a /. in /.../league-uri/vendor/league/uri/src/Uri.php:921

[kenjis]

useSlim\Psr7\Factory\UriFactory;
$factory = newUriFactory();
$uri = $factory->createUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[kenjis]

useNyholm\Psr7\Uri;
$uri = newUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[iRedds]

• the new URI instance getPath() will return foo/bar?

empty string

[MGatner]

Looks great! I feel validated in the need for this given how much code is in SiteURI on top of URI. I appreciate your verbosity in naming variables and methods - it is easy to overlook pieces which has led to many bugs in the past. I think this will be a good step forward.

I assume the URL Helper methods get changed in the next PR? I haven't looked at that one yet...

[kenjis]

@TimexPeachtree@MGatner Thank you for approvals.

[kenjis]

Description
This PR is needed for #7123

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

• add SiteURI class
  • The $routePath never starts with /.
  • If the path ends with /, the URI retains the trailing /.
  • getPath() returns the full URI path.
  • getRoutePath() returns the path relative to baseURL.

Related #5930, #7239, #7249, #7251

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

[michalsn]

Maybe this is a silly question, but... as I understand, this new class will bring full PSR-7 compatibility, right?

I don't see any related changes in the framework so we won't rely on it. The question is, who needs this class and for what?

Like... I saw your description:

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

But I'm afraid I don't fully understand the use cases.

[kenjis]

This PR only adds a new SiteURI class, and does not change the current framework behavior yet.

In the next PR(s), I will do:

• add a factory to create the SiteURI instance for the current URL: feat: add SiteURIFactory #7256
• set the SiteURI instance in the uri property of the Request
• use the SiteURI instance when getting the current URL like current_url()

About compatibility with PSR-7, the URI class will be cleaner because site-related functions like baseURL, will be separated into the SiteURI class. And the URI class will be easier to change.

Finally, I think URI and SiteURL can be made fully compatible (and having some extended functions) with PSR-7.

[MGatner]

I am obviously in favor of this but I want to be sure we get reviews from @lonnieezell and @iRedds and anyone else who was opposed to the idea originally.

[lonnieezell]

I was purposefully not commenting since my position was already known and it seemed I was overruled :)

I'm confused why we need a second class that does 99% of the same things as the URI class. The URI class is just for working with URIs in general, crafted to meet the RFC as best we could at the time. It seems like all other issues should be fixed where it's used, or the base class should be changed to support our needs.

The implementation itself is solid enough. Though if we're going to include it, we need docs and guidance on when to use which one.

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers. What is the point of this other then to throw confusion about which class people should use when? I figured it was to fix issues in how the framework was returning answers, but it doesn't seem like we're actually doing that.

[michalsn]

I agree with @lonnieezell. I don't understand why we need a second class to handle URLs. Can somebody give me a clear example of where it will be useful?

AFAIK the only issue that we have now with the URI class is that it does not follow PSR-7 (it's something path related).

Also, if the goal for both classes is to follow the PSR-7, what will be the difference? Wouldn't it be better to see some additional methods added to the current class instead?

[kenjis]

I don't understand why we need a second class to handle URLs.

Because we have already two kinds of concept URI,
and if we have two classes, It is easier to understand for us.

1. URI: a URI
2. SiteURI: a URI for a CI4 app site.

SiteURI has its own properties like baseURL, indexPage and we cannot change the URI path
inside baseURL (subfolders). Only the URI path after indexPage does matter.

Now we are already confusing 1. and 2. So we have two kinds of URI segments for one SiteURL.
See #7123 (comment)

[kenjis]

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers.

This fix (and refactoring) will be long story. This PR is just the first one.
We will use the new class in all places after all. Please wait for incoming PRs.

[MGatner]

I won't repeat my arguments, the original PR is here with discussion: #4647

I would like to point out that after doing a deep dive on URI handling and bug fixes both @kenjis and I came to the same conclusion, independently. I think the problem is larger than credited and may indeed merit the potential confusion/clutter of an additional class.

[lonnieezell]

After giving this a bit more thought, I think I'm ok with this on 2 conditions:

1. It's designed for internal use within the request class and URL helper functions. Not intended for public use or really even documented. This avoids the confusion to end users, while still providing a "URI in Request is the source of truth" that @MGatner was looking for in the previous PR.
2. It's saved for 5.0 (which we should be working on now....) since it's too big of a potential BC break.

[iRedds]

$uri = $this->request->getUri();
echo (string) $uri; // "http://localhost:8888/ci431/public/test?a=b" → Okayecho$uri->getPath(); // "test" → NG. It should be "/ci431/public/test" when following PSR-7

And I think that such behavior corresponds to PSR.

// for http://localhost:8888/ci431/public/test?a=bURI::getPath(); // => /ci431/public/test// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)// Accordingly, the leading slash will determine whether the path is absolute or rootless// for http://localhost:8888/{base_path}/test?a=bURI::withPath('/xxx'); // http://localhost:8888/xxx?a=b// for http://localhost:8888/{base_path}/test?a=bURI::withPath('xxx'); // http://localhost:8888/{base_path}/xxx?a=b

php-fig/fig-standards#503

[kenjis]

What is {base_path} in this case? How do you define it?

// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)

[iRedds]

@kenjis app.baseURL = 'http://localhost:8888{/ci431/public/}' <- base path

[kenjis]

I don't get it.
How do you set it in PSR-7 URI object?

[iRedds]

There are no objects in PSR-7. Interfaces only. That is, mandatory public methods for PSR compliance.
A class implementing UriInterface should not be limited to UriInterface methods and may contain a method that sets base_path or base_url or whatever is needed for it to work properly.

[kenjis]

Many frameworks provide the ability to get the "base path," usually considered the path up to and including the front controller. As an example, if the application is served at http://example.com/b2b/index.php, and the current URI used to request it is http://example.com/b2b/index.php/customer/register, the functionality to retrieve the base path would return /b2b/index.php. This value can then be used by routers to strip that path segment prior to attempting a match.
https://www.php-fig.org/psr/psr-7/meta/#why-is-no-functionality-included-for-retrieving-the-base-path

 * If an HTTP path is intended to be host-relative rather than path-relative
* then it must begin with a slash ("/"). HTTP paths not starting with a slash
* are assumed to be relative to some base path known to the application or
* consumer.
...
*/
public function withPath($path);

https://www.php-fig.org/psr/psr-7/

This means like this?

• if we call URI::withPath('foo/bar'), the path foo/bar is relative to some base path known to the application.
• the new URI instance getPath() will return foo/bar?

[kenjis]

<?phprequire__DIR__ . '/vendor/autoload.php';
useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
echo$uri . PHP_EOL; // http://localhost/ci4/foo/barecho$uri->getPath() . PHP_EOL; // /ci4/foo/bar

https://uri.thephpleague.com/uri/6.0/psr7/

[kenjis]

useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');

PHP Fatal error: Uncaught League\Uri\Exceptions\SyntaxError: If an authority is present the path must be empty or start with a /. in /.../league-uri/vendor/league/uri/src/Uri.php:921

[kenjis]

useSlim\Psr7\Factory\UriFactory;
$factory = newUriFactory();
$uri = $factory->createUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[kenjis]

useNyholm\Psr7\Uri;
$uri = newUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[iRedds]

• the new URI instance getPath() will return foo/bar?

empty string

[MGatner]

Looks great! I feel validated in the need for this given how much code is in SiteURI on top of URI. I appreciate your verbosity in naming variables and methods - it is easy to overlook pieces which has led to many bugs in the past. I think this will be a good step forward.

I assume the URL Helper methods get changed in the next PR? I haven't looked at that one yet...

[kenjis]

@TimexPeachtree@MGatner Thank you for approvals.

[kenjis]

Description
This PR is needed for #7123

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

• add SiteURI class
  • The $routePath never starts with /.
  • If the path ends with /, the URI retains the trailing /.
  • getPath() returns the full URI path.
  • getRoutePath() returns the path relative to baseURL.

Related #5930, #7239, #7249, #7251

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

[michalsn]

Maybe this is a silly question, but... as I understand, this new class will bring full PSR-7 compatibility, right?

I don't see any related changes in the framework so we won't rely on it. The question is, who needs this class and for what?

Like... I saw your description:

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

But I'm afraid I don't fully understand the use cases.

[kenjis]

This PR only adds a new SiteURI class, and does not change the current framework behavior yet.

In the next PR(s), I will do:

• add a factory to create the SiteURI instance for the current URL: feat: add SiteURIFactory #7256
• set the SiteURI instance in the uri property of the Request
• use the SiteURI instance when getting the current URL like current_url()

About compatibility with PSR-7, the URI class will be cleaner because site-related functions like baseURL, will be separated into the SiteURI class. And the URI class will be easier to change.

Finally, I think URI and SiteURL can be made fully compatible (and having some extended functions) with PSR-7.

[MGatner]

I am obviously in favor of this but I want to be sure we get reviews from @lonnieezell and @iRedds and anyone else who was opposed to the idea originally.

[lonnieezell]

I was purposefully not commenting since my position was already known and it seemed I was overruled :)

I'm confused why we need a second class that does 99% of the same things as the URI class. The URI class is just for working with URIs in general, crafted to meet the RFC as best we could at the time. It seems like all other issues should be fixed where it's used, or the base class should be changed to support our needs.

The implementation itself is solid enough. Though if we're going to include it, we need docs and guidance on when to use which one.

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers. What is the point of this other then to throw confusion about which class people should use when? I figured it was to fix issues in how the framework was returning answers, but it doesn't seem like we're actually doing that.

[michalsn]

I agree with @lonnieezell. I don't understand why we need a second class to handle URLs. Can somebody give me a clear example of where it will be useful?

AFAIK the only issue that we have now with the URI class is that it does not follow PSR-7 (it's something path related).

Also, if the goal for both classes is to follow the PSR-7, what will be the difference? Wouldn't it be better to see some additional methods added to the current class instead?

[kenjis]

I don't understand why we need a second class to handle URLs.

Because we have already two kinds of concept URI,
and if we have two classes, It is easier to understand for us.

1. URI: a URI
2. SiteURI: a URI for a CI4 app site.

SiteURI has its own properties like baseURL, indexPage and we cannot change the URI path
inside baseURL (subfolders). Only the URI path after indexPage does matter.

Now we are already confusing 1. and 2. So we have two kinds of URI segments for one SiteURL.
See #7123 (comment)

[kenjis]

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers.

This fix (and refactoring) will be long story. This PR is just the first one.
We will use the new class in all places after all. Please wait for incoming PRs.

[MGatner]

I won't repeat my arguments, the original PR is here with discussion: #4647

I would like to point out that after doing a deep dive on URI handling and bug fixes both @kenjis and I came to the same conclusion, independently. I think the problem is larger than credited and may indeed merit the potential confusion/clutter of an additional class.

[lonnieezell]

After giving this a bit more thought, I think I'm ok with this on 2 conditions:

1. It's designed for internal use within the request class and URL helper functions. Not intended for public use or really even documented. This avoids the confusion to end users, while still providing a "URI in Request is the source of truth" that @MGatner was looking for in the previous PR.
2. It's saved for 5.0 (which we should be working on now....) since it's too big of a potential BC break.

[iRedds]

$uri = $this->request->getUri();
echo (string) $uri; // "http://localhost:8888/ci431/public/test?a=b" → Okayecho$uri->getPath(); // "test" → NG. It should be "/ci431/public/test" when following PSR-7

And I think that such behavior corresponds to PSR.

// for http://localhost:8888/ci431/public/test?a=bURI::getPath(); // => /ci431/public/test// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)// Accordingly, the leading slash will determine whether the path is absolute or rootless// for http://localhost:8888/{base_path}/test?a=bURI::withPath('/xxx'); // http://localhost:8888/xxx?a=b// for http://localhost:8888/{base_path}/test?a=bURI::withPath('xxx'); // http://localhost:8888/{base_path}/xxx?a=b

php-fig/fig-standards#503

[kenjis]

What is {base_path} in this case? How do you define it?

// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)

[iRedds]

@kenjis app.baseURL = 'http://localhost:8888{/ci431/public/}' <- base path

[kenjis]

I don't get it.
How do you set it in PSR-7 URI object?

[iRedds]

There are no objects in PSR-7. Interfaces only. That is, mandatory public methods for PSR compliance.
A class implementing UriInterface should not be limited to UriInterface methods and may contain a method that sets base_path or base_url or whatever is needed for it to work properly.

[kenjis]

Many frameworks provide the ability to get the "base path," usually considered the path up to and including the front controller. As an example, if the application is served at http://example.com/b2b/index.php, and the current URI used to request it is http://example.com/b2b/index.php/customer/register, the functionality to retrieve the base path would return /b2b/index.php. This value can then be used by routers to strip that path segment prior to attempting a match.
https://www.php-fig.org/psr/psr-7/meta/#why-is-no-functionality-included-for-retrieving-the-base-path

 * If an HTTP path is intended to be host-relative rather than path-relative
* then it must begin with a slash ("/"). HTTP paths not starting with a slash
* are assumed to be relative to some base path known to the application or
* consumer.
...
*/
public function withPath($path);

https://www.php-fig.org/psr/psr-7/

This means like this?

• if we call URI::withPath('foo/bar'), the path foo/bar is relative to some base path known to the application.
• the new URI instance getPath() will return foo/bar?

[kenjis]

<?phprequire__DIR__ . '/vendor/autoload.php';
useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
echo$uri . PHP_EOL; // http://localhost/ci4/foo/barecho$uri->getPath() . PHP_EOL; // /ci4/foo/bar

https://uri.thephpleague.com/uri/6.0/psr7/

[kenjis]

useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');

PHP Fatal error: Uncaught League\Uri\Exceptions\SyntaxError: If an authority is present the path must be empty or start with a /. in /.../league-uri/vendor/league/uri/src/Uri.php:921

[kenjis]

useSlim\Psr7\Factory\UriFactory;
$factory = newUriFactory();
$uri = $factory->createUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[kenjis]

useNyholm\Psr7\Uri;
$uri = newUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[iRedds]

• the new URI instance getPath() will return foo/bar?

empty string

[MGatner]

Looks great! I feel validated in the need for this given how much code is in SiteURI on top of URI. I appreciate your verbosity in naming variables and methods - it is easy to overlook pieces which has led to many bugs in the past. I think this will be a good step forward.

I assume the URL Helper methods get changed in the next PR? I haven't looked at that one yet...

[kenjis]

@TimexPeachtree@MGatner Thank you for approvals.

[kenjis]

Description
This PR is needed for #7123

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

• add SiteURI class
  • The $routePath never starts with /.
  • If the path ends with /, the URI retains the trailing /.
  • getPath() returns the full URI path.
  • getRoutePath() returns the path relative to baseURL.

Related #5930, #7239, #7249, #7251

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

[michalsn]

Maybe this is a silly question, but... as I understand, this new class will bring full PSR-7 compatibility, right?

I don't see any related changes in the framework so we won't rely on it. The question is, who needs this class and for what?

Like... I saw your description:

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

But I'm afraid I don't fully understand the use cases.

[kenjis]

This PR only adds a new SiteURI class, and does not change the current framework behavior yet.

In the next PR(s), I will do:

• add a factory to create the SiteURI instance for the current URL: feat: add SiteURIFactory #7256
• set the SiteURI instance in the uri property of the Request
• use the SiteURI instance when getting the current URL like current_url()

About compatibility with PSR-7, the URI class will be cleaner because site-related functions like baseURL, will be separated into the SiteURI class. And the URI class will be easier to change.

Finally, I think URI and SiteURL can be made fully compatible (and having some extended functions) with PSR-7.

[MGatner]

I am obviously in favor of this but I want to be sure we get reviews from @lonnieezell and @iRedds and anyone else who was opposed to the idea originally.

[lonnieezell]

I was purposefully not commenting since my position was already known and it seemed I was overruled :)

I'm confused why we need a second class that does 99% of the same things as the URI class. The URI class is just for working with URIs in general, crafted to meet the RFC as best we could at the time. It seems like all other issues should be fixed where it's used, or the base class should be changed to support our needs.

The implementation itself is solid enough. Though if we're going to include it, we need docs and guidance on when to use which one.

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers. What is the point of this other then to throw confusion about which class people should use when? I figured it was to fix issues in how the framework was returning answers, but it doesn't seem like we're actually doing that.

[michalsn]

I agree with @lonnieezell. I don't understand why we need a second class to handle URLs. Can somebody give me a clear example of where it will be useful?

AFAIK the only issue that we have now with the URI class is that it does not follow PSR-7 (it's something path related).

Also, if the goal for both classes is to follow the PSR-7, what will be the difference? Wouldn't it be better to see some additional methods added to the current class instead?

[kenjis]

I don't understand why we need a second class to handle URLs.

Because we have already two kinds of concept URI,
and if we have two classes, It is easier to understand for us.

1. URI: a URI
2. SiteURI: a URI for a CI4 app site.

SiteURI has its own properties like baseURL, indexPage and we cannot change the URI path
inside baseURL (subfolders). Only the URI path after indexPage does matter.

Now we are already confusing 1. and 2. So we have two kinds of URI segments for one SiteURL.
See #7123 (comment)

[kenjis]

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers.

This fix (and refactoring) will be long story. This PR is just the first one.
We will use the new class in all places after all. Please wait for incoming PRs.

[MGatner]

I won't repeat my arguments, the original PR is here with discussion: #4647

I would like to point out that after doing a deep dive on URI handling and bug fixes both @kenjis and I came to the same conclusion, independently. I think the problem is larger than credited and may indeed merit the potential confusion/clutter of an additional class.

[lonnieezell]

After giving this a bit more thought, I think I'm ok with this on 2 conditions:

1. It's designed for internal use within the request class and URL helper functions. Not intended for public use or really even documented. This avoids the confusion to end users, while still providing a "URI in Request is the source of truth" that @MGatner was looking for in the previous PR.
2. It's saved for 5.0 (which we should be working on now....) since it's too big of a potential BC break.

[iRedds]

$uri = $this->request->getUri();
echo (string) $uri; // "http://localhost:8888/ci431/public/test?a=b" → Okayecho$uri->getPath(); // "test" → NG. It should be "/ci431/public/test" when following PSR-7

And I think that such behavior corresponds to PSR.

// for http://localhost:8888/ci431/public/test?a=bURI::getPath(); // => /ci431/public/test// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)// Accordingly, the leading slash will determine whether the path is absolute or rootless// for http://localhost:8888/{base_path}/test?a=bURI::withPath('/xxx'); // http://localhost:8888/xxx?a=b// for http://localhost:8888/{base_path}/test?a=bURI::withPath('xxx'); // http://localhost:8888/{base_path}/xxx?a=b

php-fig/fig-standards#503

[kenjis]

What is {base_path} in this case? How do you define it?

// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)

[iRedds]

@kenjis app.baseURL = 'http://localhost:8888{/ci431/public/}' <- base path

[kenjis]

I don't get it.
How do you set it in PSR-7 URI object?

[iRedds]

There are no objects in PSR-7. Interfaces only. That is, mandatory public methods for PSR compliance.
A class implementing UriInterface should not be limited to UriInterface methods and may contain a method that sets base_path or base_url or whatever is needed for it to work properly.

[kenjis]

Many frameworks provide the ability to get the "base path," usually considered the path up to and including the front controller. As an example, if the application is served at http://example.com/b2b/index.php, and the current URI used to request it is http://example.com/b2b/index.php/customer/register, the functionality to retrieve the base path would return /b2b/index.php. This value can then be used by routers to strip that path segment prior to attempting a match.
https://www.php-fig.org/psr/psr-7/meta/#why-is-no-functionality-included-for-retrieving-the-base-path

 * If an HTTP path is intended to be host-relative rather than path-relative
* then it must begin with a slash ("/"). HTTP paths not starting with a slash
* are assumed to be relative to some base path known to the application or
* consumer.
...
*/
public function withPath($path);

https://www.php-fig.org/psr/psr-7/

This means like this?

• if we call URI::withPath('foo/bar'), the path foo/bar is relative to some base path known to the application.
• the new URI instance getPath() will return foo/bar?

[kenjis]

<?phprequire__DIR__ . '/vendor/autoload.php';
useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
echo$uri . PHP_EOL; // http://localhost/ci4/foo/barecho$uri->getPath() . PHP_EOL; // /ci4/foo/bar

https://uri.thephpleague.com/uri/6.0/psr7/

[kenjis]

useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');

PHP Fatal error: Uncaught League\Uri\Exceptions\SyntaxError: If an authority is present the path must be empty or start with a /. in /.../league-uri/vendor/league/uri/src/Uri.php:921

[kenjis]

useSlim\Psr7\Factory\UriFactory;
$factory = newUriFactory();
$uri = $factory->createUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[kenjis]

useNyholm\Psr7\Uri;
$uri = newUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[iRedds]

• the new URI instance getPath() will return foo/bar?

empty string

[MGatner]

Looks great! I feel validated in the need for this given how much code is in SiteURI on top of URI. I appreciate your verbosity in naming variables and methods - it is easy to overlook pieces which has led to many bugs in the past. I think this will be a good step forward.

I assume the URL Helper methods get changed in the next PR? I haven't looked at that one yet...

[kenjis]

@TimexPeachtree@MGatner Thank you for approvals.

[kenjis]

Description
This PR is needed for #7123

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

• add SiteURI class
  • The $routePath never starts with /.
  • If the path ends with /, the URI retains the trailing /.
  • getPath() returns the full URI path.
  • getRoutePath() returns the path relative to baseURL.

Related #5930, #7239, #7249, #7251

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

[michalsn]

Maybe this is a silly question, but... as I understand, this new class will bring full PSR-7 compatibility, right?

I don't see any related changes in the framework so we won't rely on it. The question is, who needs this class and for what?

Like... I saw your description:

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

But I'm afraid I don't fully understand the use cases.

[kenjis]

This PR only adds a new SiteURI class, and does not change the current framework behavior yet.

In the next PR(s), I will do:

• add a factory to create the SiteURI instance for the current URL: feat: add SiteURIFactory #7256
• set the SiteURI instance in the uri property of the Request
• use the SiteURI instance when getting the current URL like current_url()

About compatibility with PSR-7, the URI class will be cleaner because site-related functions like baseURL, will be separated into the SiteURI class. And the URI class will be easier to change.

Finally, I think URI and SiteURL can be made fully compatible (and having some extended functions) with PSR-7.

[MGatner]

I am obviously in favor of this but I want to be sure we get reviews from @lonnieezell and @iRedds and anyone else who was opposed to the idea originally.

[lonnieezell]

I was purposefully not commenting since my position was already known and it seemed I was overruled :)

I'm confused why we need a second class that does 99% of the same things as the URI class. The URI class is just for working with URIs in general, crafted to meet the RFC as best we could at the time. It seems like all other issues should be fixed where it's used, or the base class should be changed to support our needs.

The implementation itself is solid enough. Though if we're going to include it, we need docs and guidance on when to use which one.

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers. What is the point of this other then to throw confusion about which class people should use when? I figured it was to fix issues in how the framework was returning answers, but it doesn't seem like we're actually doing that.

[michalsn]

I agree with @lonnieezell. I don't understand why we need a second class to handle URLs. Can somebody give me a clear example of where it will be useful?

AFAIK the only issue that we have now with the URI class is that it does not follow PSR-7 (it's something path related).

Also, if the goal for both classes is to follow the PSR-7, what will be the difference? Wouldn't it be better to see some additional methods added to the current class instead?

[kenjis]

I don't understand why we need a second class to handle URLs.

Because we have already two kinds of concept URI,
and if we have two classes, It is easier to understand for us.

1. URI: a URI
2. SiteURI: a URI for a CI4 app site.

SiteURI has its own properties like baseURL, indexPage and we cannot change the URI path
inside baseURL (subfolders). Only the URI path after indexPage does matter.

Now we are already confusing 1. and 2. So we have two kinds of URI segments for one SiteURL.
See #7123 (comment)

[kenjis]

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers.

This fix (and refactoring) will be long story. This PR is just the first one.
We will use the new class in all places after all. Please wait for incoming PRs.

[MGatner]

I won't repeat my arguments, the original PR is here with discussion: #4647

I would like to point out that after doing a deep dive on URI handling and bug fixes both @kenjis and I came to the same conclusion, independently. I think the problem is larger than credited and may indeed merit the potential confusion/clutter of an additional class.

[lonnieezell]

After giving this a bit more thought, I think I'm ok with this on 2 conditions:

1. It's designed for internal use within the request class and URL helper functions. Not intended for public use or really even documented. This avoids the confusion to end users, while still providing a "URI in Request is the source of truth" that @MGatner was looking for in the previous PR.
2. It's saved for 5.0 (which we should be working on now....) since it's too big of a potential BC break.

[iRedds]

$uri = $this->request->getUri();
echo (string) $uri; // "http://localhost:8888/ci431/public/test?a=b" → Okayecho$uri->getPath(); // "test" → NG. It should be "/ci431/public/test" when following PSR-7

And I think that such behavior corresponds to PSR.

// for http://localhost:8888/ci431/public/test?a=bURI::getPath(); // => /ci431/public/test// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)// Accordingly, the leading slash will determine whether the path is absolute or rootless// for http://localhost:8888/{base_path}/test?a=bURI::withPath('/xxx'); // http://localhost:8888/xxx?a=b// for http://localhost:8888/{base_path}/test?a=bURI::withPath('xxx'); // http://localhost:8888/{base_path}/xxx?a=b

php-fig/fig-standards#503

[kenjis]

What is {base_path} in this case? How do you define it?

// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)

[iRedds]

@kenjis app.baseURL = 'http://localhost:8888{/ci431/public/}' <- base path

[kenjis]

I don't get it.
How do you set it in PSR-7 URI object?

[iRedds]

There are no objects in PSR-7. Interfaces only. That is, mandatory public methods for PSR compliance.
A class implementing UriInterface should not be limited to UriInterface methods and may contain a method that sets base_path or base_url or whatever is needed for it to work properly.

[kenjis]

Many frameworks provide the ability to get the "base path," usually considered the path up to and including the front controller. As an example, if the application is served at http://example.com/b2b/index.php, and the current URI used to request it is http://example.com/b2b/index.php/customer/register, the functionality to retrieve the base path would return /b2b/index.php. This value can then be used by routers to strip that path segment prior to attempting a match.
https://www.php-fig.org/psr/psr-7/meta/#why-is-no-functionality-included-for-retrieving-the-base-path

 * If an HTTP path is intended to be host-relative rather than path-relative
* then it must begin with a slash ("/"). HTTP paths not starting with a slash
* are assumed to be relative to some base path known to the application or
* consumer.
...
*/
public function withPath($path);

https://www.php-fig.org/psr/psr-7/

This means like this?

• if we call URI::withPath('foo/bar'), the path foo/bar is relative to some base path known to the application.
• the new URI instance getPath() will return foo/bar?

[kenjis]

<?phprequire__DIR__ . '/vendor/autoload.php';
useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
echo$uri . PHP_EOL; // http://localhost/ci4/foo/barecho$uri->getPath() . PHP_EOL; // /ci4/foo/bar

https://uri.thephpleague.com/uri/6.0/psr7/

[kenjis]

useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');

PHP Fatal error: Uncaught League\Uri\Exceptions\SyntaxError: If an authority is present the path must be empty or start with a /. in /.../league-uri/vendor/league/uri/src/Uri.php:921

[kenjis]

useSlim\Psr7\Factory\UriFactory;
$factory = newUriFactory();
$uri = $factory->createUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[kenjis]

useNyholm\Psr7\Uri;
$uri = newUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[iRedds]

• the new URI instance getPath() will return foo/bar?

empty string

[MGatner]

Looks great! I feel validated in the need for this given how much code is in SiteURI on top of URI. I appreciate your verbosity in naming variables and methods - it is easy to overlook pieces which has led to many bugs in the past. I think this will be a good step forward.

I assume the URL Helper methods get changed in the next PR? I haven't looked at that one yet...

[kenjis]

@TimexPeachtree@MGatner Thank you for approvals.

[kenjis]

Description
This PR is needed for #7123

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

• add SiteURI class
  • The $routePath never starts with /.
  • If the path ends with /, the URI retains the trailing /.
  • getPath() returns the full URI path.
  • getRoutePath() returns the path relative to baseURL.

Related #5930, #7239, #7249, #7251

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

[michalsn]

Maybe this is a silly question, but... as I understand, this new class will bring full PSR-7 compatibility, right?

I don't see any related changes in the framework so we won't rely on it. The question is, who needs this class and for what?

Like... I saw your description:

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

But I'm afraid I don't fully understand the use cases.

[kenjis]

This PR only adds a new SiteURI class, and does not change the current framework behavior yet.

In the next PR(s), I will do:

• add a factory to create the SiteURI instance for the current URL: feat: add SiteURIFactory #7256
• set the SiteURI instance in the uri property of the Request
• use the SiteURI instance when getting the current URL like current_url()

About compatibility with PSR-7, the URI class will be cleaner because site-related functions like baseURL, will be separated into the SiteURI class. And the URI class will be easier to change.

Finally, I think URI and SiteURL can be made fully compatible (and having some extended functions) with PSR-7.

[MGatner]

I am obviously in favor of this but I want to be sure we get reviews from @lonnieezell and @iRedds and anyone else who was opposed to the idea originally.

[lonnieezell]

I was purposefully not commenting since my position was already known and it seemed I was overruled :)

I'm confused why we need a second class that does 99% of the same things as the URI class. The URI class is just for working with URIs in general, crafted to meet the RFC as best we could at the time. It seems like all other issues should be fixed where it's used, or the base class should be changed to support our needs.

The implementation itself is solid enough. Though if we're going to include it, we need docs and guidance on when to use which one.

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers. What is the point of this other then to throw confusion about which class people should use when? I figured it was to fix issues in how the framework was returning answers, but it doesn't seem like we're actually doing that.

[michalsn]

I agree with @lonnieezell. I don't understand why we need a second class to handle URLs. Can somebody give me a clear example of where it will be useful?

AFAIK the only issue that we have now with the URI class is that it does not follow PSR-7 (it's something path related).

Also, if the goal for both classes is to follow the PSR-7, what will be the difference? Wouldn't it be better to see some additional methods added to the current class instead?

[kenjis]

I don't understand why we need a second class to handle URLs.

Because we have already two kinds of concept URI,
and if we have two classes, It is easier to understand for us.

1. URI: a URI
2. SiteURI: a URI for a CI4 app site.

SiteURI has its own properties like baseURL, indexPage and we cannot change the URI path
inside baseURL (subfolders). Only the URI path after indexPage does matter.

Now we are already confusing 1. and 2. So we have two kinds of URI segments for one SiteURL.
See #7123 (comment)

[kenjis]

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers.

This fix (and refactoring) will be long story. This PR is just the first one.
We will use the new class in all places after all. Please wait for incoming PRs.

[MGatner]

I won't repeat my arguments, the original PR is here with discussion: #4647

I would like to point out that after doing a deep dive on URI handling and bug fixes both @kenjis and I came to the same conclusion, independently. I think the problem is larger than credited and may indeed merit the potential confusion/clutter of an additional class.

[lonnieezell]

After giving this a bit more thought, I think I'm ok with this on 2 conditions:

1. It's designed for internal use within the request class and URL helper functions. Not intended for public use or really even documented. This avoids the confusion to end users, while still providing a "URI in Request is the source of truth" that @MGatner was looking for in the previous PR.
2. It's saved for 5.0 (which we should be working on now....) since it's too big of a potential BC break.

[iRedds]

$uri = $this->request->getUri();
echo (string) $uri; // "http://localhost:8888/ci431/public/test?a=b" → Okayecho$uri->getPath(); // "test" → NG. It should be "/ci431/public/test" when following PSR-7

And I think that such behavior corresponds to PSR.

// for http://localhost:8888/ci431/public/test?a=bURI::getPath(); // => /ci431/public/test// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)// Accordingly, the leading slash will determine whether the path is absolute or rootless// for http://localhost:8888/{base_path}/test?a=bURI::withPath('/xxx'); // http://localhost:8888/xxx?a=b// for http://localhost:8888/{base_path}/test?a=bURI::withPath('xxx'); // http://localhost:8888/{base_path}/xxx?a=b

php-fig/fig-standards#503

[kenjis]

What is {base_path} in this case? How do you define it?

// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)

[iRedds]

@kenjis app.baseURL = 'http://localhost:8888{/ci431/public/}' <- base path

[kenjis]

I don't get it.
How do you set it in PSR-7 URI object?

[iRedds]

There are no objects in PSR-7. Interfaces only. That is, mandatory public methods for PSR compliance.
A class implementing UriInterface should not be limited to UriInterface methods and may contain a method that sets base_path or base_url or whatever is needed for it to work properly.

[kenjis]

Many frameworks provide the ability to get the "base path," usually considered the path up to and including the front controller. As an example, if the application is served at http://example.com/b2b/index.php, and the current URI used to request it is http://example.com/b2b/index.php/customer/register, the functionality to retrieve the base path would return /b2b/index.php. This value can then be used by routers to strip that path segment prior to attempting a match.
https://www.php-fig.org/psr/psr-7/meta/#why-is-no-functionality-included-for-retrieving-the-base-path

 * If an HTTP path is intended to be host-relative rather than path-relative
* then it must begin with a slash ("/"). HTTP paths not starting with a slash
* are assumed to be relative to some base path known to the application or
* consumer.
...
*/
public function withPath($path);

https://www.php-fig.org/psr/psr-7/

This means like this?

• if we call URI::withPath('foo/bar'), the path foo/bar is relative to some base path known to the application.
• the new URI instance getPath() will return foo/bar?

[kenjis]

<?phprequire__DIR__ . '/vendor/autoload.php';
useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
echo$uri . PHP_EOL; // http://localhost/ci4/foo/barecho$uri->getPath() . PHP_EOL; // /ci4/foo/bar

https://uri.thephpleague.com/uri/6.0/psr7/

[kenjis]

useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');

PHP Fatal error: Uncaught League\Uri\Exceptions\SyntaxError: If an authority is present the path must be empty or start with a /. in /.../league-uri/vendor/league/uri/src/Uri.php:921

[kenjis]

useSlim\Psr7\Factory\UriFactory;
$factory = newUriFactory();
$uri = $factory->createUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[kenjis]

useNyholm\Psr7\Uri;
$uri = newUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[iRedds]

• the new URI instance getPath() will return foo/bar?

empty string

[MGatner]

Looks great! I feel validated in the need for this given how much code is in SiteURI on top of URI. I appreciate your verbosity in naming variables and methods - it is easy to overlook pieces which has led to many bugs in the past. I think this will be a good step forward.

I assume the URL Helper methods get changed in the next PR? I haven't looked at that one yet...

[kenjis]

@TimexPeachtree@MGatner Thank you for approvals.

[kenjis]

Description
This PR is needed for #7123

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

• add SiteURI class
  • The $routePath never starts with /.
  • If the path ends with /, the URI retains the trailing /.
  • getPath() returns the full URI path.
  • getRoutePath() returns the path relative to baseURL.

Related #5930, #7239, #7249, #7251

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

[michalsn]

Maybe this is a silly question, but... as I understand, this new class will bring full PSR-7 compatibility, right?

I don't see any related changes in the framework so we won't rely on it. The question is, who needs this class and for what?

Like... I saw your description:

We need two URI classes. One for general purpose URI class, and the other is for URI of the app site.

But I'm afraid I don't fully understand the use cases.

[kenjis]

This PR only adds a new SiteURI class, and does not change the current framework behavior yet.

In the next PR(s), I will do:

• add a factory to create the SiteURI instance for the current URL: feat: add SiteURIFactory #7256
• set the SiteURI instance in the uri property of the Request
• use the SiteURI instance when getting the current URL like current_url()

About compatibility with PSR-7, the URI class will be cleaner because site-related functions like baseURL, will be separated into the SiteURI class. And the URI class will be easier to change.

Finally, I think URI and SiteURL can be made fully compatible (and having some extended functions) with PSR-7.

[MGatner]

I am obviously in favor of this but I want to be sure we get reviews from @lonnieezell and @iRedds and anyone else who was opposed to the idea originally.

[lonnieezell]

I was purposefully not commenting since my position was already known and it seemed I was overruled :)

I'm confused why we need a second class that does 99% of the same things as the URI class. The URI class is just for working with URIs in general, crafted to meet the RFC as best we could at the time. It seems like all other issues should be fixed where it's used, or the base class should be changed to support our needs.

The implementation itself is solid enough. Though if we're going to include it, we need docs and guidance on when to use which one.

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers. What is the point of this other then to throw confusion about which class people should use when? I figured it was to fix issues in how the framework was returning answers, but it doesn't seem like we're actually doing that.

[michalsn]

I agree with @lonnieezell. I don't understand why we need a second class to handle URLs. Can somebody give me a clear example of where it will be useful?

AFAIK the only issue that we have now with the URI class is that it does not follow PSR-7 (it's something path related).

Also, if the goal for both classes is to follow the PSR-7, what will be the difference? Wouldn't it be better to see some additional methods added to the current class instead?

[kenjis]

I don't understand why we need a second class to handle URLs.

Because we have already two kinds of concept URI,
and if we have two classes, It is easier to understand for us.

1. URI: a URI
2. SiteURI: a URI for a CI4 app site.

SiteURI has its own properties like baseURL, indexPage and we cannot change the URI path
inside baseURL (subfolders). Only the URI path after indexPage does matter.

Now we are already confusing 1. and 2. So we have two kinds of URI segments for one SiteURL.
See #7123 (comment)

[kenjis]

It seems strange to me to make this new class and we're not using it ourselves in IncomingRequest or the URL helpers.

This fix (and refactoring) will be long story. This PR is just the first one.
We will use the new class in all places after all. Please wait for incoming PRs.

[MGatner]

I won't repeat my arguments, the original PR is here with discussion: #4647

I would like to point out that after doing a deep dive on URI handling and bug fixes both @kenjis and I came to the same conclusion, independently. I think the problem is larger than credited and may indeed merit the potential confusion/clutter of an additional class.

[lonnieezell]

After giving this a bit more thought, I think I'm ok with this on 2 conditions:

1. It's designed for internal use within the request class and URL helper functions. Not intended for public use or really even documented. This avoids the confusion to end users, while still providing a "URI in Request is the source of truth" that @MGatner was looking for in the previous PR.
2. It's saved for 5.0 (which we should be working on now....) since it's too big of a potential BC break.

[iRedds]

$uri = $this->request->getUri();
echo (string) $uri; // "http://localhost:8888/ci431/public/test?a=b" → Okayecho$uri->getPath(); // "test" → NG. It should be "/ci431/public/test" when following PSR-7

And I think that such behavior corresponds to PSR.

// for http://localhost:8888/ci431/public/test?a=bURI::getPath(); // => /ci431/public/test// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)// Accordingly, the leading slash will determine whether the path is absolute or rootless// for http://localhost:8888/{base_path}/test?a=bURI::withPath('/xxx'); // http://localhost:8888/xxx?a=b// for http://localhost:8888/{base_path}/test?a=bURI::withPath('xxx'); // http://localhost:8888/{base_path}/xxx?a=b

php-fig/fig-standards#503

[kenjis]

What is {base_path} in this case? How do you define it?

// for http://localhost:8888/{base_path}/test?a=bURI::getPath(); // => test (without leading slash, as an indicator that the base path is being used.)

[iRedds]

@kenjis app.baseURL = 'http://localhost:8888{/ci431/public/}' <- base path

[kenjis]

I don't get it.
How do you set it in PSR-7 URI object?

[iRedds]

There are no objects in PSR-7. Interfaces only. That is, mandatory public methods for PSR compliance.
A class implementing UriInterface should not be limited to UriInterface methods and may contain a method that sets base_path or base_url or whatever is needed for it to work properly.

[kenjis]

Many frameworks provide the ability to get the "base path," usually considered the path up to and including the front controller. As an example, if the application is served at http://example.com/b2b/index.php, and the current URI used to request it is http://example.com/b2b/index.php/customer/register, the functionality to retrieve the base path would return /b2b/index.php. This value can then be used by routers to strip that path segment prior to attempting a match.
https://www.php-fig.org/psr/psr-7/meta/#why-is-no-functionality-included-for-retrieving-the-base-path

 * If an HTTP path is intended to be host-relative rather than path-relative
* then it must begin with a slash ("/"). HTTP paths not starting with a slash
* are assumed to be relative to some base path known to the application or
* consumer.
...
*/
public function withPath($path);

https://www.php-fig.org/psr/psr-7/

This means like this?

• if we call URI::withPath('foo/bar'), the path foo/bar is relative to some base path known to the application.
• the new URI instance getPath() will return foo/bar?

[kenjis]

<?phprequire__DIR__ . '/vendor/autoload.php';
useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
echo$uri . PHP_EOL; // http://localhost/ci4/foo/barecho$uri->getPath() . PHP_EOL; // /ci4/foo/bar

https://uri.thephpleague.com/uri/6.0/psr7/

[kenjis]

useLeague\Uri\Http;
$uri = Http::createFromBaseUri('foo/bar', 'http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');

PHP Fatal error: Uncaught League\Uri\Exceptions\SyntaxError: If an authority is present the path must be empty or start with a /. in /.../league-uri/vendor/league/uri/src/Uri.php:921

[kenjis]

useSlim\Psr7\Factory\UriFactory;
$factory = newUriFactory();
$uri = $factory->createUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[kenjis]

useNyholm\Psr7\Uri;
$uri = newUri('http://localhost/ci4/');
$uri2 = $uri->withPath('controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // controller/method$uri2 = $uri->withPath('/controller/method');
echo$uri2 . PHP_EOL; // http://localhost/controller/methodecho$uri2->getPath() . PHP_EOL; // /controller/method

[iRedds]

• the new URI instance getPath() will return foo/bar?

empty string

[MGatner]

Looks great! I feel validated in the need for this given how much code is in SiteURI on top of URI. I appreciate your verbosity in naming variables and methods - it is easy to overlook pieces which has led to many bugs in the past. I think this will be a good step forward.

I assume the URL Helper methods get changed in the next PR? I haven't looked at that one yet...

[kenjis]

@TimexPeachtree@MGatner Thank you for approvals.