8000 [3.0] [HttpFoundation] clean up/redesign Request class · Issue #6406 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[3.0] [HttpFoundation] clean up/redesign Request class #6406

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
Tobion opened this issue Dec 18, 2012 · 15 comments
Closed

[3.0] [HttpFoundation] clean up/redesign Request class #6406

Tobion opened this issue Dec 18, 2012 · 15 comments
Labels
Feature HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed)

Comments

@Tobion
Copy link
Contributor
Tobion commented Dec 18, 2012

There are several inconsistent method names and I think the class can be better designed.

  • Synchronization of setters, getters, and parameter bags (server etc.) is unclear and inconsistent. For example there is a setMethod method which also modifies the ServerBag but there is no setPath. So either a Request should be immutable or mutable. But we have a mix of it. Furthermore, some getters a cached like getPathInfo and some are not like getHost. So when modifying the public parameter bags, sometimes it has influence on the Request getters and sometimes not. See also [HttpFoundation] Request::getPathInfo() inconsistencies #10478
  • getUriForPath is wrong. The correct name according to the current behavior would be getUriForPathInfo
  • the names for getBaseUrl and getBasePath are both unfortunate because the only difference is the inclusion of the script file name and has nothing to do with URL vs path.

The method name for Request::getPathInfo() is so bad because

  1. it returns the path of a URI and not a "path info" (there is no such thing in the URI spec)
  2. it suggest to return "more info" in form of an array like the php function pathinfo does, but it only returns the plain path
    So the name is really confusing as it does not behave similar to phps pathinfo() and is also semantically wrong.
    I suggest to deprecate getPathInfo() and introduce getPath().
    Working with it bothered me ever since. And I think we should fix that now before it's too late.

Changing some names like baseUrl (that also does not exist in CGI terms) will need to be reflected in the routing component as well.
@Tobion
Copy link
Contributor Author
Tobion commented Dec 18, 2012

@fabpot would you accept such an PR?

@igorw
Copy link
Contributor
igorw commented Dec 18, 2012
8000

FYI, PATH_INFO comes from CGI. But since HttpFoundation is translating CGI back to HTTP, path may indeed make more sense.

@Tobion
Copy link
Contributor Author
Tobion commented Dec 18, 2012

Yes, I'm aware of that and this is definitely the reason why it was named this way. And I agree with you that since we deal with HTTP, not CGI, we should not do the same mistake.

@fabpot
Copy link
Member
fabpot commented Dec 18, 2012

I agree with you but I'm -1 for changing this at this point. Probably something to be done for 3.0

@Tobion
Copy link
Contributor Author
Tobion commented Dec 18, 2012

Well, I changed by mind. The getPathInfo() does not return the full URI path but only the path after the script file.
So it is correct not to name it getPath.
But then there is an inconsistency elsewhere: getUriForPath is wrong. The correct name according to the current behavior would be getUriForPathInfo...
And the names for getBaseUrl and getBasePath are both unfortunate because the only difference is the inclusion of the script file name and has nothing to do with URL vs path.

@ghost
Copy link
8000
ghost commented Dec 18, 2012

@Tobion - how about adding a new method getUriForPathInfo and deprecating getUriForPath ?

@fabpot
Copy link
Member
fabpot commented Dec 18, 2012

the getUriForPath method is used qui frequently. So, again, let's do an overall in 3.0.

@fabpot fabpot added the RFP label Mar 5, 2014
@Tobion Tobion mentioned this issue Mar 18, 2014
Closed
@pyrech pyrech mentioned this issue Aug 4, 2014
Closed
@linaori
Copy link
Contributor
linaori commented Sep 1, 2014

I would love to see the public properties being hidden away in getters. This is already possible for 2.6 and easy to implement. The public properties could then be marked as deprecated and made private in 3.0.

@jakzal jakzal mentioned this issue Sep 16, 2014
Closed
@Tobion
Copy link
Contributor Author
Tobion commented Sep 16, 2014

getUri should return the full URI of the resource beeing requested. And I guess getRequestUri is/should based on http://www.w3.org/Protocols/rfc2616/rfc2616-sec5.html
So it should return the request URI that was used in the request line. That can be the full uri, or absulte path etc. According to the rfc it could also be *. Anyway, I don' think the request class really returns the request uri according to the rfc.

@Tobion
Copy link
Contributor Author
Tobion commented Oct 8, 2014

http://tools.ietf.org/html/rfc3875

 script-URI = <scheme> "://" <server-name> ":" <server-port>
                   <script-path> <extra-path> "?" <query-string>

 where <scheme> is found from SERVER_PROTOCOL, <server-name>,
   <server-port> and <query-string> are the values of the respective
   meta-variables.  The SCRIPT_NAME and PATH_INFO values, URL-encoded
   with ";", "=" and "?"  reserved, give <script-path> and <extra-path>.

So I think the the request class should have

function getPath() // returns the full path of a request, e.g. "/app.php/foo"
function getScriptPath() // returns the path of a request to the executing script (front controller), e.g. "/app.php"
function getExtraPath() // returns the path of a request without the script, e.g. "/foo"

The disadvantage is that people who don't want to ignore the script part (like routing) would need to call something like $realPath = $request->getExtraPath() ?: $request->getPath(). So maybe its better to invert the logic:

function getFullPath() // returns the full path of a request, e.g. "/app.php/foo"
function getScriptPath() // returns the path of a request to the executing script (front controller), e.g. "/app.php"
function getPath() // returns the path of a request without the script, e.g. "/foo"

@jakzal jakzal mentioned this issue Feb 7, 2015
Closed
@Tobion
Copy link
Contributor Author
Tobion commented Feb 9, 2015

PSR-7 looks good to me. So IMO Symfony 3.0 Request/Response should be based on that.

@Tobion Tobion mentioned this issue Feb 9, 2015
Closed
@andig
Copy link
andig commented Mar 21, 2015

So IMO Symfony 3.0 Request/Response should be based on that.

Is there any specific work going on in that direction? I've recently ported our app (https://github.com/volkszaehler/volkszaehler.org) to HTTPKernelInterface (and ReactPHP...) and would like to understand where to go with this once PSR-7 is alive.

@ghost
Copy link
ghost commented Mar 21, 2015

@andig : that might depend on if we can get a PSR for HttpKernelInterface too. once PSR-7 is voted on, then hopefully we can move towards that.

@ewgRa ewgRa mentioned this issue Jun 16, 2015
Closed
@ewgRa
Copy link
Contributor
ewgRa commented Jun 16, 2015

So either a Request should be immutable or mutable

I make some profiling and realize that isSecure, getHost and so on methods usually return same result, because headers and server bags that is involved in results of this methods usually are not changed. And for now there is a lot of unnecessary calculations. Immutable request can help to avoid them.

@andig
Copy link
andig commented Jun 17, 2015

There are several inconsistent method names and I think the class can be better designed.

I would like to add that the constructors (if not using the factory methods) make it easy to create inconsistent representations. If I e.g. construct with $request parameters but forget $content, the request object will itself represent something that doesn't exist in HTTP. I was wondering if such logical inconsistencies should be errored or warned (probably not) or correc 9DA2 ted by the Request implementation?

If off-topic please ignore.

@jakzal jakzal mentioned this issue Oct 13, 2015
Closed
@GuilhemN GuilhemN mentioned this issue Feb 2, 2016
Closed
@ro0NL ro0NL mentioned this issue Mar 31, 2016
Closed
@fabpot fabpot removed this from the 3.0 milestone Oct 18, 2016
@Tobion Tobion closed this as completed Oct 2, 2017
@wouterj wouterj mentioned this issue Apr 14, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature HttpFoundation RFC RFC = Request For Comments (proposals about features that you want to be discussed)
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
6 participants
@fabpot @igorw @andig @Tobion @ewgRa @linaori
0