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

[WebProfilerBundle] fixed getSession when no session has been set deprecation warnings #27458

Merged
merged 1 commit into from Jun 4, 2018

Conversation

Projects
None yet
6 participants
@GregOriol
Contributor

GregOriol commented May 31, 2018

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 MIT

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 caused Silex-WebProfiler's tests to fail)

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Jun 1, 2018

@nicolas-grekas

minor CS comment (we don't use brackets usually when not needed, like here)

@@ -180,7 +180,7 @@ public function searchBarAction(Request $request)
$this->cspHandler->disableCsp();
}
if (null === $session = $request->getSession()) {
if (!$request->hasSession() || null === ($session = $request->getSession())) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 1, 2018

Member

I'd suggest to use if (!$request->hasSession() || !$session = $request->getSession()) {

@@ -289,7 +289,7 @@ public function searchAction(Request $request)
$limit = $request->query->get('limit');
$token = $request->query->get('token');
if (null !== $session = $request->getSession()) {
if ($request->hasSession() && ($session = $request->getSession())) {

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Jun 1, 2018

Member

if ($request->hasSession() && $session = $request->getSession()) {

@GregOriol GregOriol referenced this pull request Jun 1, 2018

Open

upgrade to Symfony 4 #132

GregOriol added a commit to GregOriol/symfony that referenced this pull request Jun 1, 2018

Enhanced CS
(following comments in symfony#27458)
@@ -1,7 +1,7 @@
{% extends '@WebProfiler/Profiler/layout.html.twig' %}
{% macro profile_search_filter(request, result, property) %}
{%- if request.session is not null -%}
{%- if request.hasSession and request.session is not null -%}

This comment has been minimized.

@alcalyn

alcalyn Jun 1, 2018

Contributor

Why not only {%- if request.hasSession -%} ?

GregOriol added a commit to GregOriol/symfony that referenced this pull request Jun 1, 2018

@@ -180,7 +180,7 @@ public function searchBarAction(Request $request)
$this->cspHandler->disableCsp();
}
if (null === $session = $request->getSession()) {
if (!$request->hasSession() || !$session = $request->getSession()) {

This comment has been minimized.

@xabbuh

xabbuh Jun 2, 2018

Member

if (!$request->hasSession()) is enough

This comment has been minimized.

@GregOriol

GregOriol Jun 2, 2018

Contributor

Are you sure? I think we need $session in the else part.
We could move $session = $request->getSession() into the else though.

This comment has been minimized.

@xabbuh

xabbuh Jun 2, 2018

Member

moving it to the else seems reasonable to me

@xabbuh

xabbuh approved these changes Jun 2, 2018

@alcalyn

alcalyn approved these changes Jun 4, 2018

@nicolas-grekas nicolas-grekas changed the base branch from master to 4.1 Jun 4, 2018

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Jun 4, 2018

Thank you @GregOriol.

@nicolas-grekas nicolas-grekas merged commit 0f3ba5a into symfony:4.1 Jun 4, 2018

1 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Jun 4, 2018

bug #27458 [WebProfilerBundle] fixed getSession when no session has b…
…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       | MIT

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 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

@fabpot fabpot referenced this pull request Jun 25, 2018

Merged

Release v4.1.1 #27706

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment