-
Notifications
You must be signed in to change notification settings - Fork 2k
feat: add SiteURIFactory #7256
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: add SiteURIFactory #7256
Conversation
77d43a8 to
e1d9a9c
Compare
157d9cb to
d68e8ac
Compare
b629857 to
b5308b0
Compare
b5308b0 to
3079cd9
Compare
3079cd9 to
63c2b31
Compare
63c2b31 to
a749f0e
Compare
a749f0e to
1c1a6ab
Compare
system/HTTP/SiteURIFactory.php
Outdated
| private function updateServer(string $key, string $value): void | ||
| { | ||
| $_SERVER[$key] = $value; | ||
| } | ||
|
|
||
| private function updateGetArray(array $array): void | ||
| { | ||
| $_GET = $array; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These make me pretty uneasy. What about an external service dependency that handles Globals, so it can be mocked or replaced? This behavior within a Factory is very counterintuitive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean?
Adding another class that handles globals?
private $superglobals;
private function updateServer(string $key, string $value): void
{
$this->superglobals->setServer($key, $value);
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, precisely. It could be adapted to anywhere else in the framework we touch globals.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added class Superglobals.
239b2cc to
61beca5
Compare
system/HTTP/SiteURIFactory.php
Outdated
|
|
||
| // Strip the SCRIPT_NAME path from the URI | ||
| if ( | ||
| $path !== '' && isset($this->superglobals->server('SCRIPT_NAME')[0]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isset($this->superglobals->server('SCRIPT_NAME')[0])
What is this condition checked?
$_SERVER['SCRIPT_NAME'] is a string.
The condition comes from:
CodeIgniter4/system/HTTP/IncomingRequest.php
Lines 282 to 286 in 6cedbe9
| // Strip the SCRIPT_NAME path from the URI | |
| if ( | |
| $uri !== '' && isset($_SERVER['SCRIPT_NAME'][0]) | |
| && pathinfo($_SERVER['SCRIPT_NAME'], PATHINFO_EXTENSION) === 'php' | |
| ) { |
And it came from:
bcit-ci/CodeIgniter@ead327f7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does that end up being translated as this, because of string array access?
$_SERVER['SCRIPT_NAME'] !== ''There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I assume this is unintended but that's the equivalence.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored. fde3216
Because it might be false.
It is difficult to understand.
61beca5 to
6b3dd7a
Compare
Needs #7252Description
Supersedes #7239
Add
SiteURIFactoryto create the current URI object.To create the current URI object before the Request object creation.
Also, to remove all URI adjustment processing currently performed in the constructor of the Request object.
This PR does not change any existing features yet. The next #7282 changes the behavior.
SiteURIFactory::createFromGlobals()SuperglobalsChecklist: