Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

[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

Closed
VincentLanglet wants to merge7 commits intosymfony:7.2fromVincentLanglet:stateless

Conversation

@VincentLanglet
Copy link
Contributor

@VincentLangletVincentLanglet commentedJul 27, 2024
edited
Loading

QA
Branch?7.2
Bug fix?no
New feature?no
Deprecations?no
IssuesRelated to#57502 but does not fix it
LicenseMIT

I think asking the user to check everywhere in his code for

$request->attributes->get('_stateless')

before accessinggetSession is kinda verbose and rely on some internal knowledge about how symfony describe the request as stateless.

It's easier for the developer to write

if (!$request->isStateless() && $request->hasSession()) {

rather than having to know the "internal attribute" to check

if (!$request->attributes->getBoolean('_stateless') && $request->hasSession()) {

So I propose to introduce this method as a facade to the internal attribute.

jdreesen reacted with thumbs up emoji
@carsonbotcarsonbot added this to the7.2 milestoneJul 27, 2024
@carsonbotcarsonbot changed the titleIntroduce Request::isStateless method[HttpFoundation] Introduce Request::isStateless methodJul 27, 2024
@VincentLangletVincentLangletforce-pushed thestateless branch 3 times, most recently from53e5d30 to5719dcbCompareJuly 27, 2024 19:15
@OskarStarkOskarStark changed the title[HttpFoundation] Introduce Request::isStateless method[HttpFoundation] AddRequest::isStateless methodJul 28, 2024
Copy link
Member

@xabbuhxabbuh left a 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.

@VincentLanglet
Copy link
ContributorAuthor

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_stateless logic, we have to provide a way to know

  • When the stateless attribute is set to false/true
  • When the stateless attribute is not set (thehas check)

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!$request->isStateless() since null and false behave the same way, but this allow to checknull !== $request->isStateless() to reproduce the check$request->attributes->has('_stateless').

WDYT ?

@stof
Copy link
Member

havingisStateless returnbool|null looks weird to me as the name indicate a boolean.

Code wanting to guard session reads will only need a boolean anyway (as they don't need to distinguish the default case).
Special places needing to distinguish a request marked explicitly as stateful vs the default case (no explicit configuration) can check the attribute directly (and I doubt we will have lots of such use cases outside the core)

@VincentLanglet
Copy link
ContributorAuthor

havingisStateless returnbool|null looks weird to me as the name indicate a boolean.

Code wanting to guard session reads will only need a boolean anyway (as they don't need to distinguish the default case). Special places needing to distinguish a request marked explicitly as stateful vs the default case (no explicit configuration) can check the attribute directly (and I doubt we will have lots of such use cases outside the core)

I reverted this ina17bfcd then@stof

@nicolas-grekas
Copy link
Member

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
Copy link
ContributorAuthor

I feel like this builds on a wrong definition of the _stateless attribute.

I'm not sure to understand where is the wrong definition

Well it all boils down if you considerstateless as a check or as a directive.

The blog post which introduced it reads

Routes can now configure astateless boolean option. If set totrue, they declare that session won't be used during the handling of the request.

Note that it is writtendeclare, notforce.

If it doesn't force the session to be stateless, then this check shouldn't exists

if ($this->debug) {
thrownewUnexpectedSessionUsageException('Session was used while the request was declared stateless.');
}
if ($this->container->has('logger')) {
$this->container->get('logger')->warning('Session was used while the request was declared stateless.');
}

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
Copy link
Contributor

MatTheCat commentedAug 14, 2024
edited
Loading

Isn'tisStateless a misleading name? What it returns is if the request issupposed to be stateless; if it has been configured as such.

@VincentLanglet
Copy link
ContributorAuthor

VincentLanglet commentedAug 14, 2024
edited
Loading

Isn'tisStateless a misleading name? What it returns is if the request issupposed to be stateless; if it has been configured as such.

Do you have a suggestion@MatTheCat ? "ShouldBeStateless" ?

I understandisStateless as "is declared Stateless", which is ok with your definition here:#57854 (comment)

@MatTheCat
Copy link
Contributor

shouldBeStateless sounds good to me indeed.

@VincentLanglet
Copy link
ContributorAuthor

VincentLanglet commentedAug 14, 2024
edited
Loading

shouldBeStateless sounds good to me indeed.

Currently there is already someFirewallConfig::isStateless method. This one is not misleading ?

AndsetStateless is ok for the setter ?

@MatTheCat
Copy link
Contributor

MatTheCat commentedAug 15, 2024
edited
Loading

Currently there is already someFirewallConfig::isStateless method. This one is not misleading ?

FirewallConfig::isStateless returnstrue if you configured the firewall stateless. Why would that be misleading?

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 thinkRequest::isStateless is not a good name.

AndsetStateless is ok for the setter ?

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
Copy link
ContributorAuthor

Currently there is already someFirewallConfig::isStateless method. This one is not misleading ?

FirewallConfig::isStateless returnstrue if you configured the firewall stateless. Why would that be misleading?

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 thinkRequest::isStateless is not a good name.

IMHO part of my misunderstanding come from the fact thatFirewallConfig::isStateless imply the fact that request is flagged as stateless. Reverting this behavior, as asked in#50715 (comment) would solve all my issues and avoid the need of this method.

AndsetStateless is ok for the setter ?

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)? 🤔

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
Copy link
Member

nicolas-grekas commentedAug 16, 2024
edited
Loading

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
Copy link
Member

Symfony does not enforce requests to be fully stateless soisStateless would potentially bring false impressions, and I don’t get the need forshouldBeStateless() in userland.
Let’s close. Thanks for proposing@VincentLanglet.

@VincentLanglet
Copy link
ContributorAuthor

Symfony does not enforce requests to be fully stateless soisStateless would potentially bring false impressions, and I don’t get the need forshouldBeStateless() in userland. Let’s close. Thanks for proposing@VincentLanglet.

Yes, and personally I never used stateless request, I just needed a check because the request was declared stateless when I use stateless firewall...
Since#58017 is merged, I won't need to check it again.

@derrabus
Copy link
Member

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
Copy link
ContributorAuthor

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?

Do you think the discussion could be done in the issue#57502@derrabus or it would require a different one ?

derrabus reacted with thumbs up emoji

@94noni
Copy link
Contributor

reading PRs/issues/discussions on such request stateless topic, we can see the "misleading/misusage" of such point
same as you on this

Maybe we should start over with an issue where we could discuss the topic?

I do think this is a good starting point yes (or use existing one?)

@derrabus
Copy link
Member

Do you think the discussion could be done in the issue#57502@derrabus or it would require a different one ?

I missed that one. Of course, let's continue the discussion there.

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

Reviewers

@chalasrchalasrAwaiting requested review from chalasrchalasr is a code owner

@xabbuhxabbuhAwaiting requested review from xabbuh

@stofstofAwaiting requested review from stof

@fabpotfabpotAwaiting requested review from fabpot

1 more reviewer

@94noni94noni94noni approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Projects

None yet

Milestone

7.2

Development

Successfully merging this pull request may close these issues.

11 participants

@VincentLanglet@stof@nicolas-grekas@MatTheCat@chalasr@derrabus@94noni@fabpot@xabbuh@OskarStark@carsonbot

[8]ページ先頭

©2009-2025 Movatter.jp