Contrib.social

Create a test to test the logic of SessionManager::regenerate()

Open on Drupal.org →
Created on 4 April 2019, about 6 years ago
Updated 24 January 2023, about 2 years ago

Problem/Motivation

In #3045349: Lazy started sessions with session data are not saved with symfony/http-foundation 3.4.24 → We were forced to change the logic of SessionManager::isStarted() to account for vendor changes upstream. Specifically it had previously returned true if the session had been started and false if the session had been lazy started, but the vendor change meant that it now needed to return true in both cases. When we did so, all tests came back green, however we noticed a performance issue with the SessionManager::regenerate() method, which relied on the ->isStarted() logic to choose a code path. It was saving lazy sessions when it didn't need to and the logic in that method needed to be reverted so that it saved only if the session was started and not lazy started. 3045349 was critical and needed a fast commit, this issue is a followup to add a test for the regenerate() logic.

Proposed resolution

Create a unit test that would fail if SessionManager::regenerate() saved a lazy session.

Remaining tasks

Do it

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

📌 Task
Status

Postponed: needs info

Version

10.1 ✨

Component
Base  →

Last updated about 1 hour ago

Created by

🇺🇸United States mikelutz Michigan, USA

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

Not all content is available!

It's likely this issue predates Contrib.social: some issue and comment data are missing.

  • Comment about 2 years ago →
    🇺🇸United States smustgrave

    This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request → as a guide.

    -    session_id(Crypt::randomBytesBase64());
+    $this->setId(Crypt::randomBytesBase64());

    This code appears to have been removed so question is this task still needed?
    Can move over credit if needed to the issue it was removed in if not.

  • Comment 19 days ago →
    🇺🇸United States smustgrave

    Wonder if still a needed task? If no follow up could be closed in 3 months

  • Comment 19 days ago →
    🇺🇸United States mikelutz Michigan, USA

    I think we can close this. We ended up reverting the patch that surfaced this issue anyways, and I don't know that it needs an explicit test anymore.

contrib.social Blog FAQ Discussions
Production build 0.71.5 2024