Initial commit of RFC alpha implementation.#1
Conversation
Signed-off-by: Joey Smith <jsmith@webinertia.net>
| namespace PhpDb\Session\Container; | ||
|
|
||
| use PhpDb\Adapter\AdapterInterface; | ||
| use PhpDb\Session\DbSessionHandler; |
There was a problem hiding this comment.
Why is PhpDb taking responsibility for sessions? Would that not be more in the realm of an adapter?
There was a problem hiding this comment.
Laminas is removing "save handlers" for laminas db from the session packages. Mezzio still technically owns Sessions, mezzio-session exposes the contracts that phpdb-mezzio-session is built upon. This is the exact use case described in the mezzio-session docs.
|
|
||
| return PhpDbSessionPersistence::fromConfigArray( | ||
| $container->get(AdapterInterface::class), | ||
| isset($config['session']) && is_array($config['session']) ? $config['session'] : [] |
There was a problem hiding this comment.
It's a bit late, here, so bear with me, but would a null coalescing operator simplify this logic?
There was a problem hiding this comment.
Probably :) This is literally the first iteration of the code.
| 'INSERT INTO %1$s (%2$s) VALUES (%3$s)' | ||
| . ' ON DUPLICATE KEY UPDATE' | ||
| . ' payload = VALUES(payload)' | ||
| . ', expires_at = VALUES(expires_at)' | ||
| . ', modified_at = VALUES(modified_at)', |
There was a problem hiding this comment.
What about using a HEREDOC string instead for slightly greater readability? Also, if you're using MySQL-specific syntax, would this break in any of the other supported databases?
There was a problem hiding this comment.
Yes, I'm aware that it will break. That is the primary purpose of this RFC. So, we can decide how best to abstract it for all drivers.
There was a problem hiding this comment.
I'll add a PR to allow Upsert as a new PhpDb\Sql class and then add PR to each driver to decorate it as required. This will allow a more generic call here.
settermjd
left a comment
settermjd
left a comment
There was a problem hiding this comment.
From my first run through, it seems to be fine. I've left a few comments for your consideration. I hope they're of some help.
|
Oh, they'll help for sure 😁 |
| // PhpSessionPersistenceDecorator is available for sync-only environments | ||
| // but is NOT aliased here. Swap the alias below to use it instead. | ||
| SessionPersistenceInterface::class => PhpDbSessionPersistence::class, | ||
| ], |
There was a problem hiding this comment.
Any plans for short 'key' aliases here? 'db' for example?
There was a problem hiding this comment.
The sync-only stuff is actually just a left-over comment. It's not relevant anymore as it has been dropped.
But, generally speaking. No. I despise string keys because they provide today's tooling no further information. IE, no deprecation notices etc.
|
|
||
| public function __construct(AdapterInterface $adapter) | ||
| { | ||
| $this->sql = new Sql($adapter, 'session'); |
There was a problem hiding this comment.
I know this is an alpha but I'd like to see a config passed through with 'session' as the default table name string.
There was a problem hiding this comment.
Yea, I'll get around to it in the next revision or two. This is literally just what I built for personal use in a current project so...
| public function read(string $id): string|false | ||
| { | ||
| $select = $this->sql->select() | ||
| ->columns(['payload']) |
There was a problem hiding this comment.
Same here - the sane 'payload' column as default but also configurable.
| { | ||
| $select = $this->sql->select() | ||
| ->columns(['payload']) | ||
| ->where(['id' => $id]); |
There was a problem hiding this comment.
And the PK could also be session_id etc...
| $now = date('Y-m-d H:i:s'); | ||
|
|
||
| $this->sql->prepareStatementForSqlObject( | ||
| (new Insert('session')) |
| } | ||
|
|
||
| // No ID means a new session was created but never written to. | ||
| if ($id === '') { |
There was a problem hiding this comment.
I reckon an $id should be NULL instead of an empty string if it's not yet available
3 participants
Signed-off-by: Joey Smith jsmith@webinertia.net
Description
This PR is exploratory. This is a base working implementation based around phpdb-mysql for discussion on how to best abstract for all drivers.