Skip to content

Conversation

@AlexandreGerault
Copy link
Contributor

@AlexandreGerault AlexandreGerault commented Mar 2, 2024

Hi 😄

I previously submitted a PR #50140 for typed configuration getters that have been approved and then enhanced by #50287.

I tried to make the same features for the session store as I believe it can help to have static analysis. Not sure though the way I wrote the tests.

Let me know if any changes needed (and also I didn't find a pint or lint command)

EDIT: Tests for type errors are not correct, changing to draft

@AlexandreGerault AlexandreGerault marked this pull request as draft March 2, 2024 10:11
@AlexandreGerault AlexandreGerault marked this pull request as ready for review March 2, 2024 10:41
@AlexandreGerault
Copy link
Contributor Author

I fixed the tests but I wonder if these could be more readable or more clean:

public function testSessionTypedGettersWithWrongTypes(): void
{
    $session = $this->getSession();
    $session->getHandler()->shouldReceive('read')->once()->with($this->getSessionId())->andReturn(serialize([
        'integer' => '12',
        'array' => null,
        'boolean' => 'true',
        'float' => '12.34',
        'string' => 12,
    ]));
    $session->start();

    $this->assertTypeError(fn () => $session->integer('integer'), 'integer');
    $this->assertTypeError(fn () => $session->array('array'), 'array');
    $this->assertTypeError(fn () => $session->boolean('boolean'), 'boolean');
    $this->assertTypeError(fn () => $session->float('float'), 'float');
    $this->assertTypeError(fn () => $session->string('string'), 'string');
}

private function assertTypeError(callable $callback, string $expectedType): void
{
    try {
        $callback();
        $this->fail("Expected TypeError for $expectedType type.");
    } catch (\TypeError $e) {
        $this->assertMatchesRegularExpression("/Session value for key \[(.*)\] must be (.*) {$expectedType}, (.*) given./", $e->getMessage());
    }
}

@taylorotwell
Copy link
Member

taylorotwell commented Mar 4, 2024

I really am not sure this needs to propagate throughout the entire framework. 😬 Where else might this also apply?

@AlexandreGerault
Copy link
Contributor Author

AlexandreGerault commented Mar 4, 2024

I don't know, I'm just figuring out when using the framework with static analysis (phpstan in my case)

I let the core team decide whether or not it has to be a framework feature. I guess if not it can still be done via the extend method of the container 😁(package ?)

@driesvints driesvints changed the title feat(session): add typed getters to session store [11.x] Add typed getters to session store Mar 4, 2024
@taylorotwell
Copy link
Member

Going to hold off on this for now I think.

@AlexandreGerault AlexandreGerault deleted the feature/typed-session-getters branch March 5, 2024 15:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants