Skip to content

NEW Add additional session handler implementations#11783

Merged
emteknetnz merged 4 commits intosilverstripe:6from
creative-commoners:pulls/6/session-handlers
Jul 30, 2025
Merged

NEW Add additional session handler implementations#11783
emteknetnz merged 4 commits intosilverstripe:6from
creative-commoners:pulls/6/session-handlers

Conversation

@GuySartorelli
Copy link
Copy Markdown
Member

@GuySartorelli GuySartorelli commented Jul 14, 2025

Doing this in several commits - DO NOT SQUASH

Commit 1: The cache-based session handler that can be used for Redis and Memcached
Commit 2: The database session handler
Commit 3: Add new environment variable to set the save handler
Commit 4: Injector allows backticks in the factory key now. This isn't a breaking change, because using backticks for that before would be trying to get a class name that has backticks in it, which PHP doesn't allow.

Issue

@GuySartorelli GuySartorelli marked this pull request as draft July 14, 2025 05:57
@GuySartorelli GuySartorelli mentioned this pull request Jul 14, 2025
5 tasks
@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch from fa09ce0 to 2a27e43 Compare July 15, 2025 01:18
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implementing the session handler for cache adapters as a single abstracted handler ended up being simpler than if I tried to implement Redis and Memcached handlers separately - with the bonus that it's also easier to test since we can use Symfony's ArrayAdapter.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The session lifetime is the same across all session handlers so it made sense to move into its own abstract class (pulled from FileSessionHandler).

@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch from 2a27e43 to 644e9c3 Compare July 15, 2025 01:46
/**
* @dataProvider provideDefaultSort
*/
#[DataProvider('provideDefaultSort')]
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated MNT fix. Noticed a warning about this in the test run so I figured I may as well just get it fixed.


ini_set('session.gc_maxlifetime', $gcLifetime);
Session::config()->set('lifetime', $configLifetime);
Session::config()->set('timeout', $configLifetime);
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and the implementation in FileSessionHandler were looking at the wrong config name.

Comment on lines +215 to +224
private function isDatabaseReady()
{
if (!DB::connection_attempted() || !DB::is_active()) {
return false;
}
return DB::get_schema()->hasTable(static::config()->get('table_name'));
}
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was originally going to use the new DataObjectSchema::tablesAreReadyForClass() method, but since we aren't using a DataObject (see phpdoc comment for requireTable() for reasons why not) that's not suitable.
Ultimately since the schema for this table isn't configurable, the table will either exist and have the correct schema, or not exist.

@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch 2 times, most recently from b9884bf to b034bcb Compare July 22, 2025 00:55
use SilverStripe\Core\Config\Config;
use SilverStripe\Core\Injector\Injector;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\Attributes\PreserveGlobalState;
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noticed this use statement was missing... I have no idea why it wasn't causing CI failures but this class is referenced in some of the tests higher up in this class.

@GuySartorelli GuySartorelli marked this pull request as ready for review July 22, 2025 01:03
Comment thread src/Control/SessionHandler/CacheSessionHandler.php Outdated
Comment thread _config/session.yml Outdated
Comment thread src/Control/SessionHandler/DatabaseSessionHandler.php
@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch 3 times, most recently from 6716e3d to f5e2054 Compare July 23, 2025 01:32
Comment thread src/Control/SessionHandler/CacheSessionHandler.php Outdated
Comment thread tests/php/Control/SessionHandler/CacheSessionHandlerTest.php Outdated
Comment thread tests/php/Control/SessionHandler/CacheSessionHandlerTest.php Outdated
Comment thread tests/php/Control/SessionHandler/CacheSessionHandlerTest.php Outdated
Comment thread tests/php/Control/SessionHandler/CacheSessionHandlerTest.php Outdated
Comment thread src/Control/SessionHandler/DbBuildSessionExtension.php Outdated
Comment thread src/Control/SessionHandler/DbBuildSessionExtension.php Outdated
Comment thread src/Control/SessionHandler/DatabaseSessionHandler.php Outdated
Comment thread src/Control/SessionHandler/DatabaseSessionHandler.php Outdated
Comment thread _config/session.yml
@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch from f5e2054 to a51be7e Compare July 23, 2025 03:36
public function testGc(int $gcLifetime, int $configLifetime, array $sessionLifetimeMap, array $expectDeleted): void
{
$tableName = DatabaseSessionHandler::config()->get('table_name');
ini_set('session.gc_maxlifetime', $gcLifetime);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this be reset after the test?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it particularly matters (sessions aren't used in CI anyway and this doesn't actually set anything permanently, only for the current process) - but have reset anyway for this and FileSessionHandlerTest to avoid further ping pong.


private function getSessionHandler(): ?DatabaseSessionHandler
{
$sessionHandlerServiceName = Session::getSaveHandler();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Session::getSaveHandler() returns an object, not a service name?

Probably means we don't need the $sessionHandler = Injector::inst()->get($sessionHandlerServiceName); below?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops, I must have had an earlier implementation of getSaveHandler() and not updated this or something. Fixed.

@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch from a51be7e to f0bf1d4 Compare July 23, 2025 22:55
@GuySartorelli
Copy link
Copy Markdown
Member Author

The CI failures are unrelated - compare with the 6 branch

Copy link
Copy Markdown
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I set my .env to

SS_SESSION_SAVE_HANDLER_CLASS="SilverStripe\Control\SessionHandler\DatabaseSessionHandler"

and I ran dev/build flush

I got this on the login form when I went to /admin (which then redirects to /Security/login?BackURL=%2Fadmin%2Fpages)

         // Session start emits a cookie, but only if there's no existing session. If there is a session timeout
336                 // tied to this request, make sure the session is held for the entire timeout by refreshing the cookie age.
337                 if ($cookieParams['lifetime'] && $this->requestContainsSessionId()) {
338                     Cookie::set(
339                         session_name(),

Trace

    session_start()
    Session.php:333
    SilverStripe\Control\Session->start(SilverStripe\Control\HTTPRequest)
    Session.php:252
    SilverStripe\Control\Session->init(SilverStripe\Control\HTTPRequest)
    SessionMiddleware.php:17
    SilverStripe\Control\Middleware\SessionMiddleware->process(SilverStripe\Control\HTTPRequest, Closure)
    HTTPMiddlewareAware.php:62
    SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure}(SilverStripe\Control\HTTPRequest)
    AllowedHostsMiddleware.php:71
    SilverStripe\Control\Middleware\AllowedHostsMiddleware->process(SilverStripe\Control\HTTPRequest, Closure)
    HTTPMiddlewareAware.php:62
    SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure}(SilverStripe\Control\HTTPRequest)
    TrustedProxyMiddleware.php:179
    SilverStripe\Control\Middleware\TrustedProxyMiddleware->process(SilverStripe\Control\HTTPRequest, Closure)
    HTTPMiddlewareAware.php:62
    SilverStripe\Control\Director->SilverStripe\Control\Middleware\{closure}(SilverStripe\Control\HTTPRequest)
    HTTPMiddlewareAware.php:65
    SilverStripe\Control\Director->callMiddleware(SilverStripe\Control\HTTPRequest, Closure)
    Director.php:382
    SilverStripe\Control\Director->handleRequest(SilverStripe\Control\HTTPRequest)
    HTTPApplication.php:114
    SilverStripe\Control\HTTPApplication::SilverStripe\Control\{closure}(SilverStripe\Control\HTTPRequest)
    call_user_func(Closure, SilverStripe\Control\HTTPRequest)
    HTTPApplication.php:137
    SilverStripe\Control\HTTPApplication->SilverStripe\Control\{closure}(SilverStripe\Control\HTTPRequest)
    HTTPMiddlewareAware.php:65
    SilverStripe\Control\HTTPApplication->callMiddleware(SilverStripe\Control\HTTPRequest, Closure)
    HTTPApplication.php:130
    SilverStripe\Control\HTTPApplication->execute(SilverStripe\Control\HTTPRequest, Closure, )
    HTTPApplication.php:113
    SilverStripe\Control\HTTPApplication->handle(SilverStripe\Control\HTTPRequest)
    index.php:24

@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch from f0bf1d4 to f7c4336 Compare July 24, 2025 20:58
@GuySartorelli
Copy link
Copy Markdown
Member Author

GuySartorelli commented Jul 24, 2025

Huh. For some reason DB::connection_attempted() was returning false even though we had a connection.
Fixed now by removing that call. The call to DB::is_active() will still prevent us from asking the db if there's a table before we've made a db connection.

Copy link
Copy Markdown
Member

@emteknetnz emteknetnz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm getting the following error on dev/build when I attempt to use memcached

Note there's a good chance one of my setup steps is incorrect. I've never used memcached before

In InjectionCreator.php line 20:
                                                          
  [SilverStripe\Core\Injector\InjectorNotFoundException]  
  Class `SS_SESSION_CACHE_FACTORY` does not exist         
# .env
SS_SESSION_SAVE_HANDLER_CLASS="SilverStripe\Control\SessionHandler\CacheSessionHandler"
SS_SESSION_CACHE_FACTORY="SilverStripe\Core\Cache\MemcachedCacheFactory"
SS_MEMCACHED_DSN="memcached://localhost:11211"

I setup memcached in my docker container as follows, as ssh'd in as root user

apt update && apt install -y memcached php8.3-memcached
memcached -d -m 64 -p 11211 -u memcache
Exception trace:
  at /var/www/vendor/silverstripe/framework/src/Core/Injector/InjectionCreator.php:20
 SilverStripe\Core\Injector\InjectionCreator->create() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:631
 SilverStripe\Core\Injector\Injector->instantiate() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:1027
 SilverStripe\Core\Injector\Injector->getNamedService() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:979
 SilverStripe\Core\Injector\Injector->get() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:623
 SilverStripe\Core\Injector\Injector->instantiate() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:1027
 SilverStripe\Core\Injector\Injector->getNamedService() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:979
 SilverStripe\Core\Injector\Injector->get() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:521
 SilverStripe\Core\Injector\Injector->convertServiceProperty() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:513
 SilverStripe\Core\Injector\Injector->convertServiceProperty() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:497
 SilverStripe\Core\Injector\Injector->updateSpecConstructor() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:1023
 SilverStripe\Core\Injector\Injector->getNamedService() at /var/www/vendor/silverstripe/framework/src/Core/Injector/Injector.php:979
 SilverStripe\Core\Injector\Injector->get() at /var/www/vendor/silverstripe/framework/src/Control/Session.php:209
 SilverStripe\Control\Session::getSaveHandler() at /var/www/vendor/silverstripe/framework/src/Control/SessionHandler/DbBuildSessionExtension.php:56
 SilverStripe\Control\SessionHandler\DbBuildSessionExtension->getSessionHandler() at /var/www/vendor/silverstripe/framework/src/Control/SessionHandler/DbBuildSessionExtension.php:42
 SilverStripe\Control\SessionHandler\DbBuildSessionExtension->onAfterBuild() at /var/www/vendor/silverstripe/framework/src/Core/Extension.php:141
 SilverStripe\Core\Extension->invokeExtension() at /var/www/vendor/silverstripe/framework/src/Core/Extensible.php:437
 SilverStripe\Dev\Command\DbBuild->extend() at /var/www/vendor/silverstripe/framework/src/Dev/Command/DbBuild.php:178
 SilverStripe\Dev\Command\DbBuild->doBuild() at /var/www/vendor/silverstripe/framework/src/Dev/Command/DbBuild.php:78
 SilverStripe\Dev\Command\DbBuild->execute() at /var/www/vendor/silverstripe/framework/src/Dev/Command/DevCommand.php:37
 SilverStripe\Dev\Command\DevCommand->run() at /var/www/vendor/silverstripe/framework/src/Cli/Command/PolyCommandCliWrapper.php:39
 SilverStripe\Cli\Command\PolyCommandCliWrapper->execute() at /var/www/vendor/symfony/console/Command/Command.php:318
 Symfony\Component\Console\Command\Command->run() at /var/www/vendor/symfony/console/Application.php:1092
 Symfony\Component\Console\Application->doRunCommand() at /var/www/vendor/silverstripe/framework/src/Cli/Sake.php:219
 SilverStripe\Cli\Sake->doRunCommand() at /var/www/vendor/symfony/console/Application.php:341
 Symfony\Component\Console\Application->doRun() at /var/www/vendor/symfony/console/Application.php:192
 Symfony\Component\Console\Application->run() at /var/www/vendor/silverstripe/framework/src/Cli/Sake.php:118
 SilverStripe\Cli\Sake->run() at /var/www/vendor/silverstripe/framework/bin/sake:19
 include() at /var/www/vendor/bin/sake:119

db:build [--no-populate] [--dont_populate]

@GuySartorelli
Copy link
Copy Markdown
Member Author

I think I must have only tested that before setting up the env var. The new injector config doesn't seem to be working based on that first error. I'll look into it.

@GuySartorelli
Copy link
Copy Markdown
Member Author

For some reason with the cached session handler it's forcing dev/build if I'm not authenticated (after fixing the above).... will look into that now.

@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch from 31130fd to 8fe18ae Compare July 28, 2025 23:27
@GuySartorelli
Copy link
Copy Markdown
Member Author

Rebased in case something where's happening as a result of recent changes

@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch from 8fe18ae to a06e507 Compare July 28, 2025 23:38
@GuySartorelli
Copy link
Copy Markdown
Member Author

The dev/build thing is caused by #11806 - I'll need to rebase again after the fix is merged.

@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch 2 times, most recently from 7b03459 to e1abf42 Compare July 29, 2025 00:13
The intention is to use in-memory cache adapters such as Redis and
Memcached - though theoretically any PSR16 adapter would work.
This is an easy option for multi-server hosting environments, though is
likely less performant than the other session handlers on offer.
This allows hosting providers to set specific session save handlers if
they have infrastructure set up for one that is preferred (e.g. if using
redis).
This allows us to use environment variables to set a factory class name
in injector configuration.
@GuySartorelli GuySartorelli force-pushed the pulls/6/session-handlers branch from e1abf42 to e8e6948 Compare July 29, 2025 04:03
@emteknetnz emteknetnz merged commit ad0b82a into silverstripe:6 Jul 30, 2025
12 checks passed
@emteknetnz emteknetnz deleted the pulls/6/session-handlers branch July 30, 2025 02:59
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