Add SESSION hash support to Request class#147
Conversation
|
Note
|
| Cohort / File(s) | Summary |
|---|---|
Request class — SESSION source support src/Request.php |
Adds SESSION to documented and internal source/destination hashes. getVar/get switch now handles SESSION, returning $_SESSION when session_status() indicates an active session, otherwise using empty input/defaults. hasVar uses the same path. setVar includes a SESSION case that silently skips writes if no active session. Docblocks updated to include SESSION. |
Request tests — session-aware coverage tests/unit/RequestTest.php |
Adds session lifecycle helpers (startTestSession, closeTestSession) and a suite of tests exercising read/write/has semantics for the session source under active and inactive sessions: present/missing keys, typed getters (int), hash retrieval, ignored writes when no session, and teardown cleanup. Also stores/restores relevant INI state. |
Estimated code review effort
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 35.29% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title check | ✅ Passed | The title 'Add SESSION hash support to Request class' clearly and specifically describes the main change: adding SESSION as a valid hash parameter to the Request class methods. |
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
- Post copyable unit tests in a comment
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 help to get the list of available commands and usage tips.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Request.php (1)
1-1: 🧹 Nitpick | 🔵 TrivialPipeline failures exist in unchanged code.
The CI pipeline reports formatting issues (header block separation at line 1, blank lines at end of control structures at lines 125 and 131, and missing visibility on constants at lines 35-37). These issues predate this PR's changes. Consider addressing them in a separate cleanup PR to avoid scope creep.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Request.php` at line 1, The CI reports style issues in Request.php that should be fixed in a cleanup PR: add the required header block separation (ensure a blank line after the opening <?php), declare explicit visibility for the constants defined around the identifiers at lines 35-37 (e.g., add public const for each constant in the Request class), and remove the extra blank lines at the end of the control structures referenced near the methods/blocks around lines 125 and 131 so there are no stray blank lines before closing braces; update the file accordingly and run the formatter/linter to confirm compliance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/RequestTest.php`:
- Around line 283-295: The test testGetVarSessionReturnsDefaultWhenNoSession
manipulates global session state and reopens it at the end, creating implicit
inter-test dependencies; refactor by adding setUp() and tearDown() methods on
the test class to ensure a consistent session state for every test (use
session_status(), session_start(), and session_write_close() there) so
individual tests like testGetVarSessionReturnsDefaultWhenNoSession can close the
session without needing to restart it themselves and other tests no longer rely
on execution order.
---
Outside diff comments:
In `@src/Request.php`:
- Line 1: The CI reports style issues in Request.php that should be fixed in a
cleanup PR: add the required header block separation (ensure a blank line after
the opening <?php), declare explicit visibility for the constants defined around
the identifiers at lines 35-37 (e.g., add public const for each constant in the
Request class), and remove the extra blank lines at the end of the control
structures referenced near the methods/blocks around lines 125 and 131 so there
are no stray blank lines before closing braces; update the file accordingly and
run the formatter/linter to confirm compliance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8a993894-69b0-410c-a695-6849f7988cc5
📒 Files selected for processing (2)
src/Request.phptests/unit/RequestTest.php
There was a problem hiding this comment.
Pull request overview
Adds SESSION as a supported input source to the XMF Request utility, enabling Request::getVar(), Request::get(), and Request::setVar() to read/write $_SESSION when a session is active, with safe fallbacks when it isn’t.
Changes:
- Add
SESSIONhandling paths toRequest::getVar(),Request::get(), andRequest::setVar() - Update
Requestdocblocks to mention the newsessionhash option - Add PHPUnit coverage for session-based reads/writes and no-session behavior
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/Request.php |
Adds SESSION as a supported hash for request accessors/mutators and updates docs accordingly |
tests/unit/RequestTest.php |
Adds unit tests validating session read/write behavior and fallbacks when no session is active |
src/Request.php
Outdated
| * @param string $name Variable name | ||
| * @param mixed $default Default value if the variable does not exist | ||
| * @param string $hash Source of variable value (POST, GET, FILES, COOKIE, METHOD) | ||
| * @param string $hash Source of variable value (POST, GET, FILES, COOKIE, SESSION, METHOD) |
There was a problem hiding this comment.
The getVar() docblock’s $hash parameter description is now incomplete/inaccurate: the method supports ENV, SERVER, and DEFAULT/REQUEST in addition to the listed values. Update the @param $hash text (and/or the earlier bullet list) so the documented options match the actual switch cases.
| * @param string $hash Source of variable value (POST, GET, FILES, COOKIE, SESSION, METHOD) | |
| * @param string $hash Source of variable value (GET, POST, FILES, COOKIE, ENV, SERVER, SESSION, METHOD, DEFAULT/REQUEST) |
src/Request.php
Outdated
| * - default $_REQUEST | ||
| * | ||
| * @param string $hash to get (POST, GET, FILES, METHOD) | ||
| * @param string $hash to get (POST, GET, FILES, SESSION, METHOD) |
There was a problem hiding this comment.
The get() docblock’s “@param string $hash to get (...)” list is incomplete: get() also supports COOKIE, ENV, SERVER and DEFAULT/REQUEST per the switch. Please update the parameter documentation so callers don’t miss supported hashes.
| * @param string $hash to get (POST, GET, FILES, SESSION, METHOD) | |
| * @param string $hash to get (POST, GET, FILES, COOKIE, ENV, SERVER, SESSION, METHOD, DEFAULT/REQUEST) |
| case 'SESSION': | ||
| if (session_status() === PHP_SESSION_ACTIVE) { | ||
| $_SESSION[$name] = $value; | ||
| } | ||
| break; |
There was a problem hiding this comment.
SESSION support was added to setVar(), but the method’s docblock doesn’t mention SESSION or the no-op behavior when no session is active. Please update the setVar() documentation to include SESSION and clarify what happens when the session isn’t active.
tests/unit/RequestTest.php
Outdated
| if (session_status() !== PHP_SESSION_ACTIVE) { | ||
| @session_start(); | ||
| } |
There was a problem hiding this comment.
These tests suppress session_start() errors and don’t assert that the session actually became active. That can hide environment/config problems and make failures harder to diagnose. Consider asserting PHP_SESSION_ACTIVE after attempting to start (or mark the test skipped if sessions can’t be started).
tests/unit/RequestTest.php
Outdated
| if (session_status() !== PHP_SESSION_ACTIVE) { | ||
| @session_start(); | ||
| } | ||
| $varname = 'RequestTestSession'; | ||
| $_SESSION[$varname] = 'session_value'; | ||
|
|
||
| $this->assertEquals('session_value', Request::getVar($varname, null, 'session')); | ||
|
|
||
| unset($_SESSION[$varname]); | ||
| } | ||
|
|
||
| public function testGetVarSessionReturnsDefaultWhenKeyMissing() | ||
| { | ||
| if (session_status() !== PHP_SESSION_ACTIVE) { | ||
| @session_start(); | ||
| } | ||
|
|
||
| $this->assertNull(Request::getVar('no_such_session_key', null, 'session')); | ||
| $this->assertEquals('fallback', Request::getVar('no_such_session_key', 'fallback', 'session')); | ||
| } | ||
|
|
||
| public function testGetVarSessionReturnsDefaultWhenNoSession() | ||
| { | ||
| // When session is not active, getVar should return the default | ||
| if (session_status() === PHP_SESSION_ACTIVE) { | ||
| session_write_close(); | ||
| } | ||
|
|
||
| $this->assertNull(Request::getVar('any_key', null, 'session')); | ||
| $this->assertEquals('default_val', Request::getVar('any_key', 'default_val', 'session')); | ||
|
|
||
| // Restart session for other tests | ||
| @session_start(); | ||
| } | ||
|
|
||
| public function testGetIntFromSession() | ||
| { | ||
| if (session_status() !== PHP_SESSION_ACTIVE) { | ||
| @session_start(); | ||
| } | ||
| $varname = 'RequestTestSessionInt'; | ||
| $_SESSION[$varname] = '42'; | ||
|
|
||
| $this->assertEquals(42, Request::getInt($varname, 0, 'session')); | ||
|
|
||
| unset($_SESSION[$varname]); |
There was a problem hiding this comment.
This test mutates global session state (session_write_close() / session_start()) without ensuring it’s restored if an assertion fails. Wrapping the state changes and cleanup in try/finally (and restoring the prior session status) would prevent session state from leaking into subsequent tests.
| if (session_status() !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } | |
| $varname = 'RequestTestSession'; | |
| $_SESSION[$varname] = 'session_value'; | |
| $this->assertEquals('session_value', Request::getVar($varname, null, 'session')); | |
| unset($_SESSION[$varname]); | |
| } | |
| public function testGetVarSessionReturnsDefaultWhenKeyMissing() | |
| { | |
| if (session_status() !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } | |
| $this->assertNull(Request::getVar('no_such_session_key', null, 'session')); | |
| $this->assertEquals('fallback', Request::getVar('no_such_session_key', 'fallback', 'session')); | |
| } | |
| public function testGetVarSessionReturnsDefaultWhenNoSession() | |
| { | |
| // When session is not active, getVar should return the default | |
| if (session_status() === PHP_SESSION_ACTIVE) { | |
| session_write_close(); | |
| } | |
| $this->assertNull(Request::getVar('any_key', null, 'session')); | |
| $this->assertEquals('default_val', Request::getVar('any_key', 'default_val', 'session')); | |
| // Restart session for other tests | |
| @session_start(); | |
| } | |
| public function testGetIntFromSession() | |
| { | |
| if (session_status() !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } | |
| $varname = 'RequestTestSessionInt'; | |
| $_SESSION[$varname] = '42'; | |
| $this->assertEquals(42, Request::getInt($varname, 0, 'session')); | |
| unset($_SESSION[$varname]); | |
| $previousStatus = session_status(); | |
| $varname = 'RequestTestSession'; | |
| try { | |
| if ($previousStatus !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } | |
| $_SESSION[$varname] = 'session_value'; | |
| $this->assertEquals('session_value', Request::getVar($varname, null, 'session')); | |
| } finally { | |
| if (isset($_SESSION[$varname])) { | |
| unset($_SESSION[$varname]); | |
| } | |
| $currentStatus = session_status(); | |
| if ($previousStatus !== $currentStatus) { | |
| if ($previousStatus === PHP_SESSION_ACTIVE && $currentStatus !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } elseif ($previousStatus !== PHP_SESSION_ACTIVE && $currentStatus === PHP_SESSION_ACTIVE) { | |
| session_write_close(); | |
| } | |
| } | |
| } | |
| } | |
| public function testGetVarSessionReturnsDefaultWhenKeyMissing() | |
| { | |
| $previousStatus = session_status(); | |
| try { | |
| if ($previousStatus !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } | |
| $this->assertNull(Request::getVar('no_such_session_key', null, 'session')); | |
| $this->assertEquals('fallback', Request::getVar('no_such_session_key', 'fallback', 'session')); | |
| } finally { | |
| $currentStatus = session_status(); | |
| if ($previousStatus !== $currentStatus) { | |
| if ($previousStatus === PHP_SESSION_ACTIVE && $currentStatus !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } elseif ($previousStatus !== PHP_SESSION_ACTIVE && $currentStatus === PHP_SESSION_ACTIVE) { | |
| session_write_close(); | |
| } | |
| } | |
| } | |
| } | |
| public function testGetVarSessionReturnsDefaultWhenNoSession() | |
| { | |
| $previousStatus = session_status(); | |
| try { | |
| // When session is not active, getVar should return the default | |
| if ($previousStatus === PHP_SESSION_ACTIVE) { | |
| session_write_close(); | |
| } | |
| $this->assertNull(Request::getVar('any_key', null, 'session')); | |
| $this->assertEquals('default_val', Request::getVar('any_key', 'default_val', 'session')); | |
| } finally { | |
| $currentStatus = session_status(); | |
| if ($previousStatus !== $currentStatus) { | |
| if ($previousStatus === PHP_SESSION_ACTIVE && $currentStatus !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } elseif ($previousStatus !== PHP_SESSION_ACTIVE && $currentStatus === PHP_SESSION_ACTIVE) { | |
| session_write_close(); | |
| } | |
| } | |
| } | |
| } | |
| public function testGetIntFromSession() | |
| { | |
| $previousStatus = session_status(); | |
| $varname = 'RequestTestSessionInt'; | |
| try { | |
| if ($previousStatus !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } | |
| $_SESSION[$varname] = '42'; | |
| $this->assertEquals(42, Request::getInt($varname, 0, 'session')); | |
| } finally { | |
| if (isset($_SESSION[$varname])) { | |
| unset($_SESSION[$varname]); | |
| } | |
| $currentStatus = session_status(); | |
| if ($previousStatus !== $currentStatus) { | |
| if ($previousStatus === PHP_SESSION_ACTIVE && $currentStatus !== PHP_SESSION_ACTIVE) { | |
| @session_start(); | |
| } elseif ($previousStatus !== PHP_SESSION_ACTIVE && $currentStatus === PHP_SESSION_ACTIVE) { | |
| session_write_close(); | |
| } | |
| } | |
| } |
Allow reading from and writing to $_SESSION via the $hash parameter. When session is not active, getVar() returns the default value and setVar() silently skips the write, preventing errors.
mambax7
force-pushed
the
feature/add-session-hash-to-request
branch
from
a6dc00a to
69edcc8
Compare
- Add setUp()/tearDown() session lifecycle management instead of per-test @session_start() calls with implicit cross-test dependencies - Add requireActiveSession() helper that skips tests when sessions are unavailable, removing @ error suppression - Complete @param $hash docs on getVar(), get(), setVar(), and set() to list all supported hashes (GET, POST, FILES, COOKIE, ENV, SERVER, SESSION, METHOD, DEFAULT/REQUEST) - Document setVar() SESSION no-op behavior when no session is active
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/Request.php (2)
32-35:⚠️ Potential issue | 🟡 MinorAdd the missing blank line before the constants block.
CI is still failing on Line 35 with “Header blocks must be separated by a single blank line.” Insert a blank line between this docblock and
const MASK_NO_TRIM.As per coding guidelines,
src/**/*.php: Follow PSR-12 with a line length limit of 200 characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Request.php` around lines 32 - 35, Add a single blank line between the docblock for "Available masks for cleaning variables" and the constants block so the header block is separated from the code; specifically insert one empty line above the declaration of const MASK_NO_TRIM in Request.php (the constants block containing MASK_NO_TRIM) to satisfy PSR-12 header separation and the CI rule.
122-132:⚠️ Potential issue | 🟡 MinorRemove the trailing blank lines inside
getVar().The extra blank lines after the assignments in the
if/elsechain are still tripping PHPCS on Line 125 and Line 131.♻️ Suggested fix
if (isset($input[$name]) && null !== $input[$name]) { // Get the variable from the input hash and clean it $var = static::cleanVar($input[$name], $mask, $type); - } elseif (null !== $default) { // Clean the default value $var = static::cleanVar($default, $mask, $type); } else { $var = $default; - }As per coding guidelines,
src/**/*.php: Follow PSR-12 with a line length limit of 200 characters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Request.php` around lines 122 - 132, Remove the extra blank lines inside the getVar() if/elseif/else blocks so PHPCS/PSR-12 complaints are resolved: in the code that assigns $var (the branch using static::cleanVar($input[$name], $mask, $type), the elseif branch using static::cleanVar($default, $mask, $type), and the final else setting $var = $default) delete the blank lines immediately after those assignments so each branch contains only the assignment and no trailing empty lines.tests/unit/RequestTest.php (1)
17-35:⚠️ Potential issue | 🟠 MajorClear
$_SESSIONin the fixture hooks.Starting or reopening the session does not reset its contents. If one of the new session tests fails before its
unset(), that key survives into later methods and makes the class order-dependent. Reset the session array insetUp()/tearDown()so these cases stay isolated.♻️ Suggested fix
protected function setUp(): void { $this->object = new Request; // Ensure a consistent session state for tests that need it if (session_status() !== PHP_SESSION_ACTIVE) { session_start(); } + $_SESSION = array(); } @@ protected function tearDown(): void { // Restore active session state after each test if (session_status() !== PHP_SESSION_ACTIVE) { session_start(); } + $_SESSION = array(); }As per coding guidelines,
tests/unit/**/*.php: Tests must be fully isolated with no XOOPS installation required.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/unit/RequestTest.php` around lines 17 - 35, The tests leave session state between runs: in RequestTest::setUp() and RequestTest::tearDown() ensure the session is started (session_status() check) and then reset $_SESSION to an empty array (e.g., $_SESSION = []) so each test starts/ends with a clean session; update the setUp() and tearDown() methods to start the session if needed and immediately clear $_SESSION to guarantee isolation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/Request.php`:
- Around line 32-35: Add a single blank line between the docblock for "Available
masks for cleaning variables" and the constants block so the header block is
separated from the code; specifically insert one empty line above the
declaration of const MASK_NO_TRIM in Request.php (the constants block containing
MASK_NO_TRIM) to satisfy PSR-12 header separation and the CI rule.
- Around line 122-132: Remove the extra blank lines inside the getVar()
if/elseif/else blocks so PHPCS/PSR-12 complaints are resolved: in the code that
assigns $var (the branch using static::cleanVar($input[$name], $mask, $type),
the elseif branch using static::cleanVar($default, $mask, $type), and the final
else setting $var = $default) delete the blank lines immediately after those
assignments so each branch contains only the assignment and no trailing empty
lines.
In `@tests/unit/RequestTest.php`:
- Around line 17-35: The tests leave session state between runs: in
RequestTest::setUp() and RequestTest::tearDown() ensure the session is started
(session_status() check) and then reset $_SESSION to an empty array (e.g.,
$_SESSION = []) so each test starts/ends with a clean session; update the
setUp() and tearDown() methods to start the session if needed and immediately
clear $_SESSION to guarantee isolation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 391cc42e-05ac-42c7-b124-4b1e143b26d4
📒 Files selected for processing (2)
src/Request.phptests/unit/RequestTest.php
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
Comments suppressed due to low confidence (1)
src/Request.php:404
- The setVar() PHPDoc types look too narrow: $value isn’t necessarily a string (it’s mixed in practice), and the return value can be null/mixed (not always string). Since this docblock was updated, it would be good to align @param/@return types with the actual signature/behavior to help static analysis and API consumers.
* For SESSION, the write is silently skipped if no session is active.
*
* @param string $name Name
* @param string $value Value
* @param string $hash Hash (GET, POST, REQUEST, COOKIE, FILES, ENV, SERVER, SESSION, METHOD)
* @param bool $overwrite Boolean
*
* @return string Previous value
*/
tests/unit/RequestTest.php
Outdated
| // Ensure a consistent session state for tests that need it | ||
| if (session_status() !== PHP_SESSION_ACTIVE) { | ||
| session_start(); | ||
| } |
There was a problem hiding this comment.
Starting a session in setUp() for every test can emit warnings/failures in environments where sessions are disabled or headers can’t be modified, and it also forces global state changes on tests that don’t need sessions. Consider removing session_start() from setUp() and instead have requireActiveSession() attempt to start the session (e.g., with @session_start()) and mark the specific session-dependent tests skipped if it can’t be activated.
tests/unit/RequestTest.php
Outdated
| // Restore active session state after each test | ||
| if (session_status() !== PHP_SESSION_ACTIVE) { | ||
| session_start(); | ||
| } |
There was a problem hiding this comment.
tearDown() currently tries to start a session when it’s not active, which doesn’t really “restore” state and can re-introduce the same session_start() warnings after a test closes the session (including skipped tests). Prefer leaving session state alone in tearDown(), or explicitly closing/cleaning up (e.g., session_write_close() and clearing $_SESSION) if you need isolation.
| // Restore active session state after each test | |
| if (session_status() !== PHP_SESSION_ACTIVE) { | |
| session_start(); | |
| } | |
| // Do not modify session state here; individual tests or setUp() manage it. | |
| $this->object = null; |
| * Helper to ensure an active session, skipping the test if unavailable. | ||
| */ | ||
| private function requireActiveSession(): void | ||
| { |
There was a problem hiding this comment.
requireActiveSession() only checks session_status() but never attempts to start the session, so its skip message can be misleading and it relies on setUp() having already started the session. If you move session initialization out of setUp(), update this helper to attempt to start the session (and skip only if activation fails).
| { | |
| { | |
| if (session_status() === PHP_SESSION_ACTIVE) { | |
| return; | |
| } | |
| // Try to start a session if one is not already active. | |
| if (headers_sent()) { | |
| $this->markTestSkipped('Cannot start a session: headers already sent in this environment.'); | |
| } | |
| try { | |
| @session_start(); | |
| } catch (\Throwable $exception) { | |
| $this->markTestSkipped('Cannot start a session in this environment: ' . $exception->getMessage()); | |
| } |
tests/unit/RequestTest.php
Outdated
| $this->requireActiveSession(); | ||
| session_write_close(); | ||
|
|
||
| $varname = 'XMF_TEST_SESSION_NO_WRITE'; | ||
| Request::setVar($varname, 'should_not_persist', 'session'); | ||
|
|
||
| // tearDown() restarts the session; verify nothing was set | ||
| session_start(); | ||
| $this->assertArrayNotHasKey($varname, $_SESSION); | ||
| } |
There was a problem hiding this comment.
This test assumes the session doesn’t already contain $varname. If a prior run/environment leaves the key set, the final assertArrayNotHasKey() can fail even though setVar() correctly skipped writing. Make the test deterministic by unsetting the key before session_write_close() and after session_start(), or by using a fresh session id / clearing $_SESSION for the test.
| * - post $_POST | ||
| * - get $_GET | ||
| * - files $_FILES | ||
| * - cookie $_COOKIE | ||
| * - env $_ENV | ||
| * - server $_SERVER | ||
| * - session $_SESSION (returns default if no active session) | ||
| * - method via current $_SERVER['REQUEST_METHOD'] | ||
| * - default $_REQUEST | ||
| * | ||
| * @param string $name Variable name | ||
| * @param mixed $default Default value if the variable does not exist | ||
| * @param string $hash Source of variable value (POST, GET, FILES, COOKIE, METHOD) | ||
| * @param string $hash Source of variable value (GET, POST, FILES, COOKIE, ENV, SERVER, SESSION, METHOD, DEFAULT/REQUEST) | ||
| * @param string $type Return type for the variable (INT, FLOAT, BOOLEAN, WORD, | ||
| * ALPHANUM, CMD, BASE64, STRING, ARRAY, PATH, NONE) For more |
There was a problem hiding this comment.
The getVar() docblock now advertises SESSION support, but the proxy methods (e.g., getInt/getFloat/getBool/etc.) still document only POST/GET/FILES/COOKIE/METHOD even though they accept the same $hash values via getVar(). Consider updating those docblocks too so the public API documentation stays consistent.
- Remove session_start() from setUp()/tearDown() — it emits warnings in CLI environments where headers are already sent, causing PHPUnit to convert them to test failures - Add startTestSession() helper that disables cookie-based session IDs (session.use_cookies=0, session.use_only_cookies=0) before starting, then asserts PHP_SESSION_ACTIVE or skips the test - Add closeTestSession() helper that closes session and asserts it is no longer active - Each session test is self-contained with try/finally cleanup, no cross-test state dependencies - No @ error suppression anywhere
tests/unit/RequestTest.php
Outdated
| ini_set('session.use_cookies', '0'); | ||
| ini_set('session.use_only_cookies', '0'); | ||
| ini_set('session.cache_limiter', ''); | ||
| session_start(); |
There was a problem hiding this comment.
startTestSession() mutates global session ini settings (session.use_cookies, session.use_only_cookies, session.cache_limiter) but never restores the previous values. This can leak configuration into other unit tests running in the same process. Capture the previous ini values and restore them in closeTestSession()/tearDown() (or via a try/finally around the mutation).
tests/unit/RequestTest.php
Outdated
| // Start a fresh session and verify nothing leaked | ||
| $this->startTestSession(); | ||
| try { | ||
| $this->assertArrayNotHasKey($varname, $_SESSION); | ||
| } finally { |
There was a problem hiding this comment.
This test says “Start a fresh session”, but closeTestSession() only calls session_write_close() and does not destroy the session or reset the session id, so the next session_start() may resume the same session. Either update the comment/assertion to match the behavior, or ensure a truly new session (e.g., destroy/reset/regenerate id) before asserting no data leaked.
tests/unit/RequestTest.php
Outdated
| session_start(); | ||
| if (session_status() !== PHP_SESSION_ACTIVE) { |
There was a problem hiding this comment.
startTestSession() calls session_start() directly. With failOnWarning="true" in phpunit.xml, any warning emitted by session_start() (e.g., session misconfiguration, headers already sent) will fail the test run before markTestSkipped() is reached. Consider using @session_start() (or checking the boolean return value) and skipping when startup fails, so environments that can’t start sessions don’t produce warnings.
| session_start(); | |
| if (session_status() !== PHP_SESSION_ACTIVE) { | |
| $started = @session_start(); | |
| if ($started === false || session_status() !== PHP_SESSION_ACTIVE) { |
PHPStan 2.1 no longer reports the array_filter callable error for Metagen.php, causing the baseline entry to be unmatched. This setting prevents unmatched baseline entries from being treated as errors during incremental baseline cleanup.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/RequestTest.php`:
- Around line 267-293: The helper leaves INI changes and $_SESSION around;
modify startTestSession() to save the original INI values (e.g., into a private
property like $savedSessionIni) before calling
ini_set('session.use_cookies','0'), ini_set('session.use_only_cookies','0'),
ini_set('session.cache_limiter',''), then start the session as before; update
closeTestSession() to fully clean up by, when session_status() ===
PHP_SESSION_ACTIVE, unsetting session data (session_unset() or $_SESSION = []),
calling session_destroy(), calling session_write_close(), and then restoring the
original INI values from $savedSessionIni so process-wide settings and $_SESSION
are reset for other tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f84febeb-82f0-416a-8856-19d1c5bd69db
📒 Files selected for processing (1)
tests/unit/RequestTest.php
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #147 +/- ##
============================================
+ Coverage 33.23% 33.62% +0.38%
- Complexity 1058 1064 +6
============================================
Files 45 45
Lines 2750 2766 +16
============================================
+ Hits 914 930 +16
Misses 1836 1836 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
phpstan.neon
Outdated
| excludePaths: | ||
| - tests/ | ||
| treatPhpDocTypesAsCertain: false | ||
| reportUnmatchedIgnoredErrors: false |
There was a problem hiding this comment.
This PR is scoped to adding SESSION support in Request, but it also changes PHPStan behavior by disabling reportUnmatchedIgnoredErrors. Turning this off reduces signal from static analysis (stale ignoreErrors entries will no longer be reported) and isn’t mentioned in the PR description; consider reverting it (or documenting why it’s required and addressing the underlying unmatched ignore instead).
src/Request.php 1. Proxy method docblocks updated — all 10 methods (getInt, getFloat, getBool, getWord, getCmd, getString, getArray, getText, getUrl, getPath) now list (POST, GET, FILES, COOKIE, ENV, SERVER, SESSION, METHOD) instead of just (POST, GET, FILES, COOKIE, METHOD) 2. PSR-12 fix — removed extra blank lines at lines 125 and 131 inside getVar() if/else blocks tests/unit/RequestTest.php 3. startTestSession() — now saves original INI values before overriding, uses @session_start() to suppress warnings, and clears $_SESSION on entry 4. closeTestSession() — now calls $_SESSION = [] and session_destroy() instead of just session_write_close() 5. tearDown() — added full cleanup: destroys active session, clears $_SESSION, restores saved INI values 6. Fixed misleading comment — "Start a fresh session" → "Re-open session and verify the write was skipped" phpstan.neon + phpstan-baseline.neon 7. Removed reportUnmatchedIgnoredErrors: false and regenerated baseline (done earlier)
|
| private function closeTestSession(): void | ||
| { | ||
| if (session_status() === PHP_SESSION_ACTIVE) { | ||
| $_SESSION = []; | ||
| session_destroy(); | ||
| } | ||
| $this->assertNotSame( | ||
| PHP_SESSION_ACTIVE, | ||
| session_status(), | ||
| 'Session should not be active after close.' | ||
| ); | ||
| } |
There was a problem hiding this comment.
session_destroy() clears session data but does not actually close the session in the current process, so session_status() can remain PHP_SESSION_ACTIVE. This makes the assertion in closeTestSession() unreliable and can leave a session active across tests. Prefer explicitly closing the session (e.g., call session_write_close()/session_abort() after clearing) and only use session_destroy() if you also close the session afterward.
| // Ensure full cleanup even if a test fails mid-way | ||
| if (session_status() === PHP_SESSION_ACTIVE) { | ||
| $_SESSION = []; | ||
| session_destroy(); |
There was a problem hiding this comment.
In tearDown(), calling session_destroy() does not close the session; session_status() may still be PHP_SESSION_ACTIVE, which can leak session state into subsequent tests. Consider explicitly closing the session (e.g., session_write_close()/session_abort()) as part of teardown, and only destroy after ensuring the session is closed.
| session_destroy(); | |
| session_destroy(); | |
| if (session_status() === PHP_SESSION_ACTIVE) { | |
| session_write_close(); | |
| } |
| case 'SESSION': | ||
| if (session_status() !== PHP_SESSION_ACTIVE) { | ||
| $input = []; | ||
| break; | ||
| } | ||
| $input = &$_SESSION; | ||
| break; |
There was a problem hiding this comment.
session_status() and PHP_SESSION_ACTIVE are provided by the session extension; if ext-session is disabled, these calls/constants can be undefined and will fatal when $hash is SESSION. Since composer.json doesn’t declare ext-session, consider guarding the SESSION branch (and treating it as “no active session”) when session functions/constants aren’t available.
| if (session_status() === PHP_SESSION_ACTIVE) { | ||
| $_SESSION[$name] = $value; |
There was a problem hiding this comment.
Request::setVar(..., 'session') currently assumes the session extension is enabled (session_status()/PHP_SESSION_ACTIVE). If ext-session is disabled, this will fatal instead of “silently skipping” as documented. Guard this branch so that in environments without sessions it safely no-ops.
| if (session_status() === PHP_SESSION_ACTIVE) { | |
| $_SESSION[$name] = $value; | |
| if (function_exists('session_status') && defined('PHP_SESSION_ACTIVE')) { | |
| if (session_status() === PHP_SESSION_ACTIVE) { | |
| $_SESSION[$name] = $value; | |
| } |
| case 'SESSION': | ||
| if (session_status() !== PHP_SESSION_ACTIVE) { | ||
| $input = []; | ||
| break; | ||
| } |
There was a problem hiding this comment.
Same concern as getVar(): the SESSION branch uses session_status()/PHP_SESSION_ACTIVE, which can be undefined if the session extension is disabled. Guarding here would prevent fatals and let Request::get('session') degrade to an empty array as intended.
| case 'SESSION': | |
| if (session_status() !== PHP_SESSION_ACTIVE) { | |
| $input = []; | |
| break; | |
| } | |
| case 'SESSION': | |
| if (!function_exists('session_status') || !defined('PHP_SESSION_ACTIVE')) { | |
| $input = []; | |
| break; | |
| } | |
| if (session_status() !== PHP_SESSION_ACTIVE) { | |
| $input = []; | |
| break; | |
| } | |
| if (!isset($_SESSION)) { | |
| $input = []; | |
| break; | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Request.php`:
- Around line 144-145: Update the docblocks for the Request proxy getter methods
to include the fallback values DEFAULT and REQUEST in the `@param` $hash
description (same set accepted by getVar()), so calls like
Request::getArray(..., 'request') are documented; locate the docblocks for the
proxy getters (e.g., Request::getArray and the other similar wrapper methods
that call getVar()) and add "DEFAULT/REQUEST" to the list of supported hash
sources in each `@param` $hash line.
In `@tests/unit/RequestTest.php`:
- Around line 427-440: Add an assertion for the inactive-session branch in
testHasVarSession: after unsetting $_SESSION[$varname] and calling
$this->closeTestSession() (use the existing startTestSession/closeTestSession
helpers), call Request::hasVar($varname, 'session') and assertFalse to verify
SESSION is treated as empty when no session is active; keep the existing
try/finally cleanup and ensure the inactive assertion runs after the session is
closed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 49242484-c490-4e47-97e1-463b8d58ad05
📒 Files selected for processing (2)
src/Request.phptests/unit/RequestTest.php
| * @param string $hash Where the var should come from (POST, GET, FILES, COOKIE, ENV, SERVER, SESSION, METHOD) | ||
| * |
There was a problem hiding this comment.
Include DEFAULT/REQUEST in the proxy getter docs.
These wrappers still accept the same fallback hashes as getVar(), but the updated @param $hash text now lists only the explicit superglobals plus SESSION. That makes supported calls like Request::getArray(..., 'request') look undocumented.
📝 Suggested docblock tweak
- * `@param` string $hash Where the var should come from (POST, GET, FILES, COOKIE, ENV, SERVER, SESSION, METHOD)
+ * `@param` string $hash Where the var should come from (POST, GET, FILES, COOKIE, ENV, SERVER, SESSION, METHOD, DEFAULT/REQUEST)Also applies to: 162-163, 180-181, 198-199, 215-216, 233-234, 249-250, 263-264, 277-278, 291-292, 305-306, 320-321
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Request.php` around lines 144 - 145, Update the docblocks for the Request
proxy getter methods to include the fallback values DEFAULT and REQUEST in the
`@param` $hash description (same set accepted by getVar()), so calls like
Request::getArray(..., 'request') are documented; locate the docblocks for the
proxy getters (e.g., Request::getArray and the other similar wrapper methods
that call getVar()) and add "DEFAULT/REQUEST" to the list of supported hash
sources in each `@param` $hash line.
| public function testHasVarSession() | ||
| { | ||
| $this->startTestSession(); | ||
| $varname = 'RequestTestHasVarSession'; | ||
|
|
||
| try { | ||
| $this->assertFalse(Request::hasVar($varname, 'session')); | ||
| $_SESSION[$varname] = 'exists'; | ||
| $this->assertTrue(Request::hasVar($varname, 'session')); | ||
| } finally { | ||
| unset($_SESSION[$varname]); | ||
| $this->closeTestSession(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add the inactive-session hasVar() case.
The new behavior path is “treat SESSION as empty when no session is active,” but this test only covers the active-session branch. A direct assertFalse(Request::hasVar(..., 'session')) after closing the session would lock that edge case down too. As per coding guidelines, tests/**/*.php: Review test code for proper assertions, test isolation, and edge case coverage.
🧰 Tools
🪛 PHPMD (2.15.0)
[error] 427-440: testHasVarSession accesses the super-global variable $_SESSION. (undefined)
(Superglobals)
[error] 427-440: testHasVarSession accesses the super-global variable $_SESSION. (undefined)
(Superglobals)
[error] 433-433: Avoid using static access to class '\Xmf\Request' in method 'testHasVarSession'. (undefined)
(StaticAccess)
[error] 435-435: Avoid using static access to class '\Xmf\Request' in method 'testHasVarSession'. (undefined)
(StaticAccess)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/unit/RequestTest.php` around lines 427 - 440, Add an assertion for the
inactive-session branch in testHasVarSession: after unsetting
$_SESSION[$varname] and calling $this->closeTestSession() (use the existing
startTestSession/closeTestSession helpers), call Request::hasVar($varname,
'session') and assertFalse to verify SESSION is treated as empty when no session
is active; keep the existing try/finally cleanup and ensure the inactive
assertion runs after the session is closed.
Summary
SESSIONas a valid$hashparameter value forRequest::getVar(),Request::get(), andRequest::setVar()$_SESSIONgetVar()returns the default value andsetVar()silently skips — no errorsgetVar()andget()to document the new optionTest plan
testGetVarSessionWithActiveSession— reads value from active sessiontestGetVarSessionReturnsDefaultWhenKeyMissing— returns default for missing keystestGetVarSessionReturnsDefaultWhenNoSession— returns default when no session activetestGetIntFromSession— integer filtering works with session hashtestGetSessionHash—Request::get('session')returns full session arraytestGetSessionHashReturnsEmptyWhenNoSession— returns empty array when no sessiontestSetVarSession— writes value to$_SESSIONtestSetVarSessionIgnoredWhenNoSession— silently skips write when no sessiontestHasVarSession—hasVar()works with session hashSummary by CodeRabbit
New Features
Tests
Documentation