humanmade / altis-core

Core Module for Altis
https://www.altis-dxp.com/
20 stars 4 forks source link

Add PHP session handler with storage in Redis #96

Closed roborourke closed 3 years ago

roborourke commented 4 years ago

We want to add support for PHP sessions in multi-server configuration. By default, PHP stores sessions on the disk, which won't work properly with autoscaling. The typical place these are offloaded is into the object cache, i.e. Redis.

This is useful for ecommerce sites which use sessions for storing cart information, but is only very rarely used outside of these sites.

One option is WP Session Manager, a plugin that enables support for PHP sessions using Redis.

https://github.com/ericmann/wp-session-manager

This would have implications for the cacheability of the site, and we need to check that session IDs are correctly passed through the CDN to the backend. This may require us to reconfigure the cookie name in PHP (by default SESSIONID I think?).

We should include this capability and provide the Redis integration if it is enabled in the cloud module.

shadyvb commented 3 years ago

As per @rmccue in, Predis ( a library we're already using on Altis Cloud ) does provide a built-in session handler, so it's worth checking that out first, also Pantheon's plugin WP Native PHP Sessions is another viable option.

shadyvb commented 3 years ago

In https://github.com/humanmade/altis-cloud/pull/440 I've added support for Predis session handler, but will need a follow-up PR on humanmade/docker-wordpress-php to remove the php.ini line disabling sessions altogether, as it cannot be modified in runtime.

One thing I'm not 100% sure is the change in how session_status() works now, before this change, it would report 0 as in DISABLED, but now it'll report 1 as in NONE even if sessions are not activated in config, as in this case we register a dummy session handler, but it is still a valid handler. Any code that depends on session_status() being 1 might malfunction, although the correct value to check for is 2 as in ACTIVE.

The alternative to this approach would be to conditionally update php.ini to include/exclude the offending line, but I'm not entirely sure how to do that.

roborourke commented 3 years ago

One thing I'm not 100% sure is the change in how session_status() works now, before this change, it would report 0 as in DISABLED, but now it'll report 1 as in NONE even if sessions are not activated in config, as in this case we register a dummy session handler, but it is still a valid handler. Any code that depends on session_status() being 1 might malfunction, although the correct value to check for is 2 as in ACTIVE.

I'm not sure there's much we can do about that other than document the expected return values and how to use it. We can't really warn on 3rd party code checking for any truthy return value unless there's a code sniff for it.

Right now unless you're aware of specific examples of code not checking the status properly I don't think we need to worry about it beyond documentation.

roborourke commented 3 years ago

I've tagged version 4.2.0 for the PHP container with your php.ini update. We'll need to update local server to use this version too and work with @humanmade-cloud to plan this into infrastructure updates.

roborourke commented 3 years ago

The code elements are in place so I'll close this out. Good work @shadyvb