Skip to content
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 URIFactory #7239

Closed
wants to merge 17 commits into from
Closed

feat: add URIFactory #7239

wants to merge 17 commits into from

Conversation

kenjis
Copy link
Member

@kenjis kenjis commented Feb 10, 2023

Needs #7232

Description
Add URIFactory to create the current URI object.

To create the current URI object before the Request object. 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.

  • add URIFactory::createFromGlobals()
  • add URI::setRoutePath() and URI::getRoutePath()

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

@kenjis kenjis added enhancement PRs that improve existing functionalities 4.4 labels Feb 10, 2023
@kenjis kenjis force-pushed the feat-URIFactory branch 2 times, most recently from 816dbcc to 516a76a Compare February 10, 2023 06:36
Comment on lines +34 to +37
public function __construct(array &$server, array &$get, App $appConfig)
{
$this->server = &$server;
$this->get = &$get;
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think passing by reference, which means modifying superglobal arrays, is the right approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

What is the right approach?

@iRedds
Copy link
Collaborator

iRedds commented Feb 10, 2023

In my opinion, two factories are needed
\CodeIgniter\HTTP\URI::createFromRequest(Request $request) and \CodeIgniter\HTTP\URI::createFromString(string $url)

To create a separate class that works directly with _SERVER and _GET, and even created before the request object, this is not correct in my opinion.

@kenjis
Copy link
Member Author

kenjis commented Feb 10, 2023

How do you create Request object without URI object?

@iRedds
Copy link
Collaborator

iRedds commented Feb 10, 2023

How do you create Request object without URI object?

Say hello to the framework architects for the URI class in the Request constructor.
For the same reason, in other classes, _SERVER is also directly accessed.

@kenjis
Copy link
Member Author

kenjis commented Feb 10, 2023

Say hello to the framework architects for the URI class in the Request constructor.
For the same reason, in other classes, _SERVER is also directly accessed.

Sorry, I don't get what you say at all.
Who are the framework architects?
What do you mean by say hello?

@iRedds
Copy link
Collaborator

iRedds commented Feb 10, 2023

The URI class depends on the Request class for incoming requests because the Request class must provide access to a copy of _SERVER.

Therefore, making the Request class dependent on the URI is a framework architecture error.

I don't know who designed these classes.
I meant that the question "How do you create Request object without URI object?" should be forwarded to those who designed.

@kenjis kenjis marked this pull request as draft February 10, 2023 10:22
@kenjis
Copy link
Member Author

kenjis commented Feb 10, 2023

The URI class depends on the Request class for incoming requests because the Request class must provide access to a copy of _SERVER.

Indeed.

I meant that the question "How do you create Request object without URI object?" should be forwarded to those who designed.

Who designed does not matter.
The only question is how do we redesign.

@iRedds
Copy link
Collaborator

iRedds commented Feb 10, 2023

Who designed does not matter. The only question is how do we redesign.

I don't know.
There are two abstract options.

  1. Qualitatively, but this is BC at the level of major versions.
public function __constructor (protected array $query = [], protected array $request = [], protected array $server = []) {}
// and factory
public static function createFromGlobals(): Request 
{
    $request = new static($_GET, $_REQUEST, $_SERVER);
    return $request->withUri(Uri::createFromRequest($request));
}
  1. Remove the restriction on $uri = null in the IncomingRequest constructor. And when null, use a request-based factory.
    if (empty($uri) || empty($userAgent)) {
    throw new InvalidArgumentException('You must supply the parameters: uri, userAgent.');
    }
if (empty($userAgent)) {
    throw new InvalidArgumentException('You must supply the parameters: uri, userAgent.');
}

$this->uri = $uri ?? URI::createFromRequest($this);

Or something similar.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4.4 enhancement PRs that improve existing functionalities
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants