Skip to content

N°9625 - Add Session::UnsetAll() method to ensure that even the "welcome" session value is correctly resetted#970

Open
Molkobain wants to merge 4 commits into
developfrom
issue/9625-add-session-unsetall-helper
Open

N°9625 - Add Session::UnsetAll() method to ensure that even the "welcome" session value is correctly resetted#970
Molkobain wants to merge 4 commits into
developfrom
issue/9625-add-session-unsetall-helper

Conversation

@Molkobain

Copy link
Copy Markdown
Member

Internal

@Molkobain Molkobain added this to the 3.3.0 milestone Jul 10, 2026
@Molkobain Molkobain requested review from dflaven, eespie and rquetiez July 10, 2026 08:05
@Molkobain Molkobain self-assigned this Jul 10, 2026
Copilot AI review requested due to automatic review settings July 10, 2026 08:05
@Molkobain Molkobain added the bug Something isn't working label Jul 10, 2026
@CombodoApplicationsAccount CombodoApplicationsAccount added the internal Work made by Combodo label Jul 10, 2026

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Adds comprehensive session clearing so logout also resets values such as welcome.

Changes:

  • Adds Session::UnsetAll().
  • Uses it during login session reset.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
sources/Service/Session/Session.php Adds full-session reset API.
application/loginwebpage.class.inc.php Clears all session variables on reset.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread sources/Service/Session/Session.php
Comment thread sources/Service/Session/Session.php
@greptile-apps

greptile-apps Bot commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR fixes a bug where the welcome session flag (set by DisplayWelcomePopup() in pages/UI.php) was not cleared on logout because LoginWebPage::ResetSession() only unsetting ten hard-coded session keys. The fix introduces Session::UnsetAll(), which clears $_SESSION wholesale, and replaces the explicit-key loop in ResetSession() with a single call to that method.

  • Session::UnsetAll() mirrors the pattern of Session::Set(): when the session is not active it starts one, clears $_SESSION, then writes-and-closes; when already active it just assigns [].
  • Session::ListVariables() receives a defensive !isset($_SESSION) guard consistent with other methods in the class, addressing the concern raised in a prior review.
  • loginwebpage.class.inc.php swaps the Application\\Helper\\Session alias import for the canonical Service\\Session\\Session namespace; both resolve to the same class via the class_alias in sources/alias.php.

Confidence Score: 5/5

Safe to merge — the change is a targeted, well-scoped fix with no functional regressions introduced.

The new UnsetAll() method follows the existing Set() pattern exactly, the ListVariables() guard addresses the previously raised concern, and the behavioral broadening of ResetSession() (clearing all keys instead of ten named ones) is intentional and correct. The only gap is that the test covers only the active-session code path.

No files require special attention.

Important Files Changed

Filename Overview
sources/Service/Session/Session.php Adds UnsetAll() (clears $_SESSION entirely, consistent with Set() pattern) and adds a missing !isset($_SESSION) guard to ListVariables(). Both changes are correct and consistent with existing class conventions.
application/loginwebpage.class.inc.php ResetSession() now delegates to Session::UnsetAll() instead of unsetting 10 hard-coded keys; broadens the clear to include previously missed keys (e.g. welcome, transactions). Import updated from the alias to the canonical service namespace.
tests/php-unit-tests/unitary-tests/application/Session/SessionTest.php testUnsetAll() covers the active-session branch only; the inactive-session branch (Start → clear → WriteClose) is untested.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Browser
    participant LoginWebPage
    participant Session
    participant PHP

    Browser->>LoginWebPage: logout / re-login trigger
    LoginWebPage->>Session: UnsetAll()
    alt session already active
        Session->>PHP: "$_SESSION = []"
    else session not yet active
        Session->>PHP: session_start()
        Session->>PHP: "$_SESSION = []"
        Session->>PHP: session_write_close()
    end
    LoginWebPage->>LoginWebPage: UserRights::_ResetSessionCache()
    Note over LoginWebPage,Session: welcome, auth_user, transactions,<br/>and all other keys are now cleared
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Browser
    participant LoginWebPage
    participant Session
    participant PHP

    Browser->>LoginWebPage: logout / re-login trigger
    LoginWebPage->>Session: UnsetAll()
    alt session already active
        Session->>PHP: "$_SESSION = []"
    else session not yet active
        Session->>PHP: session_start()
        Session->>PHP: "$_SESSION = []"
        Session->>PHP: session_write_close()
    end
    LoginWebPage->>LoginWebPage: UserRights::_ResetSessionCache()
    Note over LoginWebPage,Session: welcome, auth_user, transactions,<br/>and all other keys are now cleared
Loading

Reviews (3): Last reviewed commit: "N°9625 - Code review updates" | Re-trigger Greptile

Comment thread sources/Service/Session/Session.php
@Molkobain

Copy link
Copy Markdown
Member Author

@greptileai

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Session::Unset('itop_env');
Session::Unset('obj_messages');
Session::Unset('profile_list');
Session::UnsetAll();

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

@odain-cbd @eespie do you think that we should preserve login_mode entry in the session instead of clearing everything?

Comment thread sources/Service/Session/Session.php
@Molkobain Molkobain requested a review from odain-cbd July 10, 2026 10:07
@Molkobain Molkobain requested a review from Copilot July 10, 2026 11:50
@Molkobain

Copy link
Copy Markdown
Member Author

@greptileai

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

Comment thread sources/Service/Session/Session.php
Comment thread application/loginwebpage.class.inc.php
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working internal Work made by Combodo

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants