Skip to content

[WebProfilerBundle] Display multiple HTTP headers in WDT #20599

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

Merged
merged 1 commit into from
Dec 19, 2016
Merged

[WebProfilerBundle] Display multiple HTTP headers in WDT #20599

merged 1 commit into from
Dec 19, 2016

Conversation

ro0NL
Copy link
Contributor

@ro0NL ro0NL commented Nov 22, 2016

Q A
Branch? 2.7 (partially in 3.2 already)
Bug fix? yes
New feature? no
BC breaks? no-ish
Deprecations? no
Tests pass? yes
Fixed tickets #20595 (comment)
License MIT
Doc PR reference to the documentation PR, if any

Backport of #20595 for 2.7, that includes a fix for displaying multiple HTTP headers in the WDT.

image

//cc @nicolas-grekas this should cover 2.7 - 3.1 right?

@nicolas-grekas
Copy link
Member

FYI, I moved the logic in the request collector: https://github.com/symfony/symfony/pull/20595/files#diff-e8f5b14fbfbbeac60fc9f3abe310c3b0R128

@ro0NL
Copy link
Contributor Author

ro0NL commented Nov 22, 2016

Updated, rebasing should be easy now.

@nicolas-grekas nicolas-grekas added this to the 2.7 milestone Dec 6, 2016
@fabpot
Copy link
Member

fabpot commented Dec 19, 2016

Thank you @ro0NL.

@fabpot fabpot merged commit 257a856 into symfony:2.7 Dec 19, 2016
fabpot added a commit that referenced this pull request Dec 19, 2016
…ro0NL)

This PR was merged into the 2.7 branch.

Discussion
----------

[WebProfilerBundle] Display multiple HTTP headers in WDT

| Q             | A
| ------------- | ---
| Branch?       | 2.7 (partially in 3.2 already)
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no-ish
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | [#20595 (comment)](#20595 (comment))
| License       | MIT
| Doc PR        | reference to the documentation PR, if any

Backport of #20595 for 2.7, that includes a fix for displaying multiple HTTP headers in the WDT.

![image](https://cloud.githubusercontent.com/assets/1047696/20536902/43de2692-b0eb-11e6-8eb7-f0584daf5963.png)

//cc @nicolas-grekas this should cover 2.7 - 3.1 right?

Commits
-------

257a856 [WebProfilerBundle] Display multiple HTTP headers in WDT
@ro0NL ro0NL deleted the wdt/multiple-headers branch December 19, 2016 08:22
Copy link
Contributor Author

@ro0NL ro0NL left a comment

Choose a reason for hiding this comment

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

@fabpot sorry for the inconvenience :) but im not sure upstream merges went as expected..?

edit: is this about 3.1 almost being EOM?

}
if (count($cookies) > 0) {
$responseHeaders['Set-Cookie'] = $cookies;
$responseHeaders['set-cookie'][] = (string) $cookie;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Above should apply to 3.1 & 3.2 as well.

In master we can remove this block due #20569. I covered this with #20567 and should be a separate PR otherwise (basically forgotten in #20569).

$this->data[$key] = $value;
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Above should apply to 3.1 as well.

In master this is already applied due #20595.

@@ -183,7 +189,7 @@ public function getRequestQuery()

public function getRequestHeaders()
{
return new HeaderBag($this->data['request_headers']);
return new ParameterBag($this->data['request_headers']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same, should be applied to 3.1

@@ -203,7 +209,7 @@ public function getRequestAttributes()

public function getResponseHeaders()
{
return new ResponseHeaderBag($this->data['response_headers']);
return new ParameterBag($this->data['response_headers']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same, should be applied to 3.1

}

return $cookie;
}
Copy link
Contributor Author

@ro0NL ro0NL Dec 19, 2016

Choose a reason for hiding this comment

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

Same, should be applied to 3.1

and the remaining tests of course.

fabpot added a commit that referenced this pull request Dec 22, 2016
This PR was merged into the 3.1 branch.

Discussion
----------

[HttpKernel] Continuation of #20599 for 3.1

| Q             | A
| ------------- | ---
| Branch?       | 3.1 only
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | #... <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

#20599 is only merged up to 2.8.. not sure if 3.1 is already EOM?

Commits
-------

c245f87 [HttpKernel] Continuation of #20599 for 3.1
fabpot added a commit that referenced this pull request Dec 22, 2016
* 3.1:
  [HttpKernel] Continuation of #20599 for 3.1
fabpot added a commit that referenced this pull request Dec 22, 2016
* 3.2:
  [HttpKernel] Continuation of #20599 for 3.1
  Add missing upgrade path for Request::isMethodSafe() deprecation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants