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

Open
mambax7 wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add SESSION hash support to Request class #147

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 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
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 -61 to 75
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 +110 to +116
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)
* @param bool $overwrite Boolean
*
* @return string Previous value
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 +449 to +450
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 +448 to +452
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 +509 to +513
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;
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
116 changes: 116 additions & 0 deletions tests/unit/RequestTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,10 @@ class RequestTest extends \PHPUnit\Framework\TestCase
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();
}
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.

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.

Copilot uses AI. Check for mistakes.
}

/**
Expand All @@ -25,6 +29,10 @@ protected function setUp(): void
*/
protected function tearDown(): void
{
// Restore active session state after each test
if (session_status() !== PHP_SESSION_ACTIVE) {
session_start();
}
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.

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.

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

Copilot uses AI. Check for mistakes.
}

public function testGetMethod()
Expand Down Expand Up @@ -257,4 +265,112 @@ public function testSet()
$this->assertEquals($_REQUEST[$varname], 'Pourquoi');
}

/**
* Helper to ensure an active session, skipping the test if unavailable.
*/
private function requireActiveSession(): 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) {
$this->markTestSkipped('Cannot start a session in this environment.');
}
}
Comment on lines +306 to +317
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->requireActiveSession();
$varname = 'RequestTestSession';
$_SESSION[$varname] = 'session_value';

$this->assertEquals('session_value', Request::getVar($varname, null, 'session'));

unset($_SESSION[$varname]);
}

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

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

public function testGetVarSessionReturnsDefaultWhenNoSession()
{
$this->requireActiveSession();
session_write_close();

$this->assertNull(Request::getVar('any_key', null, 'session'));
$this->assertEquals('default_val', Request::getVar('any_key', 'default_val', 'session'));
// tearDown() restores the session
}

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

$this->assertEquals(42, Request::getInt($varname, 0, 'session'));

unset($_SESSION[$varname]);
}

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

$get = Request::get('session');
$this->assertTrue(is_array($get));
$this->assertEquals('get_session_value', $get[$varname]);

unset($_SESSION[$varname]);
}

public function testGetSessionHashReturnsEmptyWhenNoSession()
{
$this->requireActiveSession();
session_write_close();

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

public function testSetVarSession()
{
$this->requireActiveSession();
$varname = 'XMF_TEST_SESSION_VAR';
$value = 'session_set_value';
Request::setVar($varname, $value, 'session');
$this->assertArrayHasKey($varname, $_SESSION);
$this->assertEquals($value, $_SESSION[$varname]);
unset($_SESSION[$varname]);
}

public function testSetVarSessionIgnoredWhenNoSession()
{
$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);
}
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.

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.

Copilot uses AI. Check for mistakes.

public function testHasVarSession()
{
$this->requireActiveSession();
$varname = 'RequestTestHasVarSession';
$this->assertFalse(Request::hasVar($varname, 'session'));
$_SESSION[$varname] = 'exists';
$this->assertTrue(Request::hasVar($varname, 'session'));
unset($_SESSION[$varname]);
}
Comment on lines +427 to +440
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