8000 [HttpFoundation] Make some Request methods static by GuilhemN · Pull Request #17379 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[HttpFoundation] Make some Request methods static #17379

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
Closed
wants to merge 1 commit into from
Closed

[HttpFoundation] Make some Request methods static #17379

wants to merge 1 commit into from

Conversation

GuilhemN
Copy link
Contributor
Q A
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets #17318 (diff)
License MIT
Doc PR

This PR makes Request::getMimeType(), Request::getFormat() and Request::setFormat() static.

I'll check if this methods are in the docs and if yes, I'll create a pull request for the docs.

@GuilhemN
[HttpFoundation] Made some Request methods static
e0f839b 
Request::getMimeType(), Request::getMimeTypes(), Request::getFormat(), Request::setFormat() are now static
@xabbuh
Copy link
Member
xabbuh commented Jan 14, 2016

Isn't this a BC break (for people extending the Request class and overriding these methods)?

@GuilhemN
Copy link
Contributor Author

@xabbuh needs to be tested but when extending a class we can change it's signature, right?

@linaori
Copy link
Contributor
linaori commented Jan 15, 2016

Tested this on 5.6:

php > class A { public static function test() {} } class B extends A { public function test() {}};
PHP Fatal error:  Cannot make static method A::test() non static in class B in php shell code on line 1

Fatal error: Cannot make static method A::test() non static in class B in php shell code on line 1

@GuilhemN
Copy link
Contributor Author

Arf, do you see any solutions?

@xabbuh
Copy link
Member
xabbuh commented Jan 15, 2016

The only BC-compatible solution I see is to introduce new static method and deprecate the old ones which then could be removed in 4.0.

@linaori
Copy link
Contributor
linaori commented Jan 15, 2016

I'm in favor of refactoring the request in general... This class has far too much responsibility and mixed state/stateless.

@linaori
Copy link
Contributor
linaori commented Jan 15, 2016

Actually, I've been thinking of converting the current Request/Response objects to builders. Parameter converters etc are all working by setting values and the same goes for response listeners. By converting the ResponseBuilder and RequestBuilder into a "frozen" variant, it can also be compliant with PSR. This means that when modifying the request > builder but your action > PSR request. Same goes for the response, you can return a Response(Builder), which gets converted to a PSR Response if not already.

@GuilhemN
Copy link
Contributor Author

@iltar I like this idea but it's a lot of work so I won't begin a big refactoring of the HttpFoundation component in this PR but you should create an issue to see the reactions ;-)

@xabbuh What do you think about getMediaType? For getFormat I don't see any alternative... :/

@GuilhemN GuilhemN mentioned this pull request Jan 15, 2016
Merged
@GuilhemN
Copy link
Contributor Author

I'm wondering if this functions should really be a part of Request...
They can also be used to create the response for example and we should maybe move them to a dedicated class.

@GuilhemN
Copy link
Contributor Author

And as you said @iltar, using standard http interfaces from php-fig would be great and an easy way to refactor the Request class as we want.

@GuilhemN GuilhemN mentioned this pull request 8000 Jan 15, 2016
Closed
@GuilhemN
Copy link
Contributor Author

See #17392

@stof
Copy link
Member
stof commented Jan 15, 2016

@Ener-Getick we already discussed this in the past, and this would require to change the architecture of the whole framework due to the immutability of the Request (the Response impacts less places, but still breaks BC). This is precisely why we already rejected it in the past

@GuilhemN
Copy link
Contributor Author

@stof understood let's just focus on the first goal of this PR ;-)

@Tobion
Copy link
Contributor
Tobion commented Jan 15, 2016

Well as @iltar already showed, this is a BC break. So not viable as-is.
Status: Needs Work

@GuilhemN
Copy link
Contributor Author

Yeah I'm trying to find a viable solution :-/

@GuilhemN
Copy link
Contributor Author

I close this PR for now as I don't see any clean solution...

@GuilhemN GuilhemN closed this Jan 16, 2016
@GuilhemN GuilhemN deleted the REQUEST branch January 16, 2016 15:35
@GuilhemN GuilhemN mentioned this pull request Feb 1, 2016
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Status: Needs Work
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@GuilhemN @xabbuh @linaori @stof @Tobion @carsonbot
0