Skip to content
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.

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

Revert removal of checks for the SID constant deprecated in PHP 8.4 #98

Merged
merged 9 commits into from
Dec 19, 2024

Conversation

mimmi20
Copy link
Contributor

@mimmi20 mimmi20 commented Dec 11, 2024

Q A
Documentation no
Bugfix yes
BC Break no
New Feature no
RFC no
QA no

Fixes #97

Description

In Release 2.22.0 the check for the Constant SID was removed. This lead to the error that the Session could not be started when a session ID was set.

renovate bot and others added 4 commits November 25, 2024 00:44
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
@mimmi20
Copy link
Contributor Author

mimmi20 commented Dec 11, 2024

@froschdesign @gsteel There are two tests failing now (testSessionExistsReturnsTrueWhenSessionStartedThenWritten and testCallingWriteCloseShouldNotAlterSessionExistsStatus).

I dont unserstand why session_write_close shall not change the Session-exists-Status.

PHP's page states

session_write_close
(PHP 4 >= 4.0.4, PHP 5, PHP 7, PHP 8)

session_write_close — Write session data and end session

src/SessionManager.php Outdated Show resolved Hide resolved
@gsteel
Copy link
Member

gsteel commented Dec 11, 2024

I dont unserstand why session_write_close shall not change the Session-exists-Status.

Closing the session persists the session data for the next request, it doesn't kill the session; There is still an active session once it has been written to storage.

To kill the active session, it must be destroyed with session_destroy() or $manager->destroy()

Signed-off-by: Thomas Müller <[email protected]>
Signed-off-by: Thomas Müller <[email protected]>
Signed-off-by: Thomas Müller <[email protected]>
Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

The tests that now emit deprecations on 8.4 can be silenced by annotating the test with #[IgnoreDeprecations]

test/SessionManagerTest.php Show resolved Hide resolved
Signed-off-by: Thomas Müller <[email protected]>
Signed-off-by: Thomas Müller <[email protected]>
@mimmi20 mimmi20 requested a review from gsteel December 11, 2024 19:19
@mimmi20
Copy link
Contributor Author

mimmi20 commented Dec 19, 2024

@gsteel Please review again.

Copy link
Member

@gsteel gsteel left a comment

Choose a reason for hiding this comment

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

Thanks @mimmi20

I'm going to merge this as-is. Reverting the previously deleted lines in #94 hopefully fixes your problem.

There's still no test here that fails when the SID constant is defined and the conditional removed, so I'd appreciate it if you could let us know how/why this fixed your issue.

@gsteel gsteel changed the base branch from 2.23.x to 2.22.x December 19, 2024 10:44
@gsteel gsteel changed the title remove id check when checking for started session Revert removal of checks for the SID constant deprecated in PHP 8.4 Dec 19, 2024
@gsteel gsteel added this to the 2.21.1 milestone Dec 19, 2024
@gsteel gsteel added BC Break Bug Something isn't working and removed BC Break labels Dec 19, 2024
@gsteel gsteel self-assigned this Dec 19, 2024
@gsteel gsteel merged commit 0c2b84c into laminas:2.22.x Dec 19, 2024
14 checks passed
@gsteel gsteel modified the milestones: 2.21.1, 2.22.1 Dec 19, 2024
@gsteel gsteel mentioned this pull request Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Can not start Session
2 participants