Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork9.7k
[HttpFoundation] AddRequest::isStateless method#57852
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
53e5d30 to5719dcbCompareUh oh!
There was an error while loading.Please reload this page.
Request::isStateless method
xabbuh 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.
The new method should also be documented in the changelog file of the HttpFoundation component.
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
VincentLanglet commentedJul 29, 2024
I tried to resolve the suggestion from@fabpot (#57852 (comment)) in the latest commitaa9e241 If we want to have only one place to deal with
This may be done by having a nullable boolean as return type of the isStateless method. That doesn't change a lot to the check WDYT ? |
stof commentedJul 29, 2024
having Code wanting to guard session reads will only need a boolean anyway (as they don't need to distinguish the default case). |
Uh oh!
There was an error while loading.Please reload this page.
VincentLanglet commentedJul 29, 2024
|
nicolas-grekas commentedAug 13, 2024
After my comment in#57854 (comment), I'm wondering if this is common enough to deserve a dedicated method. I feel like this builds on a wrong definition of the _stateless attribute. |
VincentLanglet commentedAug 13, 2024
I'm not sure to understand where is the wrong definition
If it doesn't force the session to be stateless, then this check shouldn't exists symfony/src/Symfony/Component/HttpKernel/EventListener/AbstractSessionListener.php Lines 213 to 219 inc852b8b
And even if it's still a declaration, a way to know if the request is stateless should exists (in order to know if the session is useful/usable or not) and therefore a method would be better than having to know which private attribute is used. |
MatTheCat commentedAug 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Isn't |
VincentLanglet commentedAug 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Do you have a suggestion@MatTheCat ? "ShouldBeStateless" ? I understand |
MatTheCat commentedAug 14, 2024
|
VincentLanglet commentedAug 14, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
Currently there is already some And |
MatTheCat commentedAug 15, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
When you configure a route stateless, you’re asking Symfony to warn you if the session is used. This does not offerany guarantee regarding the fact the session is used or not. As such, you cannot use this information to tell if the request is stateless or not. That’s why I think
Do you have use-cases in mind where a setter would make sense (if yes the name would be wrong the same way it is wrong for the getter)? 🤔 |
VincentLanglet commentedAug 15, 2024
IMHO part of my misunderstanding come from the fact that
No, it was just to be used in Symfony code (in order to avoid having to know the private attribute when setting it, but using the getter when accessing it). |
nicolas-grekas commentedAug 16, 2024 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
I'd rather be 👎 here, reading the attribute is fine, and the setter doesn't really make sense to me. This gives too much visibility to the setting IMHO. |
chalasr commentedAug 16, 2024
Symfony does not enforce requests to be fully stateless so |
VincentLanglet commentedAug 16, 2024
Yes, and personally I never used stateless request, I just needed a check because the request was declared stateless when I use stateless firewall... |
derrabus commentedAug 16, 2024
I have worked on several applications that made unnecessarily heavy use of the session before the decision was made to transition towards a RESTful API. It has always been hard to trace down where the stateless request processing is "tainted" by an unexpected session access. I think, Symfony could implement tooling to help with such a transition. Maybe this PR wasn't the right way (and the now-reverted logic for auto-flagging requests in stateless firewalls wasn't either), but I think we should continue talking about how we could tackle stateful/stateless requests better. Maybe we should start over with an issue where we could discuss the topic? |
VincentLanglet commentedAug 16, 2024
Do you think the discussion could be done in the issue#57502@derrabus or it would require a different one ? |
94noni commentedAug 16, 2024
reading PRs/issues/discussions on such request stateless topic, we can see the "misleading/misusage" of such point
I do think this is a good starting point yes (or use existing one?) |
Uh oh!
There was an error while loading.Please reload this page.
I think asking the user to check everywhere in his code for
before accessing
getSessionis kinda verbose and rely on some internal knowledge about how symfony describe the request as stateless.It's easier for the developer to write
rather than having to know the "internal attribute" to check
So I propose to introduce this method as a facade to the internal attribute.