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

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

Closed
Tobion opened this Issue Dec 18, 2012 · 15 comments

Comments

Projects
None yet
7 participants
@Tobion
Copy link
Member

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 #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

This comment has been minimized.

Copy link
Member

Tobion commented Dec 18, 2012

@fabpot would you accept such an PR?

@igorw

This comment has been minimized.

Copy link
Contributor

igorw commented Dec 18, 2012

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

@Tobion

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

ghost commented Dec 18, 2012

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

@fabpot

This comment has been minimized.

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.

@linaori

This comment has been minimized.

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.

@Tobion

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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"
@Tobion

This comment has been minimized.

Copy link
Member

Tobion commented Feb 9, 2015

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

@andig

This comment has been minimized.

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.

@jrobeson

This comment has been minimized.

Copy link
Contributor

jrobeson 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

This comment has been minimized.

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

This comment has been minimized.

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 corrected by the Request implementation?

If off-topic please ignore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment