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.

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

[FrameworkBundle][HttpKernel] Add session usage reporting in stateless mode #35732

Merged
merged 1 commit into from
Feb 26, 2020
Merged

[FrameworkBundle][HttpKernel] Add session usage reporting in stateless mode #35732

merged 1 commit into from
Feb 26, 2020
+120 −15

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Feb 15, 2020 •
edited
Loading

Q A
Branch? master
Bug fix? no
New feature? yes
Deprecations? no
License MIT
Doc PR symfony/symfony-docs#13344

https://github.com/orgs/symfony/projects/1#card-30506005

Provide a stateless attribute that forbid session usage.

Implementations

v1

  • New session proxy that allows session to be marked as disabled
  • New default route attribute: _stateless (automatically set by @Stateless)
  • On kernel controller event, if _stateless is true, session is marked as disabled
  • Session listener is able to check if the session is disabled and prevent its creation

v2

  • New default route attribute: _stateless (automatically set by @Stateless)
  • On kernel response, check the session usage and if session was used when _stateless attribute is set to true: Either throw an exception (debug enabled) or log a warning (debug disabled)

v3

  • New route default _stateless (automatically set by stateless route attribute)
  • On kernel response, check the session usage and if session was used when _stateless attribute is set to true: Either throw an exception (debug enabled) or log a warning (debug disabled)

@mtarld
Copy link
Contributor Author

mtarld commented Feb 15, 2020

One thing I'm wondering, should we need the DisableableSessionProxy or could we use directly Session ?

@nicolas-grekas
Copy link
Member

The request attribute is a nice idea.
For the implementation, I'd like to propose another approach: the AbstractSessionListener could check the session usage index and report back when a stateless route increments the index, wdyt?
That would not be enough to know where the session was used, but that could be improved separately when resolving https://github.com/orgs/symfony/projects/1#card-30499423

@mtarld
Copy link
Contributor Author

mtarld commented Feb 15, 2020

Yes it could be great, I'll give it a try

@mtarld
Copy link
Contributor Author

mtarld commented Feb 16, 2020

I had tried this approach but something came up to me.
Using this implementation, I think that we won't be able to prevent session start and usage as we are just reporting back. Therefore maybe session data could have been accessed or updated even if we report back session usage.
Do you see ?
But maybe I didn't get fully what you wanted to mean
WDYT ?

@chalasr chalasr added this to the next milestone Feb 16, 2020
@mtarld
Copy link
Contributor Author

mtarld commented Feb 18, 2020

@nicolas-grekas I updated code according to your approach.
Indeed the code is much cleaner, thanks !

But what should we do when session is used in a stateless mode ? Should we throw an exception ? Just log it ? Or even make that behavior configurable ?

@nicolas-grekas
Copy link
Member

Throw when no logger is passed, log when one is - and pass one when kernel.debug=false in the DI extension?

@mtarld
Copy link
Contributor Author

mtarld commented Feb 18, 2020

Ok, so now if debug is enabled, this will throw an exception. If not, it'll log a warning.

nicolas-grekas
nicolas-grekas reviewed Feb 18, 2020
View reviewed changes
Copy link
Member

@nicolas-grekas nicolas-grekas left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM, don't miss adding some tests and a CHANGELOG entry.

@mtarld mtarld force-pushed the feature/stateless branch 6 times, most recently from f5d6f6c to b8e83bb Compare February 18, 2020 17:46
@mtarld mtarld changed the title [FrameworkBundle][HttpFoundation] Add @Stateless annotation [FrameworkBundle][HttpFoundation] Add session usage reporting in stateless mode Feb 19, 2020
@mtarld mtarld mentioned this pull request Feb 19, 2020
Merged
@mtarld mtarld force-pushed the feature/stateless branch 2 times, most recently from 9a9e4dd to 85a6a10 Compare February 25, 2020 14:32
@nicolas-grekas nicolas-grekas changed the title [FrameworkBundle][HttpFoundation] Add session usage reporting in stateless mode [FrameworkBundle][HttpKernel] Add session usage reporting in stateless mode Feb 26, 2020
@fabpot
Copy link
Member

fabpot commented Feb 26, 2020

Thank you @mtarld.

@fabpot fabpot mentioned this pull request Feb 26, 2020
Closed
@fabpot fabpot merged commit 7995fed into symfony:master Feb 26, 2020
@mtarld mtarld deleted the feature/stateless branch February 26, 2020 10:41
fabpot added a commit that referenced this pull request Feb 29, 2020
@fabpot
feature #35782 [Routing] Add stateless route attribute (mtarld)
146945a 
This PR was merged into the 5.1-dev branch.

Discussion
----------

[Routing] Add stateless route attribute

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Ticket		| https://github.com/orgs/symfony/projects/1#card-30506005
| License       | MIT
| Doc PR        | TODO

On top of #35732

Add a stateless attribute for:
Routes in annotations
```
@route(stateless=true)
```
Yaml
```yml
route:
  stateless: true
```
Xml
```xml
<route stateless="true" />
```
PHP configurator
```php
$route->stateless(true);
```

That stateless attribute is a shortcut for setting `_stateless` default attribute in route.

Commits
-------

2da68ba [Routing] Add stateless route attribute
@mtarld mtarld mentioned this pull request Mar 9, 2020
Merged
@mtarld mtarld mentioned this pull request Apr 2, 2020
Closed
@nicolas-grekas nicolas-grekas modified the milestones: next, 5.1 May 4, 2020
@fabpot fabpot mentioned this pull request May 5, 2020
Merged
@gndk gndk mentioned this pull request Jul 12, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@derrabus derrabus

@fabpot fabpot

@nicolas-grekas nicolas-grekas

Assignees
No one assigned
Labels
Feature FrameworkBundle HttpFoundation Status: Reviewed
Projects
None yet
Milestone
5.1
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mtarld @nicolas-grekas @fabpot @derrabus @chalasr @carsonbot