Closed (fixed)
Project:
Session Inspector
Version:
1.0.0
Component:
User interface
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
31 Mar 2022 at 17:55 UTC
Updated:
6 Apr 2022 at 18:19 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
philipnorton42 commentedAn excellent idea.
I think the "Delete other sessions" might be the most useful. Otherwise the button would be more of an annoyance.
I'll see what I can do :)
Comment #3
larisse commentedHi! I created this patch with the comment #2 suggestion...
I think will need some adjusts in the delete page, but I will put the patch here to know if this is the way...
Feedback are welcomed :)
Comment #4
philipnorton42 commentedHi larisse,
Firstly, thank you so much for taking the time to contribute. I really appreciate your patch!
That being said, I think you've missed the brief slightly. You code does delete the sessions, but it deletes the sessions for all users on the site, essentially logging everyone out of the site at the same time.
What was required was a way to allow a user to delete all sessions that belong to them (that aren't their current session). On other words, the path of the form would be "/user/{user}/sessions/{sid}/delete-all" and not "/admin/config/people/session_inspector/delete".
The code you have written does have some good ideas. I think a service to delete sessions is a good idea, instead of adding deletion code into the session inspector service (like I have done). Would you mind if continue that work and implement suggestion #2?
Comment #5
philipnorton42 commentedSorry, I'm talking nonsense. The code does work, but as it loops through every user on the site it's not very performant.
Comment #6
larisse commentedOh, sorry! Yes, I missed the brief slightly.
I can continue work on it, of course! =)
But now I'm a little lost... Can I remove the wrong thing that I've done, once that is not very performant? Because I not able to think another way to do this, if you want this feature too.
Comment #7
philipnorton42 commentedHi larisse,
No worries! What you've written is a great start :)
I've been thinking about this and I think there might be a couple of ways to cut this.
Firstly, the destroySession() method should move from the session_inspector service should move into the deletion service.
Then, it's just a case of doing either:
1) Figure out the sessions that need deleting first, and then send that to the deleteAllSessions() method, like this:
2) Just pass the user to the deleteAllSessions() method, and the method figures out everything else. The isCurrentSession() method is currently part of the controller so it would need to be refactored into the session_inspector service. The code would look something like this.
3) Something else?...
Open to suggestions. I wrote the original code to do the job it needed to do. Now that we are introducing additional functionality it makes sense to refactor it a little.
Also, the session delete form triggers an event so that other modules can react to the session deletion. It doesn't make sense to trigger that from the form when we are doing this. It would therefore need to be moved to the either the deleteAllSessions() or destroySession() methods.
Let me know what you think.
Comment #8
larisse commentedOh, ok, ok! Yeah, the second suggestion make more sense to mee.
I'll work on this and create another patch. For while, I don't have any other suggestion, I'll find while I code, I guess...
Thank you for support!.
Comment #9
larisse commentedHi @philipnorton42. Here's a new patch. I think now I was able to implement the correct feature request.
I implemented the suggest 2 in comment #7. I just needed made some adjust in the code. To put the new link on the table, I don't have sure that I made the better way, so feel free to adjust this or anything that you think will be necessary. :)
Comment #10
philipnorton42 commentedThank you so much @larisse.
I just tested your code and it works! I really appreciate you taking the time to do this. You've done almost all of the things I was planning to do here, including moving the isCurrentSession() method into the session_inspector service. Great work!
There are a few coding style issues being reported by phpcs, but I'm happy to sort those out from here if that's ok with you? I'll make sure you get the commit credit for this.
Comment #11
larisse commentedHi @philipnorton42!
Sorry for the phpcs problems! Feel free to fix what was necessary.
Was very cool and I learned to much doing this new feature. I was happy to be able to contribute. :)
Comment #16
philipnorton42 commentedComment #17
philipnorton42 commentedThis fix has been merged into release 1.0.1 (https://www.drupal.org/project/session_inspector/releases/1.0.1).
Thanks again cmarcera and larisse!