Check for session before attempting to invalidate

[inmanturbo]

This is my first PR to a Laravel repo so please let me know if I'm doing anything incorrectly here.

All this pull request is doing is ensuring there is a session (with a single if statement) before attempting to destroy it in src/Http/Livewire/DeleteUserForm.php.

        if ($request->hasSession()) {
            $request->session()->invalidate();
            $request->session()->regenerateToken();
        }

Previously either of these two lines would break the TestableLivewire in stubs/test/livewire/DeleteAccountTest.php:24

         $request->session()->invalidate();
         $request->session()->regenerateToken();

This change will keep it from breaking tests in downstream apps that have had Jetstream installed with php artisan jetstream:install livewire.

This only affect cases where there is no $request->session() to invalidate().

Since we are simply skipping two lines of code run when there isn't a session, and doing nothing else differently when there is, this should not affect anything anywhere else, nor will it break anything.

This would close the issue I opened here:
#749

[telkins]

@taylorotwell @inmanturbo

I'm a little late to the party, but this may have code that @calebporzio does not recommend.

I just updated my composer dependencies and I now get an error when running Jetstream's Tests\Feature\DeleteAccountTest:

There was 1 error:

1) Tests\Feature\DeleteAccountTest::test_user_accounts_can_be_deleted
RuntimeException: Session store not set on request.

It seems like it's related to this: livewire/livewire#936

I'm running v2.3.1, which doesn't have the condition, if ($request->hasSession()) {, so it may not matter. (NOTE: When I wrap the two $request->session()... lines in that condition, then the test passes.) So, when the next release includes these changes, then my tests will pass, but there may still be an issue with how the session is accessed.

So...sorry for any alarm, but I thought I might raise the issue and see if the experts can better determine whether or not there's any reason to make any changes.

Thanks for your time/patience. 🤓

[inmanturbo]

@telkins thanks for sharing that issue! There may be something that needs to be fixed in livewire. In the meantime it may be worth reviewing #742 to see if session() or Session:: would be better, like in @calebporzio 's livewire/livewire#936 (comment), and eliminate the need for the if condition.

For the time being if you want to stop the spam coming from your CI, you can upgrade Jetstream to 2.x-dev.

[gpluess]

@telkins @inmanturbo

I just ran into the exact same error you're describing.

Replacing $request->session()... with session()... solved the issue for me. Tests are green again.

[driesvints]

I'm sorry but I fail to understand why Livewire would cause issues here?

[telkins]

I'm sorry but I fail to understand why Livewire would cause issues here?

Hi @driesvints 😄

I'm not sure that it has anything to do with livewire, exactly, but my tests are failing when running the following Jetstream/livewire test (Tests\Feature\DeleteAccountTest):

    public function test_user_accounts_can_be_deleted()
    {
        if (! Features::hasAccountDeletionFeatures()) {
            return $this->markTestSkipped('Account deletion is not enabled.');
        }

        $this->actingAs($user = User::factory()->create());

        $component = Livewire::test(DeleteUserForm::class)
                        ->set('password', 'password')
                        ->call('deleteUser');

        $this->assertNull($user->fresh());
    }

When trying to figure out what the problem might be, I came across the livewire issue, livewire/livewire#936, and followed @calebporzio 's advice (changing to session()) and the test passed.

That's the connection. Whether it has anything to do with livewire...I don't know. But, I thought I'd bring it up here to see what The Experts™️ had to say about it. My test is now failing and one person has suggested that it's better to use session() within livewire components (outside of mount()). Maybe that's good advice, maybe it's not, but it does seem to fix my failing test.

Also, @inmanturbo 's PR might make it all moot. I don't know. But, hopefully you and some of the others who have a better understanding of everything can figure out if there's even a cause for concern in the first place. If so, then maybe it's a livewire issue, maybe it's not...but we can cross that bridge if/when we get there. 🤓

[telkins]

@driesvints Just to save you a bit of time, here's the "money quote" from Caleb in the aforementioned Livewire issue:

I'm not sure why that is happening in the tests.

However, I wouldn't recommend relying on the request() within Livewire component methods other than mount().

I think session() and Session:: are hands-down the best way to access the session.

He doesn't say that $request->session() is bad, but when I replace $request->session() with session(), the failing test passes. 🤔

[driesvints]

Ah yeah, this PR exactly fixes that. I think Caleb is talking about a personal preference though. $request->session() should be the same as session(). In general my own preference is to rely on state passed down through DI instead of accessing it globally.

[telkins]

Ah yeah, this PR exactly fixes that. I think Caleb is talking about a personal preference though. $request->session() should be the same as session(). In general my own preference is to rely on state passed down through DI instead of accessing it globally.

Well, for me, $request->session() is not the same as session(). When I use the former, the test fails. When I use the latter, the test passes. If they were the same, then there would be no difference. This seems to be the same for @gpluess : #750 (comment)

Fwiw, I'm 100% with you that it's better "to rely on state passed down through DI instead of accessing it globally"....but these two options aren't the same for some reason, otherwise, we'd get the same results when running the test. 😕

Anyway...again, if the condition, if ($request->hasSession()) { prevents any problems with $request->session(), then there's likely little cause for alarm. (Btw, when I put the same condition in my code, then the test also passes.) So, if you guys take a closer look and believe there's no problem here, then that's good enough for me. 🤓

[inmanturbo]

@telkins I was maybe trying too hard to be helpful with the second PR. I agree that DI can be better; it is certainly easier (for me) to navigate during development. And yeah if ($request->hasSession()) is working fine.