Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[WebProfilerBundle] fixed getSession when no session has been set deprecation warnings#27458
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
nicolas-grekas left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
minor CS comment (we don't use brackets usually when not needed, like here)
| } | ||
| if (null ===$session =$request->getSession()) { | ||
| if (!$request->hasSession() ||null ===($session =$request->getSession())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'd suggest to useif (!$request->hasSession() || !$session = $request->getSession()) {
| $token =$request->query->get('token'); | ||
| if (null !==$session =$request->getSession()) { | ||
| if ($request->hasSession() && ($session =$request->getSession())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if ($request->hasSession() && $session = $request->getSession()) {
| {%macro profile_search_filter(request,result,property) %} | ||
| {%-ifrequest.sessionis notnull -%} | ||
| {%-ifrequest.hasSessionandrequest.sessionis notnull -%} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Why not only{%- if request.hasSession -%} ?
| } | ||
| if (null ===$session =$request->getSession()) { | ||
| if (!$request->hasSession() || !$session =$request->getSession()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
if (!$request->hasSession()) is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Are you sure? I think we need $session in the else part.
We could move$session = $request->getSession() into the else though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
moving it to theelse seems reasonable to me
nicolas-grekas commentedJun 4, 2018
Thank you@GregOriol. |
…een set deprecation warnings (GregOriol)This PR was submitted for the master branch but it was squashed and merged into the 4.1 branch instead (closes#27458).Discussion----------[WebProfilerBundle] fixed getSession when no session has been set deprecation warnings| Q | A| ------------- | ---| Branch? | 4.1| Bug fix? | yes| New feature? | no| BC breaks? | no| Deprecations? | no| Tests pass? | yes| Fixed tickets | none but see comment| License | MITSymfony 4.1 adds deprecation warnings when getSession is called and a session has not been set.Some code in the WebProfiler was not up-to-date to use hasSession before.(this actually caused [Silex-WebProfiler](https://github.com/silexphp/Silex-WebProfiler)'s tests to fail)Commits-------0f3ba5a [WebProfilerBundle] fixed getSession when no session has been set deprecation warnings
Uh oh!
There was an error while loading.Please reload this page.
Symfony 4.1 adds deprecation warnings when getSession is called and a session has not been set.
Some code in the WebProfiler was not up-to-date to use hasSession before.
(this actually causedSilex-WebProfiler's tests to fail)