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

[Enhancement]: New Action to fire before WC_Session_Handler converts a guest session to login. #44852

Closed
georgestephanis opened this issue Feb 21, 2024 · 2 comments · Fixed by #45146
Labels
focus: my account Issues related to my account page. priority: normal The issue/PR is of normal priority—not many people are affected or there’s a workaround, etc. team: Proton type: enhancement The issue is a request for an enhancement. type: good first issue The issue is a good candidate for the first community contribution/for a newcomer to the team.

Comments

@georgestephanis
Copy link
Contributor

Describe the solution you'd like

I'd like to request an action be slipped in to fire either in in WC_Session_Handler::save_data()

/**
* Save data and delete guest session.
*
* @param int $old_session_key session ID before user logs in.
*/
public function save_data( $old_session_key = 0 ) {
// Dirty if something changed - prevents saving nothing new.
if ( $this->_dirty && $this->has_session() ) {
global $wpdb;
$wpdb->query(
$wpdb->prepare(
"INSERT INTO {$wpdb->prefix}woocommerce_sessions (`session_key`, `session_value`, `session_expiry`) VALUES (%s, %s, %d)
ON DUPLICATE KEY UPDATE `session_value` = VALUES(`session_value`), `session_expiry` = VALUES(`session_expiry`)",
$this->_customer_id,
maybe_serialize( $this->_data ),
$this->_session_expiration
)
);
wp_cache_set( $this->get_cache_prefix() . $this->_customer_id, $this->_data, WC_SESSION_CACHE_GROUP, $this->_session_expiration - time() );
$this->_dirty = false;
if ( get_current_user_id() != $old_session_key && ! is_object( get_user_by( 'id', $old_session_key ) ) ) {
$this->delete_session( $old_session_key );
}
}
}
or WC_Session_Handler::init_session_cookie()
/**
* Setup cookie and customer ID.
*
* @since 3.6.0
*/
public function init_session_cookie() {
$cookie = $this->get_session_cookie();
if ( $cookie ) {
// Customer ID will be an MD5 hash id this is a guest session.
$this->_customer_id = $cookie[0];
$this->_session_expiration = $cookie[1];
$this->_session_expiring = $cookie[2];
$this->_has_cookie = true;
$this->_data = $this->get_session_data();
if ( ! $this->is_session_cookie_valid() ) {
$this->destroy_session();
$this->set_session_expiration();
}
// If the user logs in, update session.
if ( is_user_logged_in() && strval( get_current_user_id() ) !== $this->_customer_id ) {
$guest_session_id = $this->_customer_id;
$this->_customer_id = strval( get_current_user_id() );
$this->_dirty = true;
$this->save_data( $guest_session_id );
$this->set_customer_session_cookie( true );
}
// Update session if its close to expiring.
if ( time() > $this->_session_expiring ) {
$this->set_session_expiration();
$this->update_session_timestamp( $this->_customer_id, $this->_session_expiration );
}
} else {
$this->set_session_expiration();
$this->_customer_id = $this->generate_customer_id();
$this->_data = $this->get_session_data();
}
}
that will let plugins listen for when a guest_session_id is getting blown away and its data transferred to a user_id.

Something like do_action( 'wc_migrate_guest_session_key', $old_session_key, $user_id ); or the like.

This would let third-party code either connect the prior session key to a user id for data storage, or migrate the old session id to a different cookie that would enable discrete sessions even if multiple browsers authenticated to the same user_id.

Describe alternatives you've considered

If this isn't considered, I've evaluated doing a much messier workaround, that being duplicating the session key in a side channel, and watching for it to change or for there be a logged in user concurrent with it, and try to ensure I'm catching the event before WooCommerce clears it, but it feels very ... klunky, liable to race conditions, and adding overhead to every page load.

Additional context

For plugins that are providing additional context on user flows, the ability to be connect the pre-login result of WC()->session->get_customer_unique_id() to the post-login result where it returns a user_id instead can ensure a more seamless experience for customers without overcomplicated workarounds.

@georgestephanis georgestephanis added type: enhancement The issue is a request for an enhancement. status: awaiting triage This is a newly created issue waiting for triage. labels Feb 21, 2024
@jonathansadowski jonathansadowski added focus: my account Issues related to my account page. team: Proton and removed status: awaiting triage This is a newly created issue waiting for triage. labels Feb 21, 2024
@coreymckrill
Copy link
Collaborator

👍 We try to be careful about adding new action and filter hooks, but this seems like a worthy addition to me. If you spin up a PR for this, @georgestephanis, we'll take a look.

georgestephanis added a commit that referenced this issue Feb 26, 2024
See #44852.

I'm unsure if the `@since` should be populated now, or if that happens when something merges -- I left it as `x.x.x` for now.

Unsure if this should move above the `::save_data()` invocation so it can fire before the migration happens -- but to have less potential for impacts, I'm adding it after the work is done so folks could have a record of what the id changed from and to.
@georgestephanis
Copy link
Contributor Author

PR put together!

@Konamiman Konamiman added priority: normal The issue/PR is of normal priority—not many people are affected or there’s a workaround, etc. type: good first issue The issue is a good candidate for the first community contribution/for a newcomer to the team. labels Mar 4, 2024
coreymckrill pushed a commit that referenced this issue Mar 16, 2024
Fires an action after a customer has logged in, and their guest session id has been deleted with its data migrated to a customer id. This hook gives extensions the chance to connect the old session id to the customer id, if the key is being used externally.

Fixes #44852
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
focus: my account Issues related to my account page. priority: normal The issue/PR is of normal priority—not many people are affected or there’s a workaround, etc. team: Proton type: enhancement The issue is a request for an enhancement. type: good first issue The issue is a good candidate for the first community contribution/for a newcomer to the team.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants