Skip to content
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

Fix EZP-28612: Session is always started for anonymous user #241

Merged
merged 1 commit into from
Dec 20, 2017

Conversation

emodric
Copy link
Contributor

@emodric emodric commented Dec 19, 2017

Question Answer
Tickets https://jira.ez.no/browse/EZP-28612
Bug fix? yes
New feature? no
BC breaks? no
Tests pass? yes
Doc needed? no
License GPL-2.0

This refactors a bit FlashBagNotificationHandler in order not to start the session for anonymous user, by calling the getFlashBag only when neeeded, since it is responsible for starting the session.

Checklist:

  • Coding standards ($ composer fix-cs)
  • Ready for Code Review

@andrerom
Copy link
Contributor

@Nattfarinn Guess you forgot to mention on slack you where going to look at it? Ref #240

@emodric
Copy link
Contributor Author

emodric commented Dec 19, 2017

@andrerom @Nattfarinn Damn :)

Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

+1, thanks @emodric 🎉
This solves also the bug uncovered by REST functional tests // cc @andrerom

@lserwatka lserwatka merged commit 31f02e2 into ezsystems:master Dec 20, 2017
@andrerom
Copy link
Contributor

👍

@emodric
Copy link
Contributor Author

emodric commented Dec 20, 2017

Nice! :)

What about template change in #240 about using {% if app.request.hasPreviousSession %} ? Is that needed?

EDIT: It should work without it, app.flashes will not start the session: https://symfony.com/blog/new-in-symfony-3-3-improved-flash-messages

@Nattfarinn
Copy link
Contributor

@emodric Even if it would start, it's not required. This "if" is a good practice to avoid anonymous session starts, but in this case it's used inside Admin panel so you do have session anyway. :)

@emodric
Copy link
Contributor Author

emodric commented Dec 20, 2017

@Nattfarinn Well, not always. That layout template is also used as a layout for the login page, so just by displaying a login page, session would start if we were on lower versions of Symfony :)

EDIT: On the other hand, rendering the CSRF token in the login page does require the session, so it really doesn't matter after all :D

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

Successfully merging this pull request may close these issues.

5 participants