Enh #2131: Added Accept header parsing to CHttpRequest #2132

Merged
merged 17 commits into from Mar 27, 2013

5 participants

Contributor

Added functions getPreferredAcceptType() and getPreferredAcceptTypes() to parse the Accept header and return a list of accepted content types in order of preference, as per http://tools.ietf.org/html/rfc2616#section-14.1
NB, the specification does allow for escaping of parameters in an accept header, but this obscure case is not catered for here, since this would add an order of complexity to the regular expressions used.

issue #2131

Rupert-RR Enh #2131: Added Accept header parsing to CHttpRequest to give an array
of accepted types in order of preference
6a864f9
@Ragazzo Ragazzo commented on an outdated diff Mar 14, 2013
@@ -3,6 +3,7 @@
Version 1.1.14 work in progress
-------------------------------
+- Enh #2131: Added Accept header parsing to CHttpRequest to give an array of accepted types in order of preference (Rupert-RR)
Ragazzo
Ragazzo Mar 14, 2013

can u please put this note in changelog in the Enh section under the number order 2131?

Rupert-RR Merge remote-tracking branch 'upstream/master' into
2131-add-accept-header-parsing

Conflicts:
	CHANGELOG
a5b8240
Contributor

Updated from master, and resolved changelog conflict.

@Ragazzo Ragazzo commented on an outdated diff Mar 14, 2013
@@ -28,6 +27,7 @@ Version 1.1.14 work in progress
- Bug #2123: Fixed error in plural rules handling if locale has no plural rules defined (cebe, stepanselyuk)
- Bug #2146: CEmailValidator fix when fsockopen() can output uncatched error 'Connection refused (61)' (armab)
- Bug #2159: Fixed SQL syntax for delete command with join in MySQL (serebrov)
+- Enh #2131: Added Accept header parsing to CHttpRequest to give an array of accepted types in order of preference (Rupert-RR)
Ragazzo
Ragazzo Mar 14, 2013

good, but wrong order (oreder in Enh section), it must be between 2038 and 2205 as u see.

Contributor

Hopefully that should do it - I hadn't seen that we were doing everything in reverse numerical order - for some reason I always remembered the most recent being at the top and just didn't pay attention ;)

Owner
cebe commented Mar 14, 2013

Btw: no need to care about CHANGELOG conflicts. They will occour every day. We can handle them when we merge.

Owner

I'm wondering how many people would use this feature in practice. While preferred language seems similar, it seems to me more frequently used.

If a feature is rarely used in practice, we prefer it not included in the core.

Contributor

Fair enough - I use this every day to determine whether to send Html or xHtml. It is also useful for determining whether to send Html or JSON to Ajax requests.
It shouldn't bloat the core to much I don't think, since if people don't use it then it won't be called.
That being said, I can appreciate that there may not be a vast number of people who would use it.
Obviously up to you guys....

Owner
cebe commented Mar 15, 2013

I am also using it in many applications. When you build restfull applications you can decide the data format based on accept header. Would like to have such a function in Yii.

Ragazzo commented Mar 15, 2013

can i write my opinion in that way?) As for me i also used to build restfull api but i send this as a header with specific tokens, and not determine it from accept-header, i mean if it will be so, maybe make it somehow more specific, because when array as an example is returned it always will be that user have to do some extra-parsing, i wish it could be as usefull as getPrefferedLanguage(), just return html, ``xml,json`without subtypes, no?

Contributor

Sending a specific header is one way of doing it, but the Accept header is in the specification specifically for content negotiation - to determine which types of content the client can accept and which it prefers, so that the server may determine from that which type of content it wishes to serve (or it can return a 406 Not Acceptable header to indicate that it doesn't have anything in a type the client wants).

The PR I made does add two methods, one returning the array of accepted content types sorted by preference, and the other returning just the preferred content type (although this is simply equivalent to getting the array of content types and taking the first item), like getPreferredLanguage().
However, it is impossible to return just html, xml or json without making a lot of assumptions about how the client will format the Accept header.

For example an Accept header text/*; q=1, application/json; q=0.9 means that the client will accept any form of text above json (eg text/html, even though html is not mentioned). And something I use, which I have read is an accepted way of API versioning, is a header like application/vnd.myapp.v2+json which is a way of saying that I accept content in json as long as it corresponds to myapp v2 specifications.

Also, it should be noted that the Accept header is for content negotiation, not dictation - the client tells us what type of content it would prefer to receive, but should give a range of what we can send it. The server can then match that with its own preferences of the type of content it wants to send before choosing what type to send (or sending a 406 header if there are no matches).
An example here is the way I determine whether to send xhtml (as application/xhtml+xml) or html - there are some browsers who prefer html to xhtml (either by higher preference quality or by placing text/html before application/xhtml+xml), but as long as they can accept xhtml (regardless of whether they prefer it to html) I want to send xhtml, only sending html if they don't actually accept xhtml. To do this, I maintain a list of types that I can serve, and assign them each a preference rating. I then select the content types that are accepted by the client, and multiply the client's preference rating by my preference rating to give a rating which decides which content type I will send.

So, for the Accept header to be properly parsed, you will always have to use the array of preferences and match against your own preferences. If you want to be able to get a simple html, xml or json then it would be very easy to add a simple behavior to your CHttpRequest (in your config) which provides a method such as getSimplePreferredContentType() which would parse the array from getPreferredAcceptTypes() in whichever way you wanted (probably assuming that your client will always use a particular format in its Accept header) and return your type, but I do not think that this should be generalised.

Ragazzo commented Mar 15, 2013

Well, i see you have good understanding what rest api might looks like, but anyway this ench. must be easy to use for developers, i mean i am not agains that CHttpRequest must have a array of accepted types, just only return it in "easy way". Anyway lets see what core-developers will say.
P.S. btw, about api, good resource - there in small book collected some good practice (cookbook as i think) - http://blog.apigee.com/detail/is_your_api_naked_10_api_roadmap_considerations_part_1_visibility.

Contributor

Easy to use is always good, but I don't think that should mean making assumptions about the way something will be used which restricts its usefulness.
If all you want is an html, xml or json returned as the type you should be using, as long as you are certain that your client applications will always use text/html, application/xml or application/json (or even text/json) then you are safe just using the function getPreferredAcceptType() and the resulting array will have $result['subType'] filled with that type - fairly easy!

I have a function to give me the preferred content type from those from the Accept header and those I have defined my application as serving:

public static function preferredMediaType($availableTypes, $headerTypes) {
    $preferred = array();
    foreach ($headerTypes as $type) {
        foreach ($availableTypes as $availableType => $params) {
            $availableDec = explode('/', $availableType);
            if (($availableDec[0] == $type['type'] || $type['type'] == '*') && ($availableDec[1] == $type['subType'] || $type['subType'] == '*') && !isset($preferred[$availableType])) {
                $quality = isset($params['quality']) ? $params['quality'] : 1;
                $preferred[$availableType] = $quality * $type['quality'];
            }
            $availableDec = null;
        }
    }
    if (count($preferred) > 0) {
        arsort($preferred);
        reset($preferred);
        return key($preferred);
    }
    return null;
}

which I include in my base controller. However, this logic does not belong in CHttpRequest, which should only contain information about the request, and should not make any decisions as to what should be returned. To my mind this is more appropriate to the controller (or perhaps module), since the types that can be served are likely to depend on the controller (which chooses the view and does the rendering).

In a similar vein, I added a filter to the base controller, which would use the above function to determine the preferred content type (so that the action knows which view to render), but also could throw a 406 CHttpException if no accepted types were available, in a similar way to the HTTP verb filters.

Ragazzo commented Mar 15, 2013

Hm, maybe, need more other developers opinions i think. One more thing, maybe u can make some code refactoring because in current version it is hard to read and understand what is going on the function there?

Contributor

I'm not quite sure how you would want to refactor it - there are comments throughout explaining each step - the only thing I can see to make it easier would be a comment explaining how Accept headers work, but there is already a link to the specs for that.

@Ragazzo Ragazzo commented on an outdated diff Mar 15, 2013
framework/web/CHttpRequest.php
+ {
+ // the entire term contains only the mime type
+ $mediaRange=trim($term);
+ }
+ $types=explode('/',$mediaRange);
+ if(isset($types[0]))
+ $accept['type']=$types[0];
+ if(isset($types[1]))
+ $accept['subType']=$types[1];
+ // if the quality has not been explicitly stated, it is equal to 1
+ if(!isset($accept['quality']))
+ $accept['quality']=(double)1;
+ $accepts[]=$accept;
+ }
+ // slightly complicated sorting function - in case of equal quality, higher specificity takes preference, which includes a higher number of parameters if all else is equal - see link
+ usort($accepts,create_function('$a,$b','if($a[\'quality\']==$b[\'quality\']) {if(!($a[\'type\']==\'*\' xor $b[\'type\']==\'*\')) {if (!($a[\'subType\']==\'*\' xor $b[\'subType\']==\'*\')) {if(count($a[\'params\'])==count($b[\'params\'])) {return 0;} return count($a[\'params\'])<count($b[\'params\']) ? 1 : -1;} return $a[\'subType\']==\'*\' ? 1 : -1;} return $a[\'type\']==\'*\' ? 1 : -1;} return ($a[\'quality\']<$b[\'quality\']) ? 1 : -1;'));
Ragazzo
Ragazzo Mar 15, 2013

Well, i think this one can be in separated method with some good describing name. It is hard to understand what is calculating/sorting here.

@Ragazzo Ragazzo commented on an outdated diff Mar 15, 2013
framework/web/CHttpRequest.php
+ * 'subType' => 'xhtml+xml',
+ * 'quality' => 0.9,
+ * 'params' => array(
+ * 'level' => '1',
+ * ),
+ * )
+ * NB - to avoid great complexity, there are no steps taken to ensure that quoted strings are treated properly - if
+ * the header text includes quoted strings containing the , or ; characters then the results may not be * correct!
+ * @return array the user accepted MIME types in the order of preference.
+ * See {@link http://tools.ietf.org/html/rfc2616#section-14.1}
+ */
+ public function getPreferredAcceptTypes()
+ {
+ if($this->_preferredAcceptTypes===null)
+ {
+ $accepts=array();
Ragazzo
Ragazzo Mar 15, 2013

from here and till 882 line i think it must be also in separated method named smth. like parseAcceptTypes with good php-docs explaining.

Contributor

I have nothing against separating out those concerns into different functions.
However, there is no point in my doing that if the PR is not going to be accepted - let's wait and see what other people think first, and then see what the decision is.

Ragazzo commented Mar 15, 2013

yes of course, so anyway i vote for this to be implemented in CHttpRequest, good ench. anyway.

Owner

Alright. I think we can take it.

It would be great if the code can be optimized a bit. Right now it uses three regexp for each term. Would it be possible to use a single regexp for all terms?

Contributor

Not sure that my regexps are up to that!
I'll have a look at it maybe tomorrow to see if I can do it, and update accordingly.

@cebe cebe was assigned Mar 15, 2013
Rupert-RR added some commits Mar 18, 2013
Rupert-RR Separated out parse and compare functions.
Reduced regular expression count to 2 from 3.
Modified MIME type array map structure.
e3897e8
Rupert-RR Merge remote-tracking branch 'upstream/master' into
2131-add-accept-header-parsing

Conflicts:
	CHANGELOG
67b285d
Contributor

Ok, I have separated out the parse and compare parts into static functions.
I have reduced the regexp count to 2, but cannot see any way to get it down to 1.
It works fine according to my tests.

@samdark samdark commented on an outdated diff Mar 18, 2013
@@ -24,7 +24,6 @@ Version 1.1.14 work in progress
- Bug #2049: CStatElement relation with join option throw exception when key-field present on joined table (Yiivgeny)
- Bug #2078: Fixed problem with "undefined" parameter in query string when using CListView or CGridView with enableHistory (Parpaing)
- Bug #2086: Fixed .hgignore rule for assets folder (GeXu3, Koduc)
-- Bug #2087: CLocale: getLocaleDisplayName() was only returning the language display name, not the full locale display name (brandonkelly)
samdark
samdark Mar 18, 2013 Owner

Why this one is removed?

@samdark samdark commented on an outdated diff Mar 18, 2013
framework/web/CHttpRequest.php
@@ -801,6 +804,144 @@ public function redirect($url,$terminate=true,$statusCode=302)
}
/**
+ * Parses an Http Accept header, returning an array map with all parts of each entry
samdark
samdark Mar 18, 2013 Owner

Need dot at the end since it consists of multiple sentences.

samdark
samdark Mar 21, 2013 Owner

HTTP, all uppercase.

samdark
samdark Mar 21, 2013 Owner

Again, what is array map?

@samdark samdark commented on an outdated diff Mar 18, 2013
framework/web/CHttpRequest.php
@@ -801,6 +804,144 @@ public function redirect($url,$terminate=true,$statusCode=302)
}
/**
+ * Parses an Http Accept header, returning an array map with all parts of each entry
+ * Each array entry consists of a map with the type, subType, quality (ie preference ranking) and params, an array map of key-value parameters.
+ * For example, an Accept value of 'application/xhtml+xml;q=0.9;level=1' would give an array entry of
+ * array(
+ * 'type' => 'application',
+ * 'subType' => 'xhtml',
+ * 'baseType' => 'xml',
+ * 'params' => array(
+ * 'q' => 0.9,
+ * 'level' => '1',
+ * ),
+ * )
+ * NB - to avoid great complexity, there are no steps taken to ensure that quoted strings are treated properly - if
+ * the header text includes quoted strings containing the , or ; characters then the results may not be * correct!
samdark
samdark Mar 18, 2013 Owner

* correct?

Contributor

Do not merge yet - a kindly soul on Stack Overflow has given me a regexp that allows me to do all the parsing in one (followed by some array manipulation). I will upload a new version.

Rupert-RR added some commits Mar 21, 2013
Rupert-RR Altered parseAcceptHeader() to use only one regexp.
Thanks to Ka on StackExchange for this expression.
c255592
Rupert-RR Merge branch 'master' of git://github.com/yiisoft/yii into 2131-add-a…
…ccept-header-parsing

Upstream merge.
8193901
Contributor

Ready for merging (hopefully)!

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
@@ -50,6 +50,8 @@
* @property integer $port Port number for insecure requests.
* @property integer $securePort Port number for secure requests.
* @property CCookieCollection|CHttpCookie[] $cookies The cookie collection.
+ * @property array $preferredAcceptType The user preferred accept type as an array map, containing type, sub-type, quality and parameters.
samdark
samdark Mar 21, 2013 Owner

Doesn't give any idea of what array map is.

samdark
samdark Mar 21, 2013 Owner

Note that PhpDoc is used in IDEs inline w/o showing full class documentation.

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
@@ -50,6 +50,8 @@
* @property integer $port Port number for insecure requests.
* @property integer $securePort Port number for secure requests.
* @property CCookieCollection|CHttpCookie[] $cookies The cookie collection.
+ * @property array $preferredAcceptType The user preferred accept type as an array map, containing type, sub-type, quality and parameters.
+ * @property array $preferredAcceptTypes An array of all user accepted types in order of preference (see above).
samdark
samdark Mar 21, 2013 Owner

Like array('typea', 'typeb')?

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
@@ -801,6 +804,136 @@ public function redirect($url,$terminate=true,$statusCode=302)
}
/**
+ * Parses an Http Accept header, returning an array map with all parts of each entry.
+ * Each array entry consists of a map with the type, subType, quality (ie preference ranking) and params, an array map of key-value parameters.
@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
@@ -801,6 +804,136 @@ public function redirect($url,$terminate=true,$statusCode=302)
}
/**
+ * Parses an Http Accept header, returning an array map with all parts of each entry.
+ * Each array entry consists of a map with the type, subType, quality (ie preference ranking) and params, an array map of key-value parameters.
+ * For example, an Accept value of 'application/xhtml+xml;q=0.9;level=1' would give an array entry of
+ * array(
+ * 'type' => 'application',
+ * 'subType' => 'xhtml',
+ * 'baseType' => 'xml',
+ * 'params' => array(
+ * 'q' => 0.9,
+ * 'level' => '1',
+ * ),
+ * )
+ * NB - to avoid great complexity, there are no steps taken to ensure that quoted strings are treated properly - if
Contributor

If we are just talking about small text changes here, I'm quite happy for whoever does the merge to change to what they want.
The array map description is given in the parseAcceptHeader() function - it would seem pointless to repeat the same description over and over...

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ * 'level' => '1',
+ * ),
+ * )
+ * NB - to avoid great complexity, there are no steps taken to ensure that quoted strings are treated properly - if
+ * the header text includes quoted strings containing the , or ; characters then the results may not be correct!
+ * @return array the user accepted MIME types.
+ * See {@link http://tools.ietf.org/html/rfc2616#section-14.1}
+ */
+ public static function parseAcceptHeader($header)
+ {
+ $matches=array();
+ $accepts=array();
+ // get individual entries with their type, subtype, basetype and params
+ preg_match_all('/(?:\G\s?,\s?|^)(\w+)\/(\w+)(?:\+(\w+))?|(?<!^)\G(?:\s?;\s?(\w+)=([\w\.]+))/',$header,$matches);
+ // the regexp should (in theory) always return an array of 6 arrays
+ if(count($matches)===6) {
samdark
samdark Mar 21, 2013 Owner

{ should be at the next line

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ * ),
+ * )
+ * NB - to avoid great complexity, there are no steps taken to ensure that quoted strings are treated properly - if
+ * the header text includes quoted strings containing the , or ; characters then the results may not be correct!
+ * @return array the user accepted MIME types.
+ * See {@link http://tools.ietf.org/html/rfc2616#section-14.1}
+ */
+ public static function parseAcceptHeader($header)
+ {
+ $matches=array();
+ $accepts=array();
+ // get individual entries with their type, subtype, basetype and params
+ preg_match_all('/(?:\G\s?,\s?|^)(\w+)\/(\w+)(?:\+(\w+))?|(?<!^)\G(?:\s?;\s?(\w+)=([\w\.]+))/',$header,$matches);
+ // the regexp should (in theory) always return an array of 6 arrays
+ if(count($matches)===6) {
+ for($i=0,$itemLen=count($matches[1]);$i<$itemLen;)
samdark
samdark Mar 21, 2013 Owner

Tricky nested for-s :) Can we simplify these or change to while?

@samdark samdark commented on the diff Mar 21, 2013
framework/web/CHttpRequest.php
+ $accept['params'][$matches[4][$i]]=$matches[5][$i];
+ }
+ else
+ break;
+ }
+ // q defaults to 1 if not explicitly given
+ if(!isset($accept['params']['q']))
+ $accept['params']['q']=(double)1;
+ $accepts[] = $accept;
+ }
+ }
+ return $accepts;
+ }
+
+ /**
+ * Compare function for determining the preference of accepted MIME type array maps
samdark
samdark Mar 21, 2013 Owner

Change to

Compare function for sorting MIME types according to the preference

?

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

The compare function itself doesn't do any sorting - it just returns whether $a is preferred over $b.
And we can't strictly say that it is applied to MIME types either - it is applied to the array map that we use as a structure to hold the MIME type.

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ if(!isset($accept['params']['q']))
+ $accept['params']['q']=(double)1;
+ $accepts[] = $accept;
+ }
+ }
+ return $accepts;
+ }
+
+ /**
+ * Compare function for determining the preference of accepted MIME type array maps
+ * @param array $a user accepted MIME type as an array map
+ * @param array $b user accepted MIME type as an array map
+ * @return integer -1, 0 or 1 if $a has respectively less preference, equal preference or greater preference than $b.
+ * See {@link parseAcceptHeader()} for the format of $a and $b
+ */
+ public static function compareAcceptTypes($a, $b)
samdark
samdark Mar 21, 2013 Owner

Should be no space after ,

@samdark samdark and 1 other commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ * @param array $b user accepted MIME type as an array map
+ * @return integer -1, 0 or 1 if $a has respectively less preference, equal preference or greater preference than $b.
+ * See {@link parseAcceptHeader()} for the format of $a and $b
+ */
+ public static function compareAcceptTypes($a, $b)
+ {
+ // check for equal quality first
+ if($a['params']['q']==$b['params']['q'])
+ {
+ if(!($a['type']=='*' xor $b['type']=='*'))
+ {
+ if (!($a['subType']=='*' xor $b['subType']=='*'))
+ {
+ // finally, higher number of parameters counts as greater precedence
+ if(count($a['params'])==count($b['params']))
+ {
samdark
samdark Mar 21, 2013 Owner

Can remove { and }

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

You mean use single-line nested ifs for all the conditions?

samdark
samdark Mar 21, 2013 Owner

Yes. That's 1.1 code style. Will be changed to a better one for 2.0 but now we should use it.

@samdark samdark and 1 other commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ // check for equal quality first
+ if($a['params']['q']==$b['params']['q'])
+ {
+ if(!($a['type']=='*' xor $b['type']=='*'))
+ {
+ if (!($a['subType']=='*' xor $b['subType']=='*'))
+ {
+ // finally, higher number of parameters counts as greater precedence
+ if(count($a['params'])==count($b['params']))
+ {
+ return 0;
+ }
+ return count($a['params'])<count($b['params']) ? 1 : -1;
+ }
+ // more specific takes precedence - whichever one doesn't have a * subType
+ return $a['subType']=='*' ? 1 : -1;
samdark
samdark Mar 21, 2013 Owner

Can use === here

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

Yep - typo!

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ return $accepts;
+ }
+
+ /**
+ * Compare function for determining the preference of accepted MIME type array maps
+ * @param array $a user accepted MIME type as an array map
+ * @param array $b user accepted MIME type as an array map
+ * @return integer -1, 0 or 1 if $a has respectively less preference, equal preference or greater preference than $b.
+ * See {@link parseAcceptHeader()} for the format of $a and $b
+ */
+ public static function compareAcceptTypes($a, $b)
+ {
+ // check for equal quality first
+ if($a['params']['q']==$b['params']['q'])
+ {
+ if(!($a['type']=='*' xor $b['type']=='*'))
samdark
samdark Mar 21, 2013 Owner

Can use === here

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+
+ /**
+ * Compare function for determining the preference of accepted MIME type array maps
+ * @param array $a user accepted MIME type as an array map
+ * @param array $b user accepted MIME type as an array map
+ * @return integer -1, 0 or 1 if $a has respectively less preference, equal preference or greater preference than $b.
+ * See {@link parseAcceptHeader()} for the format of $a and $b
+ */
+ public static function compareAcceptTypes($a, $b)
+ {
+ // check for equal quality first
+ if($a['params']['q']==$b['params']['q'])
+ {
+ if(!($a['type']=='*' xor $b['type']=='*'))
+ {
+ if (!($a['subType']=='*' xor $b['subType']=='*'))
samdark
samdark Mar 21, 2013 Owner

Can use === here

@samdark samdark and 1 other commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ }
+ }
+ return $accepts;
+ }
+
+ /**
+ * Compare function for determining the preference of accepted MIME type array maps
+ * @param array $a user accepted MIME type as an array map
+ * @param array $b user accepted MIME type as an array map
+ * @return integer -1, 0 or 1 if $a has respectively less preference, equal preference or greater preference than $b.
+ * See {@link parseAcceptHeader()} for the format of $a and $b
+ */
+ public static function compareAcceptTypes($a, $b)
+ {
+ // check for equal quality first
+ if($a['params']['q']==$b['params']['q'])
samdark
samdark Mar 21, 2013 Owner

If there are strings all the time === can be used here

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

No, q should always be a double (from the parse function), but all other params should always be strings.
I usually do use === all the time, so I'm not sure why I didn't in this function, but will correct.

@samdark samdark and 1 other commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ */
+ public static function compareAcceptTypes($a, $b)
+ {
+ // check for equal quality first
+ if($a['params']['q']==$b['params']['q'])
+ {
+ if(!($a['type']=='*' xor $b['type']=='*'))
+ {
+ if (!($a['subType']=='*' xor $b['subType']=='*'))
+ {
+ // finally, higher number of parameters counts as greater precedence
+ if(count($a['params'])==count($b['params']))
+ {
+ return 0;
+ }
+ return count($a['params'])<count($b['params']) ? 1 : -1;
samdark
samdark Mar 21, 2013 Owner

Can do return $b['params'] - count($a['params']) instead of lines 893 — 897.

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

Could do, I suppose the compare function only needs to return >0, <0 or 0, I was just trying to stick to returning -1, 0 or 1.
The param count is the lowest differentiating factor - I wouldn't want it to have a higher weighting than different qs for example.

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

The more I think about it, the more reluctant I am to do this - ok the usort function doesn't care whether the compare function returns -1 or anything less than 0, but strictly, since this is a compare function, I think that the value returned should either bear some relation to the order of difference between the two objects, or be a simple ternary, rather than returning a value that might imply that the difference between objects $a and $b is greater than that between objects $b and $c when in fact $a just has more parameters than $b (an integer difference) while perhaps there is a small (double) difference in q value between $b and $c.

samdark
samdark Mar 21, 2013 Owner

That's OK for usort. It will work.

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

I know that it will work, that is not the point. The method is no longer an anonymous function passed to the usort function - it is a public method in the CHttpRequest class, and as such I think that what it returns should be consistent.
If we have content-type entries:
a) text/html; q=0.9; foo=bar
b) application/json; q=0.9
c) application/xhtml+xml; q=0.95
Then, a) is preferred to b) because it has 1 more parameter (being equal in all other respects)
c) is preferred to a) by a long way, because its most important criteria (the q value) is higher
Using the simplified comparison function, compare($a, $b) gives 1 and compare($c, $b) gives 0.05, which would seem to imply that $a would be preferred to $c since it gets a higher 'score', which is not the case.
So, in my opinion, the method should either return a properly weighted value (which the simplified comparison does not give), or a ternary value which is not open to misleading interpretations.

samdark
samdark Mar 21, 2013 Owner

Yeah. You're right.

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

Actually, I've just rechecked usort and it wouldn't work anyway for the q comparison - it casts any non integer values returned to integers, and since 0 <= q <= 1 then q2 - q1 will always give 0, so definitely not a good idea to simplify that return!

@samdark samdark and 1 other commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ if (!($a['subType']=='*' xor $b['subType']=='*'))
+ {
+ // finally, higher number of parameters counts as greater precedence
+ if(count($a['params'])==count($b['params']))
+ {
+ return 0;
+ }
+ return count($a['params'])<count($b['params']) ? 1 : -1;
+ }
+ // more specific takes precedence - whichever one doesn't have a * subType
+ return $a['subType']=='*' ? 1 : -1;
+ }
+ // more specific takes precedence - whichever one doesn't have a * type
+ return $a['type']=='*' ? 1 : -1;
+ }
+ return ($a['params']['q']<$b['params']['q']) ? 1 : -1;
samdark
samdark Mar 21, 2013 Owner

Same here, can do return $b['params']['q']-$a['params']['q']

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

As above.

@samdark samdark commented on the diff Mar 21, 2013
framework/web/CHttpRequest.php
+ }
+ return count($a['params'])<count($b['params']) ? 1 : -1;
+ }
+ // more specific takes precedence - whichever one doesn't have a * subType
+ return $a['subType']=='*' ? 1 : -1;
+ }
+ // more specific takes precedence - whichever one doesn't have a * type
+ return $a['type']=='*' ? 1 : -1;
+ }
+ return ($a['params']['q']<$b['params']['q']) ? 1 : -1;
+ }
+
+ /**
+ * Returns an array of user accepted MIME types in order of preference.
+ * Each array entry consists of a map with the type, subType, baseType and params, an array map of key-value parameters.
+ * See {@link parseAcceptHeader()} for a description of the array map.
samdark
samdark Mar 21, 2013 Owner

That's very good. Link to full description is a good idea.

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ }
+ return ($a['params']['q']<$b['params']['q']) ? 1 : -1;
+ }
+
+ /**
+ * Returns an array of user accepted MIME types in order of preference.
+ * Each array entry consists of a map with the type, subType, baseType and params, an array map of key-value parameters.
+ * See {@link parseAcceptHeader()} for a description of the array map.
+ * @return array the user accepted MIME types, as array maps, in the order of preference.
+ */
+ public function getPreferredAcceptTypes()
+ {
+ if($this->_preferredAcceptTypes===null)
+ {
+ $accepts=self::parseAcceptHeader($this->getAcceptTypes());
+ usort($accepts,array(get_class($this), 'compareAcceptTypes'));
samdark
samdark Mar 21, 2013 Owner

Should be no space after ,.

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ * @return array the user accepted MIME types, as array maps, in the order of preference.
+ */
+ public function getPreferredAcceptTypes()
+ {
+ if($this->_preferredAcceptTypes===null)
+ {
+ $accepts=self::parseAcceptHeader($this->getAcceptTypes());
+ usort($accepts,array(get_class($this), 'compareAcceptTypes'));
+ $this->_preferredAcceptTypes=$accepts;
+ }
+ return $this->_preferredAcceptTypes;
+ }
+
+ /**
+ * Returns the user preferred accept MIME type.
+ The MIME type is returned as an array map (see {@link parseAcceptHeader()}.
samdark
samdark Mar 21, 2013 Owner

Missing *

@samdark samdark commented on an outdated diff Mar 21, 2013
framework/web/CHttpRequest.php
+ $accepts=self::parseAcceptHeader($this->getAcceptTypes());
+ usort($accepts,array(get_class($this), 'compareAcceptTypes'));
+ $this->_preferredAcceptTypes=$accepts;
+ }
+ return $this->_preferredAcceptTypes;
+ }
+
+ /**
+ * Returns the user preferred accept MIME type.
+ The MIME type is returned as an array map (see {@link parseAcceptHeader()}.
+ * @return array the user preferred accept MIME type or false if the user does not have any.
+ */
+ public function getPreferredAcceptType()
+ {
+ $preferredAcceptTypes=$this->getPreferredAcceptTypes();
+ return !empty($preferredAcceptTypes) ? $preferredAcceptTypes[0] : false;
samdark
samdark Mar 21, 2013 Owner

Can be inverted i.e. return empty(

Owner
samdark commented Mar 21, 2013

Still can be improved ;)

Contributor

Everything corrected, apart from the compare function description, and the compare function return simplification, for the reasons discussed in the diff.

Owner
samdark commented Mar 21, 2013

OK, now it's fine for me except usort callback. I'm still thinking it can be simplified with substraction.

Ragazzo commented Mar 21, 2013

well, since there are no tests for this ench. i think functions compareAcceptTypes and parseAcceptHeader must be protected, no?

Owner
samdark commented Mar 21, 2013

Tests would be helpful since the code there isn't trivial.

Contributor

Tests are probably a good idea, but I have absolutely no idea as to how they should be built/included in Yii.
I don't see any argument for the methods being protected though - they are both static functions, so do not modify properties in any way. If the idea were to disallow their public use then they should be private rather than protected, but I can't see any need for that either - the compare function will never return anything different from what it returns now (numerical values required for the usort), and the structure of the array map for the parse function is unlikely to change, and even if it did it just requires standard array isset checking, unless we were to create a class for the structure, but that would be overkill.

Ragazzo commented Mar 21, 2013

there is a folder named tests in framework, u can submit some files with tests there.

Contributor

I was afraid that would be the case! :P

Rupert-RR added some commits Mar 21, 2013
Rupert-RR Added unit test file for CHttpRequest for the methods
parseAcceptHeader() and compareAcceptTypes().
Modified the regexp in parseAcceptHeader() to accept wildcards in the
path.
Modified the description of compareAcceptTypes() to better reflect the
comparison result (higher preference returns lower value, so that most
preferred is first in the array).
2ae1700
Rupert-RR Merge branch 'master' of git://github.com/yiisoft/yii into 2131-add-a…
…ccept-header-parsing

Merge from upstream.
f088486
Contributor

Ok, I have added unit tests (which was just as well because that way I found that I had forgotten to include wildcard paths in the regexp).
So hopefully all is good now... ;)

Contributor

Oops - I hadn't realised that permission changes are included in commits - I presume you don't want that there...

Ragazzo commented Mar 21, 2013

maybe use dataProviders in tests, because there are only 2-3 rows with exactly tests and other are fixtures, what do u think, it also will be easy to read after that?

Contributor

I did think about it but then I presumed that it would be difficult to see which kind of data it failed on.

Ragazzo commented Mar 21, 2013

no, phpunit has good verbose messages to tell the developer on which set of data test failed.

Contributor

Ok, done! (and I agree - it is clearer)

@Ragazzo Ragazzo and 1 other commented on an outdated diff Mar 21, 2013
tests/framework/web/CHttpRequestTest.php
+ {
+ $this->assertEquals($result,CHttpRequest::parseAcceptHeader($header),$errorString);
+ }
+
+ /**
+ * @covers CHttpRequest::compareAcceptTypes
+ * @dataProvider acceptContentTypeArrayMapDataProvider
+ */
+ public function testCompareAcceptTypes($a,$b,$result,$errorString='Compare of content type array maps did not give expected preference')
+ {
+ $this->assertEquals($result,CHttpRequest::compareAcceptTypes($a,$b),$errorString);
+ // make sure that inverse comparison holds
+ $this->assertEquals($result*-1,CHttpRequest::compareAcceptTypes($b,$a),'(Inverse) '.$errorString);
+ }
+
+ public function acceptHeaderDataProvider() {
Ragazzo
Ragazzo Mar 21, 2013

{ on new line and other dataprovider function too, i think this is the last suggestion for a really great work made by you ))

Rupert-RR
Rupert-RR Mar 21, 2013 Contributor

I always forget (personally I much prefer this way round)!
Thanks.

Owner
samdark commented Mar 21, 2013

Now it's starting to look perfect :) Thanks for not giving up after so much critics.

@cebe can you do a final check and merge?

Owner
cebe commented Mar 21, 2013

Will do.

Owner

@Rupert-RR I'm glad you made it into a single regex. Great job!

Contributor

Thanks guys - no problem for the criticisms - that's the point of the whole system, as long as its constructive - thanks for taking the time to check things.
@qiangxue I can't take much credit for the regex - I got some great help on Stack Exchange for that - just shows I need to improve my regexes.

@cebe cebe added a commit that referenced this pull request Mar 27, 2013
@cebe cebe php doc adjustments after #2132 7d91ceb
@cebe cebe added a commit that referenced this pull request Mar 27, 2013
@cebe cebe Merge branch 'Rupert-RR-2131-add-accept-header-parsing'
* Rupert-RR-2131-add-accept-header-parsing:
  php doc adjustments after #2132
  Yii code style correction
  Removed unnecessary spaces.
  Transferred data for unit tests from the test functions into data providers.
  Undo accidental permissions change on bootstrap.php
  Added unit test file for CHttpRequest for the methods parseAcceptHeader() and compareAcceptTypes(). Modified the regexp in parseAcceptHeader() to accept wildcards in the path. Modified the description of compareAcceptTypes() to better reflect the comparison result (higher preference returns lower value, so that most preferred is first in the array).
  parseAcceptHeader() function description tidy up.
  Typo corrections and code tidy up.
  Altered parseAcceptHeader() to use only one regexp. Thanks to Ka on StackExchange for this expression.
  Typo corrections
  Separated out parse and compare functions. Reduced regular expression count to 2 from 3. Modified MIME type array map structure.
  Added #135 back in changelog, which got lost somehow..
  Moved position of line to follow numerical order.
  Enh #2131: Added Accept header parsing to CHttpRequest to give an array of accepted types in order of preference
968ff9c
@cebe cebe merged commit 006a893 into yiisoft:master Mar 27, 2013
Owner
cebe commented Mar 27, 2013

Merged, thanks!

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