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.

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 3 commits into from
Apr 28, 2016

Conversation

HeahDude
Copy link
Contributor

@HeahDude HeahDude commented Apr 22, 2016

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

@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
fixes redirection profile introduced in
0a1b284.

Prevents collecting redirect data on sub request profiling.
@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

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
…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
'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
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
Morty Proxy This is a proxified and sanitized view of the page, visit original site.