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 a WP Cache session handler #760

Merged
merged 3 commits into from
May 4, 2023

Conversation

kovshenin
Copy link
Contributor

Adds support for PHP sessions with our Afterburner implementation of the WordPress object cache.

Adds support for PHP sessions with our Afterburner implementation
of the WordPress object cache.
@kovshenin kovshenin requested review from rmccue, joehoyle and a team April 28, 2023 13:53
@kovshenin kovshenin self-assigned this Apr 28, 2023
*/
public function read( string $id ) : string | false {
$data = wp_cache_get( $id, 'sessions' );
if ( ! $data ) {
Copy link
Member

Choose a reason for hiding this comment

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

This may want to use $found? or I guess if it's only ever a string, it should be safe enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it throws a warning if you return anything that's not a string (probably because open() succeeded), so even for a session that was not found we have to return an empty string. We know wp_cache_get() will return false if the value doesn't exist, because $data is a string in write() but that still should be returned as an empty string to avoid the warning.

I believe this is because open() is typically responsible for determining whether the session exists, i.e. tries to open a file descriptor, so when a descriptor is open, read() is almost guaranteed to succeed. Maybe moving wp_cache_get() to open(), storing the value in a property and then returning it in read()? Maybe that's more semantically accurate, but that makes things more confusing, especially given that the Redis connection is abstracted anyway. Forcing a string is a good tradeoff I think.

@joehoyle
Copy link
Member

joehoyle commented May 1, 2023

Very cool! I think the linter issues need fixing up.

Copy link
Member

@joehoyle joehoyle left a comment

Choose a reason for hiding this comment

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

Nice, looks good

@kovshenin kovshenin merged commit ff2c119 into afterburner-support May 4, 2023
@kovshenin kovshenin deleted the wp-cache-session-handler branch May 4, 2023 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants