\lithium\action\Request::accepts($type) does not return boolean per documentation #856

Closed
al-the-x opened this Issue Mar 12, 2013 · 6 comments

Comments

Projects
None yet
3 participants
@al-the-x
Contributor

al-the-x commented Mar 12, 2013

From lithium\action\Request::accepts() docblock (and thus generated documentation):

/**
 * Returns information about the type of content that the client is requesting.
 *
 * @see lithium\net\http\Media::negotiate()
 * @param $type mixed If not specified, returns the media type name that the client prefers,
 *              using content negotiation. If a media type name (string) is passed, returns
 *              `true` or `false`, indicating whether or not that type is accepted by the client
 *              at all. If `true`, returns the raw content types from the `Accept` header,
 *              parsed into an array and sorted by client preference.
 * @return string Returns a simple type name if the type is registered (i.e. `'json'`), or
 *         a fully-qualified content-type if not (i.e. `'image/jpeg'`), or a boolean or array,
 *         depending on the value of `$type`.
 */

In practice, however, the accepts() method returns the negotiated content name, e.g. "html" or "json", not true / false if $type matches. Which should it be so that I can submit a PR to fix?

@al-the-x

This comment has been minimized.

Show comment Hide comment
@al-the-x

al-the-x Mar 12, 2013

Contributor

FWIW, the method proposed by the docs is the most useful to me, i.e.

if ( $this->request->accepts('json') ) return $this->render( // . . .

Current workaround isn't obtrusive, but it's nice if the docs match the implementation...

if ( $this->request->accepts() == 'json' ) return $this->render( // . . .
Contributor

al-the-x commented Mar 12, 2013

FWIW, the method proposed by the docs is the most useful to me, i.e.

if ( $this->request->accepts('json') ) return $this->render( // . . .

Current workaround isn't obtrusive, but it's nice if the docs match the implementation...

if ( $this->request->accepts() == 'json' ) return $this->render( // . . .
@al-the-x

This comment has been minimized.

Show comment Hide comment
@al-the-x

al-the-x Mar 12, 2013

Contributor

Scratch that workaround. Despite that the Accept header is set to application/json only, Request::accepts() (and the underlying Media::negotiate()) always returns text (for text/plain), although Request::accepts(true) will put application/json in the returned array... :[

Contributor

al-the-x commented Mar 12, 2013

Scratch that workaround. Despite that the Accept header is set to application/json only, Request::accepts() (and the underlying Media::negotiate()) always returns text (for text/plain), although Request::accepts(true) will put application/json in the returned array... :[

@ghost ghost assigned nateabele Jun 27, 2013

@davidpersson davidpersson added the docs label Aug 3, 2014

davidpersson added a commit to davidpersson/lithium that referenced this issue Mar 28, 2015

Update Request::accepts() to return boolean when type is provided.
Fixes #856.
Refs 97412cf.

- This makes the implemention match its documentation.
- Expand documentation and update documented return type.
- Add tests.
- Add changelog entry.

davidpersson added a commit to davidpersson/lithium that referenced this issue Mar 28, 2015

Update Request::accepts() to return boolean when type is provided.
Fixes #856.
Refs 97412cf.

- This makes the implemention match its documentation.
- Expand documentation and update documented return type.
- Add tests.
- Add changelog entry.

@davidpersson davidpersson added this to the 1.0 milestone Mar 28, 2015

@davidpersson

This comment has been minimized.

Show comment Hide comment
@davidpersson

davidpersson Mar 28, 2015

Member

The accepts() method works only with short media type names i.e. 'json'. Also note that it only knows about types that are registered via Media::type().

Member

davidpersson commented Mar 28, 2015

The accepts() method works only with short media type names i.e. 'json'. Also note that it only knows about types that are registered via Media::type().

@al-the-x

This comment has been minimized.

Show comment Hide comment
@al-the-x

al-the-x Mar 30, 2015

Contributor

Thanks for finally correcting this, @davidpersson. Sadly, I'm not using Lithium anywhere anymore. Too much wheel reinvention syndrome...

Contributor

al-the-x commented Mar 30, 2015

Thanks for finally correcting this, @davidpersson. Sadly, I'm not using Lithium anywhere anymore. Too much wheel reinvention syndrome...

@al-the-x al-the-x closed this Mar 30, 2015

@davidpersson

This comment has been minimized.

Show comment Hide comment
@davidpersson

davidpersson Mar 30, 2015

Member

Ha -- no worries. Still the bug is valid thus I'm reopening and will close once the patch has been merged.

Member

davidpersson commented Mar 30, 2015

Ha -- no worries. Still the bug is valid thus I'm reopening and will close once the patch has been merged.

@davidpersson davidpersson reopened this Mar 30, 2015

@nateabele

This comment has been minimized.

Show comment Hide comment
@nateabele

nateabele Mar 31, 2015

Member

Fixed in #1180.

Member

nateabele commented Mar 31, 2015

Fixed in #1180.

@nateabele nateabele closed this Mar 31, 2015

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