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 4 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
1 change: 1 addition & 0 deletions phpstan.neon
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ parameters:
excludePaths:
- tests/
treatPhpDocTypesAsCertain: false
reportUnmatchedIgnoredErrors: false
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 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).

Copilot uses AI. Check for mistakes.
ignoreErrors:
-
message: '#Trait .+ is used zero times and is not analysed\.#'
Expand Down
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
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();
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.

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

Copilot uses AI. Check for mistakes.
if (session_status() !== PHP_SESSION_ACTIVE) {
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.

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.

Suggested change
session_start();
if (session_status() !== PHP_SESSION_ACTIVE) {
$started = @session_start();
if ($started === false || session_status() !== PHP_SESSION_ACTIVE) {

Copilot uses AI. Check for mistakes.
$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();
}
$this->assertNotSame(
PHP_SESSION_ACTIVE,
session_status(),
'Session should not be active after close.'
);
}
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->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'));
}

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

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 +427 to +440
Copy link

@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