Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings

[HttpKernel] tweaked redirection profiling in RequestDataCollector #18618

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Apr 28, 2016
Merged

[HttpKernel] tweaked redirection profiling in RequestDataCollector #18618

merged 3 commits into from
Apr 28, 2016

Conversation

HeahDude
Copy link
Contributor

@HeahDude HeahDude commented Apr 22, 2016 •
edited
Loading

Q A
Branch? master
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets ~
License MIT
Doc PR ~

@HeahDude
Copy link
Contributor Author

HeahDude commented Apr 22, 2016 •
edited
Loading

@javiereguiluz it happens because of this line https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/EventListener/ProfilerListener.php#L81.

But the scope of RequestDataCollector::collect() does not allow to know it the request is the sub request.
So I had to register the RequestDataCollector on KernelEvents::RESPONSE to get it from the event.

I also refactored a bit since $session was already defined in RequestDataCollector::collect().

Please tell me if the fix works for you too, and thanks again for reporting it :)

@HeahDude HeahDude changed the title Fix/profile redirect [HttpKernel] tweaked redirection profiling in RequestDataCollector Apr 22, 2016
@HeahDude HeahDude force-pushed the fix/profile-redirect branch 3 times, most recently from 4127b5c to a47d2e8 Compare April 22, 2016 19:00
HeahDude added 2 commits April 23, 2016 05:34
@HeahDude
[HttpKernel] added RequestDataCollector::onKernelResponse()
b26cb6d 
fixes redirection profile introduced in
0a1b284.

Prevents collecting redirect data on sub request profiling.
@HeahDude
use a request attribute flag for redirection profile
df19c14
@HeahDude HeahDude force-pushed the fix/profile-redirect branch from a47d2e8 to df19c14 Compare April 23, 2016 06:52
@HeahDude
Copy link
Contributor Author

HeahDude commented Apr 23, 2016 •
edited
Loading

I reverted the redirect controller thing as I got the wrong result while testing it with symfony-demo.
Also I've pushed df19c14 to store the redirected flag in the request attributes instead of the session and used the returned value of Session::remove. If you agree with this commit I'll squash it.

Thanks.

@fabpot
Copy link
Member

fabpot commented Apr 26, 2016

ping @javiereguiluz

@fabpot
Copy link
Member

fabpot commented Apr 28, 2016

Thank you @HeahDude.

@fabpot fabpot merged commit df19c14 into symfony:master Apr 28, 2016
fabpot added a commit that referenced this pull request Apr 28, 2016
@fabpot
bug #18618 [HttpKernel] tweaked redirection profiling in RequestDataC…
89b6bb8 
…ollector (HeahDude)

This PR was merged into the 3.1-dev branch.

Discussion
----------

[HttpKernel] tweaked redirection profiling in RequestDataCollector

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ~
| License       | MIT
| Doc PR        | ~

- c8ba3b2 removes duplicated code forgotten in #17589

- a47d2e8 fixes the collecting of redirect data in first sub request instead of redirected master request.

Commits
-------

df19c14 use a request attribute flag for redirection profile
b26cb6d [HttpKernel] added RequestDataCollector::onKernelResponse()
c8ba3b2 [HttpKernel] remove legacy duplicated code
fabpot
fabpot reviewed Apr 28, 2016
View reviewed changes
src/Symfony/Component/HttpKernel/DataCollector/RequestDataCollector.php
'status_text' => Response::$statusTexts[(int) $statusCode],
));
}
if (isset($session)) {
Copy link
Member

Choose a reason for hiding this comment

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

Using isset on a variable should be avoided. I've fixed it in becdbd9

@HeahDude HeahDude deleted the fix/profile-redirect branch April 28, 2016 10:48
@fabpot fabpot mentioned this pull request May 13, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Bug HttpKernel Status: Needs Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@HeahDude @fabpot @javiereguiluz @carsonbot
Morty Proxy This is a proxified and sanitized view of the page, visit original site.