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.

Sign up for GitHub

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

Jump to bottom

Psr\Http\Message\ServerRequestInterface applied for yii\web\Request #15416

Merged
merged 6 commits into from
Dec 30, 2017
Merged

Psr\Http\Message\ServerRequestInterface applied for yii\web\Request #15416

merged 6 commits into from
Dec 30, 2017

Conversation

klimov-paul
Copy link
Member

Q A
Is bugfix? no
New feature? yes
Breaks BC? yes
Tests pass? yes
Fixed issues #11328, support for #15294

Fixing yii\web\Request to implement Psr\Http\Message\ServerRequestInterface instead of just Psr\Http\Message\RequestInterface

@klimov-paul klimov-paul added severity:BC breaking Either breaks backwards compatibility or fixed previously made breakage type:enhancement labels Dec 26, 2017
@klimov-paul klimov-paul added this to the 2.1.0 milestone Dec 26, 2017
samdark
samdark reviewed Dec 26, 2017
View reviewed changes
framework/web/Request.php
*/
public function setBodyParams($values)
public function setParsedBody($values)
Copy link
Member

Choose a reason for hiding this comment

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

Do we still need to have setParsedBody() as public?

Copy link
Member Author

Choose a reason for hiding this comment

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

I would rather keep it, since it was there before.
Besides having such public method can be useful while writing unit tests.

Copy link
Member

Choose a reason for hiding this comment

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

since it was there before.

That's not a good reason for a BC breaking release. As far as I understand PSR, whole idea is to make Request immutable i.e. no setters.

framework/web/Request.php
* @param array $serverParams server parameters.
* @since 2.1.0
*/
public function setServerParams(array $serverParams)
Copy link
Member

Choose a reason for hiding this comment

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

There are with* methods for everything but serverParams. I think it makes sense to make it uniform having only with* public methods.

Copy link
Member Author

@klimov-paul klimov-paul Dec 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

All with* methods are dictated by PSR. Since PSR does not provide withServerParams() method, I see no reason for implementing it. If you find this inconsistent you should report it to PHPFig.

Usage of with* methods is uneffective, since it causes object cloning. While configuring single Request instance via with* methods you will have it cloned 5-10 times.
Yii behaviors and filters would be unable to function via immutable setters as they may need to adjust passing Request or Response without being able to re-set thier origin.

Also keep in mind that while being cloned every instance of yii\base\Component, which Request is, looses any event handlers or behaviors (filters) attached to it.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also keep in mind that removing setters in favor of with* methods will make impossible to configure Request instance via DI, even with the recent features from yiisoft/di. For example:

[
    '__class' => 'yii\web\Request',
    'withCookieParams' => [....], // have no effect: calls `$request->withCookieParams()`, but looses returned new instance
]

Copy link
Member

Choose a reason for hiding this comment

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

The whole idea of the PSR is that Request object should be created once (passing everything needed in constructor) and then not being modified again except cloning via with.

Copy link
Member Author

Choose a reason for hiding this comment

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

If you sure in which way it should be - feel free to make modifications.

Copy link
Member

Choose a reason for hiding this comment

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

Do we have any request behaviors currently?

Copy link
Member

Choose a reason for hiding this comment

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

Losing event handlers could be a problem. Need to check code again before I suggest anything...

Copy link
Member

Choose a reason for hiding this comment

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

Currently Request doesn't trigger anything and there are no known behaviors for it.

framework/web/Request.php
* @param array $cookies array of key/value pairs representing cookies.
* @since 2.1.0
*/
public function setCookieParams(array $cookies)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need it public?

Copy link
Member Author

Choose a reason for hiding this comment

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

See #15416 (comment)

framework/web/Request.php
@@ -1753,6 +1871,17 @@ public function setUploadedFiles($uploadedFiles)
$this->_uploadedFiles = $uploadedFiles;
Copy link
Member

Choose a reason for hiding this comment

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

Do we need it public?

Copy link
Member Author

Choose a reason for hiding this comment

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

See #15416 (comment)

framework/web/Request.php
/**
* @param array $attributes attributes derived from the request.
*/
public function setAttributes(array $attributes)
Copy link
Member

Choose a reason for hiding this comment

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

Do we need it public?

Copy link
Member Author

Choose a reason for hiding this comment

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

See #15416 (comment)

framework/web/Request.php
* @return array attributes derived from the request.
* @since 2.1.0
*/
protected function defaultAttributes()
Copy link
Member

Choose a reason for hiding this comment

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

What's the usecase?

Copy link
Member Author

Choose a reason for hiding this comment

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

According to PSR request attributes are determined by particular Request class logic.
Having separated method allows developer to define internal attributes while extending yii\web\Request without necessity of re-defining private field for thier actual storage.

@samdark
Copy link
Member

samdark commented Dec 30, 2017

Currently Request serves many responsibilities:

  1. Filling itself with $_GET, $_POST, $_SERVER etc. considering trusted hosts and trusted headers.
  2. Storing data obtained and giving it to end user via convenient methods.
  3. Resolving request into route via resolve() method.
  4. Generating and setting cookies/tokens for CSRF. Verifying CSRF tokens.

The idea of PSR-7 is immutability and it makes sense in request/response paradigm. After request is received and PSR-7 request object is created, it's set in stone. Attempt to modify it usually means a possible bug.

It makes sens to have multiple classes:

  1. PSR-7 Request which is data container that serves purpose 2: storing data and having convenient methods for its retrieval. It could be initialized via constructor. There are multiple advantages in having it separate. It's more lightweight. Middlewares don't need to re-parse request same as everything else. Also it will be very convenient in tests.
  2. Request builder that has methods/settings to prepare data for PSR-7 Request.
  3. Resolving request data into route isn't request responsibility so should be moved into router (application).
  4. CSRF protection should be converted into middleware.

@klimov-paul klimov-paul merged commit 874fcaa into yiisoft:2.1 Dec 30, 2017
@klimov-paul
Copy link
Member Author

PR merged as an intermediate solution.
Further PSR-7 immutability disscussion should be handled at #15437

@samdark
Copy link
Member

samdark commented Dec 30, 2017

OK. Thanks.

@deltacube deltacube mentioned this pull request Jan 10, 2018
Merged
@klimov-paul klimov-paul deleted the psr-7-server-request branch January 10, 2018 11:48
@samdark samdark mentioned this pull request Feb 6, 2018
Closed
@vstokarev vstokarev mentioned this pull request Aug 25, 2018
Closed
@klimov-paul klimov-paul mentioned this pull request Nov 2, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@samdark samdark samdark left review comments

@SilverFire SilverFire SilverFire approved these changes

Assignees
No one assigned
Labels
severity:BC breaking Either breaks backwards compatibility or fixed previously made breakage type:enhancement
Projects
None yet
Milestone
3.0.0 (see yiisoft/yii-core instead)
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@klimov-paul @samdark @SilverFire