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

Add auto_regenerate option to CacheSessionPersistence #51

Merged
merged 7 commits into from
May 7, 2024
Merged

Add auto_regenerate option to CacheSessionPersistence #51

merged 7 commits into from
May 7, 2024

Conversation

michal-izewski
Copy link
Contributor

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

Description

Allow to turn off automatic session ID regeneration on each session data change.

  • Why should it be added?
    • In some custom implementations (for example, when a session is shared between two or more backend applications written in different languages) there may be a need to handle session ID regeneration manually and allow session data to be changed without regenerating ID, it gives more flexibility of implementation.
  • What will it enable?
    • Change allows to turn off automatic session ID regeneration when session data changes but leaves it on as a default behavior to keep BC

@gsteel
Copy link
Member

gsteel commented May 7, 2024

Can you please add tests to cover behaviour when $autoRegenerate is false?

As this is similar to #44 but without the BC problems, I'll defer to @weierophinney for review.

Fixing the psalm issues in the factory will require verifying that $config['auto_regenerate'] is either bool | null, or adding assert(is_bool($autoRegenerate)) directly after the assignment.

@michal-izewski
Copy link
Contributor Author

Can you please add tests to cover behaviour when $autoRegenerate is false?

As this is similar to #44 but without the BC problems, I'll defer to @weierophinney for review.

Fixing the psalm issues in the factory will require verifying that $config['auto_regenerate'] is either bool | null, or adding assert(is_bool($autoRegenerate)) directly after the assignment.

Thanks for the comment, I've updated both unit tests and the psalm baseline to cover the auto_regenerate option

Michal Izewski added 2 commits May 7, 2024 14:55
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.

Thank you @michal-izewski - This looks good to me with a couple of minor tweaks 👍

src/CacheSessionPersistence.php Outdated Show resolved Hide resolved
test/CacheSessionPersistenceIntegrationTest.php Outdated Show resolved Hide resolved
@gsteel gsteel requested a review from weierophinney May 7, 2024 14:26
@gsteel gsteel added this to the 1.13.0 milestone May 7, 2024
Co-authored-by: George Steel <[email protected]>
Signed-off-by: Michał Iżewski <[email protected]>
Copy link
Contributor

@weierophinney weierophinney left a comment

Choose a reason for hiding this comment

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

One minor tweak suggested, to give a little more type certainty in the factory. Otherwise, this looks good! I'm happy with an opt-in solution for this; the regeneration was done for a defense-in-depth approach, but obviously there are cases where this is not desired. Leaving the default as regeneration will work well for this.

@@ -46,7 +47,8 @@ public function __invoke(ContainerInterface $container)
$cookieDomain,
$cookieSecure,
$cookieHttpOnly,
$cookieSameSite
$cookieSameSite,
$autoRegenerate
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd cast this to boolean here (and do the same for $cookieSameSite while you're at it):

(bool) $cookieSameSite,
(bool) $autoRegenerate,

This will remove one more baseline item, and prevent some common errors.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've introduced type casting for all boolean arguments (as suggested) - $cookieSameSite is expected to be a string so I left it as is for now

Signed-off-by: Michal Izewski <[email protected]>
@weierophinney weierophinney added Enhancement New feature or request and removed Unit Test Needed labels May 7, 2024
@weierophinney weierophinney merged commit f57890f into mezzio:1.13.x May 7, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants