- Issue created by @tim-diels
- @tim-diels opened merge request.
- Status changed to Needs review
almost 2 years ago 10:08am 30 January 2023 - 🇧🇪Belgium tim-diels Belgium 🇧🇪
Functionality added in MR. Would love to see a review and hear the feedback.
- Status changed to Needs work
almost 2 years ago 9:22pm 31 January 2023 - 🇬🇧United Kingdom philipnorton42 Cheshire
Hi tim-deils,
Thanks for taking an interest in session inspector!
Your changes make sense, but I'm spotting a couple of problems when I try to run things. I'll go through the bugs I found here, rather than add them to the merge request as comments.
1) Running the drush command without any parameters throws the following error.
```
$ drush session-inspector:delete-all-sessionsIn Tables.php line 369:
'root' not found
```This is because the command runs line 71 of the SessionInspectorCommands class with the $property of 'root' and the $value being the path of my Drupal installation.
This line:
```
$accounts = $this->userStorage->loadByProperties([$property => $value]);
```I think there's more in the $options variable than you were expecting and that's causing some problems with the loadByProperties() method.
I'm running Drush 11.4.0 if that helps?
2) No session data is found
When looking at the session page for a user it displays no information. This is because the getSessions() method now uses an array, but the use of array_keys() within this method means that the array must be indexed when being passed in.
I changed the code to be this and it fixed the problem.
```
$sessions = $this->sessionInspector->getSessions([$user->id() => $user]);
```I think, however, that it might be better to have a getSessionForUser() and getAllSessions() methods so that they can be treated differently and not have to code a single method to adapt to both situations.
3) Protection on the drush command
The drush command to delete everyones session needs to have a "are you sure?" check on it. Might help prevent mistakes with such a destructive command.
One of my clients has over a million users and things would get pretty heated if I accidentally logged everyone out! :)
4) Unit tests
It would be great to have some unit tests for the command. I created the following that checks that a user's session is killed off when the drush command is run.
```
<?phpnamespace Drupal\Tests\session_inspector\Functional;
use Drupal\Tests\BrowserTestBase;
use Drush\TestTraits\DrushTestTrait;/**
* Test the functionality of the session inspector module drush commands.
*
* @group session_inspector
*/
class SessionInspectorCommandsTest extends BrowserTestBase {use DrushTestTrait;
/**
* Modules to enable.
*
* @var array
*/
protected static $modules = [
'session_inspector',
];/**
* The theme to install as the default for testing.
*
* @var string
*/
public $defaultTheme = 'stark';/**
* Test that a user is logged out via drush session .
*/
public function testDrushClearSession() {
$user = $this->createUser(['inspect own user sessions']);
$this->drupalLogin($user);$this->drush('session-inspector:delete-all-sessions');
$this->drupalGet('user/' . $user->id() . '/sessions');
$this->assertSession()->responseContains('Access denied');
}}
```Due to the problems discussed above I haven't actually managed to get this test to run correctly, but it should help you spot problems on your end.
Thanks again for taking the time to make these changes. I really appreciate it.
Let me know if you have any questions.
Phil - Status changed to Closed: won't fix
about 1 year ago 12:28pm 24 December 2023 - 🇬🇧United Kingdom philipnorton42 Cheshire
Just clearing out some old issues for this module. I'm happy to discuss adding this feature if you really want it, but for the time being I'm closing this issue.