feat: added Redis-ready session scaling#3978
Conversation
📝 WalkthroughWalkthroughAdds Redis-backed session support: Docker images updated to PHP 8.5 and install the Redis PHP extension; new Redis service in docker-compose; runtime session configuration and a new RedisSessionHandler class; default config and migrations updated; tests added to cover session and Redis connection behavior. Changes
Sequence DiagramsequenceDiagram
participant App as Application Bootstrap
participant Config as Configuration
participant PC as PhpConfigurator
participant RSH as RedisSessionHandler
participant Redis as Redis Service
App->>PC: configureSession(faqConfig)
PC->>Config: get('session.handler')
Config-->>PC: 'redis' / 'files' / other
alt handler == 'redis'
PC->>Config: get('session.redisDsn')
Config-->>PC: 'tcp://redis:6379?database=0'
PC->>RSH: configure(dsn, validate=true)
RSH->>RSH: buildSocketTarget(dsn)
RSH->>Redis: stream_socket_client(tcp://redis:6379)
Redis-->>RSH: connect / fail
RSH->>RSH: ini_set('session.save_handler','redis')
RSH->>RSH: ini_set('session.save_path', dsn)
else handler == 'files'
PC->>PC: ini_set('session.save_handler','files')
else
PC-->>App: throw RuntimeException("Unsupported session handler")
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @.docker/apache/Dockerfile:
- Around line 64-66: The Dockerfile currently installs the Redis PHP extension
without a version lock via the RUN command `pecl install redis &&
docker-php-ext-enable redis`; change this to pin the extension to the known
stable version by specifying the version in the pecl install command (e.g.,
`pecl install redis-6.3.0`) and keep the `docker-php-ext-enable redis` step so
builds are reproducible and unaffected by future upstream major releases.
In `@phpmyfaq/src/phpMyFAQ/Bootstrap/PhpConfigurator.php`:
- Around line 76-89: The switch on $sessionHandler currently includes a
'database' case that falls through to ini_set('session.save_handler', 'files'),
which misleads admins; update the switch in PhpConfigurator so 'database' is
treated as unsupported: remove the 'database' label (or replace its branch) and
let the default branch throw the RuntimeException that lists allowed values, or
explicitly throw a RuntimeException for 'database' with the same message; keep
RedisSessionHandler::configure($redisDsn) unchanged.
In `@phpmyfaq/src/phpMyFAQ/Session/RedisSessionHandler.php`:
- Around line 28-39: The configure() method currently calls
validateConnection($redisDsn) on every invocation causing extra TCP
connect/close and TOCTOU risk; remove that proactive connectivity check from
configure() (leave ini_set('session.save_handler','redis') and
ini_set('session.save_path',$redisDsn) intact) and instead make validation
opt-in (e.g., add a new method validateAndSaveDsnForSetup or accept a boolean
$validate = false parameter) so that validateConnection is only invoked during
setup/admin actions; reference the configure(), validateConnection(),
DEFAULT_DSN, and the session.save_handler/session.save_path ini settings when
making this change.
In `@README.md`:
- Line 78: Update the stale PHP version mentions in README.md by replacing each
occurrence of the string "PHP 8.4" with "PHP 8.5" (the Dockerfiles were bumped),
ensuring entries that reference PHP versions (look for literal "PHP 8.4"
occurrences near the service list around the Redis entry) are updated so the
README matches the current Dockerfiles.
🧹 Nitpick comments (3)
docker-compose.yml (1)
31-38: Redis has no authentication configured.While the file header says "development only," the Redis instance has no password (
requirepass) and its port is published to the host. If this compose file is ever used in a less-trusted network, the Redis instance is openly writable. Consider adding a password via the command or environment, consistent with how MariaDB/Postgres are configured.redis: image: redis:7-alpine restart: always - command: redis-server --appendonly yes + command: redis-server --appendonly yes --requirepass phpmyfaq ports: - '6379:6379' volumes: - ./volumes/redis:/dataIf a password is added, the DSN default would need updating to include
?auth=phpmyfaq(e.g.,tcp://redis:6379?auth=phpmyfaq&database=0).tests/phpMyFAQ/Setup/Migration/Versions/Migration420AlphaTest.php (1)
83-109: Test method name is misleading — it now verifies more than rate-limit entries.
testUpAddsRateLimitConfigEntriesalso assertsqueue.transport,session.handler, andsession.redisDsn. Consider renaming to something liketestUpAddsExpectedConfigEntriesto accurately reflect the broader scope.tests/phpMyFAQ/Bootstrap/PhpConfiguratorTest.php (1)
118-152: Temp directory cleanup may be skipped if the assertion fails.If
assertSameon line 145 throws, lines 147–151 won't execute, leaving the temp session directory behind. Consider moving cleanup into afinallyblock or usingtearDown.Proposed fix using try/finally
- $this->assertSame('ok', $value); - - $sessionFile = $sessionDirectory . '/sess_' . $sessionId; - if (file_exists($sessionFile)) { - unlink($sessionFile); + try { + $this->assertSame('ok', $value); + } finally { + $sessionFile = $sessionDirectory . '/sess_' . $sessionId; + if (file_exists($sessionFile)) { + unlink($sessionFile); + } + if (is_dir($sessionDirectory)) { + rmdir($sessionDirectory); + } } - rmdir($sessionDirectory);
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@README.md`:
- Line 11: The README has inconsistent PHP version requirements: change the PHP
requirement text so both occurrences match (either update the phrase on line 36
that currently says "only supported on PHP 8.3 and up" to "requires PHP 8.5+" or
revert the new text on line 11 back to "PHP 8.3 and up"); locate the two strings
in the README (the sentence at the top mentioning "requires PHP 8.5+" and the
sentence around line 36 mentioning "only supported on PHP 8.3 and up") and make
them identical, keeping formatting and punctuation consistent.
🧹 Nitpick comments (1)
phpmyfaq/src/phpMyFAQ/Bootstrap/PhpConfigurator.php (1)
69-88:PMF_SESSION_SAVE_PATHis set before the handler switch, so Redis will silently overwrite it.When
session.handlerisredis,RedisSessionHandler::configure()overwritessession.save_path(set on line 70) with the Redis DSN. The earlierini_setis harmless but misleading. Consider moving thePMF_SESSION_SAVE_PATHblock inside the'files'case so the intent is clearer.♻️ Suggested restructuring
- if (defined('PMF_SESSION_SAVE_PATH') && PMF_SESSION_SAVE_PATH !== '') { - ini_set('session.save_path', value: PMF_SESSION_SAVE_PATH); - } - $sessionHandler = strtolower((string) ($configuration?->get('session.handler') ?? 'files')); $redisDsn = trim((string) ($configuration?->get('session.redisDsn') ?? '')); switch ($sessionHandler) { case 'files': ini_set('session.save_handler', value: 'files'); + if (defined('PMF_SESSION_SAVE_PATH') && PMF_SESSION_SAVE_PATH !== '') { + ini_set('session.save_path', value: PMF_SESSION_SAVE_PATH); + } break; case 'redis': RedisSessionHandler::configure($redisDsn); break;
1 participant
Summary by CodeRabbit
New Features
Infrastructure
Documentation
Tests