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

Add modSessionHandlerInterface #15957

Closed
wants to merge 3 commits into from
Closed

Conversation

theboxer
Copy link
Member

@theboxer theboxer commented Jan 6, 2022

What does it do?

  • Creates a modSessionHandlerInterface interface and uses it for modSessionHandler
  • Moves flushSessions method from Security\Flush processor to the interface & handler
  • In the Security\Flush processor - checks if flushSessions method exists in the handler class stored in session_handler_class system setting and executes it if so

Why is it needed?

Custom session handlers can't flush all sessions.

How to test

#15928

Related issue(s)/PR(s)

Resolves #15928

@cla-bot cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Jan 6, 2022
@theboxer theboxer added this to the v3.1.0 milestone Jan 6, 2022
@Ibochkarev Ibochkarev added the pr/review-needed Pull request requires review and testing. label Jan 8, 2022
@JoshuaLuckers
Copy link
Contributor

@sergant210 can you check if this PR solves your issue?

@theboxer
Copy link
Member Author

theboxer commented Feb 1, 2022

btw. this is kind of duplicate of #15934 I didn't notice that PR sooner

@sergant210
Copy link
Collaborator

sergant210 commented Feb 18, 2022

@JoshuaLuckers My PR solves my issue ;)

@opengeek
Copy link
Member

My PR is solves my issue ;)

This was not the question.

@JoshuaLuckers
Copy link
Contributor

@theboxer can you explain the main difference between this PR and #15934 ? I don't have enough time to look into the details to decide which one we should approve and merge.

@Ibochkarev
Copy link
Collaborator

Up

@rthrash
Copy link
Member

rthrash commented Feb 2, 2023

This pull request has been mentioned on MODX Community. There might be relevant details there:

https://community.modx.com/t/error-flush-sessions-not-supported-when-trying-to-logout-all-users-in-modx-3/6361/2

@Mark-H
Copy link
Collaborator

Mark-H commented Feb 10, 2024

Closing in favour of #16522 - please see my explanation there.

@Mark-H Mark-H closed this Feb 10, 2024
opengeek pushed a commit that referenced this pull request Aug 15, 2024
)

### What does it do?

- Fixes the bugs from #15928, per #15934
- Refactored inititialization of the session handler and adopt the PHP
core SessionHandlerInterface, per #15934
- Move flushing sessions logic into the session handler to optionally
allow that to be extended, per #15957

### Why is it needed?

In two stale PRs #15928 and #15957 we have two proposals for dealing
with some bugs and extending session handlers. While trying to figure
out which one to use, I found both to have a solid approach, and wanting
to use one bit of one and another part of other PR.

This proposal replaces both those PRs as a middle ground, taking the
most benefit from both approachs.

### Breaking changes

This change includes a small breaking change in order to adopt the PHP
standard SessionHandlerInterface, namely adding parameters into the
open() method. There's not really any way around that in order to adopt
the standard.

As extending session handlers is a pretty advanced thing that I assume
not many people have done, I suggest we accept that for the sake of
getting closer to the standards, but make special note of this breaking
change in the docs for 3.1 and the announcement to make sure people
learn about it in case custom handlers need to be adjusted.

### How to test
Flush sessions

### Related issue(s)/PR(s)

Replaces #15934 and #15957
Fixes #15928

---------

Co-authored-by: sergant210 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed CLA confirmed for contributors to this PR. pr/review-needed Pull request requires review and testing.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Logout all users action doesn't work with custom session handlers
7 participants