Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Zend\Http\Header\GenericHeader introduces BC break #7295

Merged
merged 2 commits into from
Mar 23, 2015

Conversation

weierophinney
Copy link
Member

V2.3.4 (and possibly earlier) has a BC break to version 2.2.3 (and possibly later)
Links to: #5301

This is caused by the insistence that underscores are converted to hyphens at L92. It is debatable as to whether RFC2616 actually insists that underscores are not allowed.

Irrespective of that, the code

  • should not change header names under the covers
  • should throw an exception as it does a few lines later for non RFC compliance, which would at least warn developers that something is wrong, rather than trying to figure out why their code suddenly breaks when a service endpoint is hit, and is consistent with the exception throwing later.

In addition, it would be useful to include a NonRfc2616Header that can be used in place of GenericHeader with appropriate documentation in the reference guide.

@Martin-P
Copy link
Contributor

Martin-P commented Mar 5, 2015

It is debatable as to whether RFC2616 actually insists that underscores are not allowed.

It is not done because of RFC2616, but for Apache. See this comment: #5301 (comment)

Apache, basically. The PhpEnvironment-specfic request object may pull the headers from get_headers(), and those often substitute underscores for dashes.

@ghost
Copy link
Author

ghost commented Mar 5, 2015

That may be the case for servers running PHP/Apache, but in this case (and I suspect many others,) the target server is a not PHP/Apache, but a glassfish or tomcat server. i.e. it is the target that dictates the acceptable header name format, not the client, hence my comment "should not change header names under the covers"

@weierophinney
Copy link
Member

RFC-2616 is dead, and RFC 7230 supercedes it. In looking through the spec, header field names are allowed to be a token value. In the spec, that's defined as:

token = 1*tchar

In other words, 1 or more tchar values. Those are defined as:

tchar = "!" / "#" / "$" / "%" / "&" / "'" / "*" / "+" / "-" / "." /
    "^" / "_" / "`" / "|" / "~" / DIGIT / ALPHA

The above clearly notes _ as a valid value.

I'll try and get a patch today so we can have the fix in 2.4.

@weierophinney weierophinney added this to the 2.4.0 milestone Mar 23, 2015
@weierophinney weierophinney self-assigned this Mar 23, 2015
@weierophinney
Copy link
Member

Tests passed against my branch: https://travis-ci.org/weierophinney/zf2/builds/55541093

*/
if (!preg_match('/^[!#-\'*+\-\.0-9A-Z\^-z|~]+$/', $fieldName)) {
if (!preg_match('/^[!#$%&\'*+\-\.\^_`|~0-9a-zA-Z]+$/', $fieldName)) {
throw new Exception\InvalidArgumentException(
'Header name must be a valid RFC 2616 (section 4.2) field-name.'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given how RFC 2616 is superceded by RFC 7230 (and various others), maybe we should also just update the exception message as well?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point; I'll change it now.

@mwillbanks mwillbanks merged commit 3893080 into zendframework:master Mar 23, 2015
mwillbanks added a commit that referenced this pull request Mar 23, 2015
mwillbanks added a commit that referenced this pull request Mar 23, 2015
@weierophinney weierophinney deleted the hotfix/7295 branch March 24, 2015 13:27
gianarb pushed a commit to zendframework/zend-http that referenced this pull request May 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants