Skip to content

Add SESSION hash support to Request class #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mambax7 merged 15 commits into from
Apr 7, 2026
Merged

Add SESSION hash support to Request class #147

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
69edcc8
Add SESSION hash support to Request::getVar(), get(), and setVar()
mambax7 Mar 11, 2026
2fcef08
Improve SESSION tests and complete Request docblocks
mambax7 Mar 11, 2026
ae74064
Fix SESSION tests for CLI/PHPUnit environments
mambax7 Mar 11, 2026
eec0b4d
Add reportUnmatchedIgnoredErrors: false to phpstan.neon
mambax7 Mar 11, 2026
e3cb224
Updates
mambax7 Mar 11, 2026
357383b
Restore PHPStan guardrails
mambax7 Apr 6, 2026
37bed60
Revert " Restore PHPStan guardrails"
mambax7 Apr 6, 2026
cffd7f1
Fix Request session review feedback
mambax7 Apr 6, 2026
fec4a7f
Fix review config and session test setup
mambax7 Apr 6, 2026
b6aff13
Align CodeRabbit config with supported PHP versions
mambax7 Apr 6, 2026
b7c3c20
Fix session read and teardown review issues
mambax7 Apr 6, 2026
6d9b77d
Fix session test isolation and CI drift
mambax7 Apr 7, 2026
7f27e12
Support webmozart assert 2 in tests
mambax7 Apr 7, 2026
ccbe227
Fix PR 147 review feedback
mambax7 Apr 7, 2026
a3a41e0
Remove stale Request phpstan ignores
mambax7 Apr 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 27 additions & 4 deletions src/Request.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,12 +64,13 @@ public static function getMethod()
* - 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
Comment on lines -62 to 76
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
* information see FilterInput::clean().
Expand Down Expand Up @@ -106,6 +107,13 @@ public static function getVar($name, $default = null, $hash = 'default', $type =
case 'SERVER':
$input = &$_SERVER;
break;
case 'SESSION':
if (session_status() !== PHP_SESSION_ACTIVE) {
$input = [];
break;
}
$input = &$_SESSION;
break;
Comment on lines +111 to +117
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
default:
$input = &$_REQUEST;
break;
Expand Down Expand Up @@ -385,9 +393,11 @@ public static function hasVar($name, $hash = 'default')
/**
* Set a variable in one of the request variables
*
* For SESSION, the write is silently skipped if no session is active.
*
* @param string $name Name
* @param string $value Value
* @param string $hash Hash
* @param string $hash Hash (GET, POST, REQUEST, COOKIE, FILES, ENV, SERVER, SESSION, METHOD)
Comment on lines 397 to +399
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

setVar() accepts $value = null and the method is used to set values into request/session hashes, so the PHPDoc @param string $value is inaccurate and can mislead static analysis/IDE type inference. Update the PHPDoc to reflect the actual accepted type (e.g., mixed).

Copilot uses AI. Check for mistakes.
* @param bool $overwrite Boolean
*
* @return string Previous value
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

The PHPDoc for setVar() says @return string Previous value, but the implementation returns null when the key did not previously exist (and may return non-string values depending on what was stored). Adjust the return PHPDoc to match the actual return type (e.g., mixed).

Suggested change
* @return string Previous value
* @return mixed Previous value

Copilot uses AI. Check for mistakes.
Expand Down Expand Up @@ -437,6 +447,11 @@ public static function setVar($name, $value = null, $hash = 'method', $overwrite
case 'SERVER':
$_SERVER[$name] = $value;
break;
case 'SESSION':
if (session_status() === PHP_SESSION_ACTIVE) {
$_SESSION[$name] = $value;
Comment on lines +450 to +451
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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;
}

Copilot uses AI. Check for mistakes.
}
break;
Comment on lines +449 to +453
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}

return $previous;
Expand All @@ -457,10 +472,11 @@ public static function setVar($name, $value = null, $hash = 'method', $overwrite
* - cookie $_COOKIE
* - env $_ENV
* - server $_SERVER
* - session $_SESSION (returns empty if no active session)
* - method via current $_SERVER['REQUEST_METHOD']
* - default $_REQUEST
*
* @param string $hash to get (POST, GET, FILES, METHOD)
* @param string $hash to get (GET, POST, FILES, COOKIE, ENV, SERVER, SESSION, METHOD, DEFAULT/REQUEST)
* @param int $mask Filter mask for the variable
*
* @return mixed Request hash
Expand Down Expand Up @@ -492,6 +508,13 @@ public static function get($hash = 'default', $mask = 0)
case 'SERVER':
$input = &$_SERVER;
break;
case 'SESSION':
if (session_status() !== PHP_SESSION_ACTIVE) {
$input = [];
break;
}
Comment on lines +510 to +514
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
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;
}

Copilot uses AI. Check for mistakes.
$input = &$_SESSION;
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

Request::get('session') assigns $input by reference to $_SESSION and then calls cleanVars(), which iterates arrays by reference and will therefore mutate $_SESSION in-place (e.g., trimming/cleaning values) when callers fetch the session hash (including via hasVar()/setVar() which call get(..., MASK_ALLOW_RAW)). This introduces surprising side effects/data loss; use a copy of $_SESSION here (like GET/POST) so reads don’t modify session storage.

Suggested change
$input = &$_SESSION;
$input = $_SESSION;

Copilot uses AI. Check for mistakes.
break;
default:
$input = $_REQUEST;
break;
Expand All @@ -506,7 +529,7 @@ public static function get($hash = 'default', $mask = 0)
* Sets a request variable
*
* @param array $array An associative array of key-value pairs
* @param string $hash The request variable to set (POST, GET, FILES, METHOD)
* @param string $hash The request variable to set (GET, POST, REQUEST, COOKIE, FILES, ENV, SERVER, SESSION, METHOD)
* @param bool $overwrite If true and an existing key is found, the value is overwritten,
* otherwise it is ignored
*
Expand Down
159 changes: 159 additions & 0 deletions tests/unit/RequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -257,4 +257,163 @@ public function testSet()
$this->assertEquals($_REQUEST[$varname], 'Pourquoi');
}

/**
* Attempt to start a session for testing.
*
* Disables cookie-based session IDs (not available in CLI), starts the
* session, and verifies it became active. Skips the calling test if
* sessions cannot be started in this environment.
*/
private function startTestSession(): void
{
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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).

Suggested change
{
{
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());
}

Copilot uses AI. Check for mistakes.
if (session_status() === PHP_SESSION_ACTIVE) {
return;
}
ini_set('session.use_cookies', '0');
ini_set('session.use_only_cookies', '0');
ini_set('session.cache_limiter', '');
session_start();
Comment thread
mambax7 marked this conversation as resolved.
Outdated
if (session_status() !== PHP_SESSION_ACTIVE) {
Comment thread
mambax7 marked this conversation as resolved.
Outdated
$this->markTestSkipped('Cannot start a session in this environment.');
}
}

/**
* Close any active session and verify it is no longer active.
*/
private function closeTestSession(): void
{
if (session_status() === PHP_SESSION_ACTIVE) {
session_write_close();
}
Comment on lines +332 to +336
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

session_set_save_handler() changes the global session save handler for the entire PHP process, but closeTestSession() never restores the previous/default handler. This can leak state into other test classes that might call session_start() later. Consider restoring the handler (e.g., to a new \SessionHandler() or by capturing/restoring the prior handler configuration) when closing the test session.

Copilot uses AI. Check for mistakes.
$this->assertNotSame(
PHP_SESSION_ACTIVE,
session_status(),
'Session should not be active after close.'
);
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +332 to +345
Copy link

Copilot AI Apr 6, 2026

Choose a reason for hiding this comment

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

closeTestSession() uses session_destroy() and then immediately calls session_id('') and asserts the session is no longer active. In PHP, session_destroy() destroys data but does not reliably close/end the session, and changing the session id while active can emit warnings. Consider explicitly closing the session (e.g., committing/closing) before resetting the id, and restoring the original save handler if needed to keep the test isolated.

Copilot uses AI. Check for mistakes.
}
Comment on lines +330 to +346
Copy link

Copilot AI Mar 11, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

public function testGetVarSessionWithActiveSession()
{
$this->startTestSession();
$varname = 'RequestTestSession';
$_SESSION[$varname] = 'session_value';

try {
$this->assertEquals('session_value', Request::getVar($varname, null, 'session'));
} finally {
unset($_SESSION[$varname]);
$this->closeTestSession();
}
}

public function testGetVarSessionReturnsDefaultWhenKeyMissing()
{
$this->startTestSession();

try {
$this->assertNull(Request::getVar('no_such_session_key', null, 'session'));
$this->assertEquals('fallback', Request::getVar('no_such_session_key', 'fallback', 'session'));
} finally {
$this->closeTestSession();
}
}

public function testGetVarSessionReturnsDefaultWhenNoSession()
{
$this->startTestSession();
$this->closeTestSession();

$this->assertNull(Request::getVar('any_key', null, 'session'));
$this->assertEquals('default_val', Request::getVar('any_key', 'default_val', 'session'));
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

public function testGetIntFromSession()
{
$this->startTestSession();
$varname = 'RequestTestSessionInt';
$_SESSION[$varname] = '42';

try {
$this->assertEquals(42, Request::getInt($varname, 0, 'session'));
} finally {
unset($_SESSION[$varname]);
$this->closeTestSession();
}
}

public function testGetSessionHash()
{
$this->startTestSession();
$varname = 'RequestTestSessionGet';
$_SESSION[$varname] = 'get_session_value';

try {
$get = Request::get('session');
$this->assertTrue(is_array($get));
$this->assertEquals('get_session_value', $get[$varname]);
} finally {
unset($_SESSION[$varname]);
$this->closeTestSession();
}
}

public function testGetSessionHashReturnsEmptyWhenNoSession()
{
$this->startTestSession();
$this->closeTestSession();

$get = Request::get('session');
$this->assertTrue(is_array($get));
$this->assertEmpty($get);
}

public function testSetVarSession()
{
$this->startTestSession();
$varname = 'XMF_TEST_SESSION_VAR';
$value = 'session_set_value';

try {
Request::setVar($varname, $value, 'session');
$this->assertArrayHasKey($varname, $_SESSION);
$this->assertEquals($value, $_SESSION[$varname]);
} finally {
unset($_SESSION[$varname]);
$this->closeTestSession();
}
}

public function testSetVarSessionIgnoredWhenNoSession()
{
$this->startTestSession();
$this->closeTestSession();

$varname = 'XMF_TEST_SESSION_NO_WRITE';
Request::setVar($varname, 'should_not_persist', 'session');

// Start a fresh session and verify nothing leaked
$this->startTestSession();
try {
$this->assertArrayNotHasKey($varname, $_SESSION);
} finally {
Comment thread
mambax7 marked this conversation as resolved.
Outdated
Comment on lines +446 to +458
Copy link

Copilot AI Apr 7, 2026

Choose a reason for hiding this comment

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

testSetVarSessionIgnoredWhenNoSession currently re-opens a session via startTestSession(), which resets $_SESSION to an empty array. That means the test can pass even if Request::setVar() incorrectly writes into $_SESSION while no session is active. Add an assertion immediately after the setVar() call (before re-opening a session) that the key is not present in $_SESSION, and/or avoid resetting $_SESSION in the verification step.

Copilot uses AI. Check for mistakes.
$this->closeTestSession();
}
}

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();
}
}
Comment on lines +463 to +478
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Mar 11, 2026

Choose a reason for hiding this comment

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

🧹 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.


}
Loading