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

Session cookies are created on every page load #4284

Closed
mindctrl opened this Issue Feb 10, 2016 · 43 comments

Comments

Projects
None yet
8 participants
@mindctrl
Member

mindctrl commented Feb 10, 2016

We've had two reports recently about the fact that a session cookie gets created on every page load, making it hard to cache. One came through a Pagely customer and another was from Pressjitsu.

One report:

The use of sessions in EDD prevents any page from being cached because it's issuing a unique cookie to every visitor. It would be best if EDD initialized this unique session ONLY when the user has taken any sort of action, like add something to the cart. That way, the majority of the users can enjoy fast cached responses throughout the whole website, while those with intent to buy something will get their unique cookies.

Other report:

The site is setting edd_wp_session cookies for every visitor, which makes thing a lot more painful, as nothing could be cached at the Nginx level & every request is dynamic.

If you use EDD_USE_PHP_SESSIONS set to false and fall back to WP_Session, you always get a cookie because it creates one in __construct(). Link.

@mindctrl mindctrl added the Bug label Feb 10, 2016

@cklosowski

This comment has been minimized.

Member

cklosowski commented Feb 10, 2016

We've got to make sure if we make any changes to this, that it doesn't break extensions that require the sessions, things like User History.

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Feb 10, 2016

Let's try and resolve this in a point release that has almost nothing else in it. For now, milestoning for 2.5.9

@cklosowski

This comment has been minimized.

Member

cklosowski commented Mar 17, 2016

looking into this I can almost get there. I can get sessions/cookies to only be created when adding to the cart, however for some reason the cookies don't carry over to page refreshes. All AJAX requests know about the current cart contents, but the front of site has no idea about it, which is strange.

@chriscct7

This comment has been minimized.

Member

chriscct7 commented Apr 9, 2016

This is going to break any extensions that rely on EDD sessions from the sound of it outside of the normal checkout flow

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Apr 22, 2016

@chriscct7 Yes it will and we'll have to deal with that. Unfortunately, breaking caching on every single page of every single EDD site is far worse.

@cklosowski

This comment has been minimized.

Member

cklosowski commented May 25, 2016

Every time I try and get this done, something major breaks in the cart system. I've taken a few stabs at it between getting other issues resolved. I'm going to punt (for now) and we can look to get this in a future release.

It's not that it's not important, it's just important to get right.

@cklosowski cklosowski modified the milestones: 2.6.3, 2.6 May 25, 2016

@bmoredrew

This comment has been minimized.

bmoredrew commented May 26, 2016

Thanks for the effort everyone. Is there something we can do on an individual site level? My client has a pretty large site in terms of traffic that just falls over on Pagely when we try to implement EDD, so all the pieces are in place just kind of sitting there for a one day hopeful fix/use.

@cklosowski

This comment has been minimized.

Member

cklosowski commented May 26, 2016

Drew,

At this point, we've already released some restrictions that stop it from
loading on the RSS feeds. On the EDD Site specifically we've stopped it
from starting sessions on the homepage with something like the following:

function eddwp_maybe_start_session( $start_session ) {
if ( '/' == $_SERVER['REQUEST_URI'] ) {
$start_session = false;
}

return $start_session;

}
add_filter( 'edd_start_session', 'eddwp_maybe_start_session', 10, 1 );

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented May 26, 2016

@bmoredrew One major improvement you could make is to prevent sessions from starting on specific pages, such as pages that will never have an add to cart button on them: #4429

@mindctrl

This comment has been minimized.

Member

mindctrl commented Jun 9, 2016

This issue was mentioned to one of our customers by Pagely again. The sessions are preventing her site from being cached.

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Jun 9, 2016

@mindctrl go ahead and show her how to exclude certain pages (such as home if possible) from starting caches.

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Jun 13, 2016

WooCommerce 2.5 included a complete rewrite of their session manager. They originally used a very similar system to ours. Let's look at what they did to see if that will help.

https://woocommerce.wordpress.com/2015/10/07/new-session-handler-in-2-5/

@sunnyratilal

This comment has been minimized.

Member

sunnyratilal commented Jun 30, 2016

Woo use a custom table to store this data and have a custom implementation built which handles sessions via this new table: https://github.com/kloon/woocommerce-large-sessions

@cklosowski cklosowski added this to the 2.6.6 milestone Jun 30, 2016

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Jan 20, 2017

This is triggering a fatal error when EDD_USE_PHP_SESSIONS is defined as false.

[20-Jan-2017 14:53:11 UTC] PHP Fatal error: Uncaught Error: Call to a member function get_queried_object() on null in /app/public/wp-includes/query.php:45
Stack trace:
#0 /app/public/wp-content/plugins/easy-digital-downloads/includes/class-edd-session.php(349): get_queried_object()
#1 /app/public/wp-content/plugins/easy-digital-downloads/includes/class-edd-session.php(78): EDD_Session->should_start_session()
#2 /app/public/wp-content/plugins/easy-digital-downloads/easy-digital-downloads.php(150): EDD_Session->__construct()

@sunnyratilal

This comment has been minimized.

Member

sunnyratilal commented Jan 20, 2017

Looks like get_queried_object() is called way too early - before WP_Query is set up.

Currently, when EDD_USE_PHP_SESSIONS is false, we call EDD_Session::init on plugins_loaded but when true it's loaded on init.

cklosowski added a commit that referenced this issue Jan 23, 2017

cklosowski added a commit that referenced this issue Jan 23, 2017

@cklosowski

This comment has been minimized.

Member

cklosowski commented Jan 23, 2017

This seems to resolve it being called to early, see if that works @sunnyratilal @pippinsplugins

@spencerfinnell

This comment has been minimized.

Member

spencerfinnell commented Jan 23, 2017

@cklosowski That won't work with the default permalink structure.

@cklosowski

This comment has been minimized.

Member

cklosowski commented Jan 24, 2017

@spencerfinnell well for few major versions, default has been altered from the query string params. I looked at url_to_postid but it's too early for that as well.

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Jan 24, 2017

@cklosowski It's been a while since I played with but could we load EDD_Session on init even when EDD_USE_PHP_SESSIONS is false?

@cklosowski

This comment has been minimized.

Member

cklosowski commented Jan 24, 2017

@pippinsplugins I'll give that a shot to see if that works.

@spencerfinnell

This comment has been minimized.

Member

spencerfinnell commented Jan 24, 2017

That was going to be my next question as well. Seems like that would allow for the fewest special cases.

@cklosowski

This comment has been minimized.

Member

cklosowski commented Jan 24, 2017

@pippinsplugins @spencerfinnell honestly, this seems to be working fine, I'll commit it up so we can have some more people test it out.

cklosowski added a commit that referenced this issue Jan 24, 2017

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Jan 24, 2017

This appears to work perfectly for me!

Something did just occur to me: we will likely break any plugin that sets EDD session data before items are added to the cart.

@cklosowski @sunnyratilal could you both do some testing to confirm that session vars can still be set before items are added to the cart? We may need to call EDD()->session->set_cart_cookie( true ); any time something is added to the session instead of just when adding items to the cart.

@cklosowski

This comment has been minimized.

Member

cklosowski commented Jan 24, 2017

Even doing that isn't seeming to work. @sunnyratilal can you give it a shot? I just used this to determine if it was working:

function test_stuff() {
	if ( isset( $_GET['test-stuff'] ) ) {
		EDD()->session->set( 'testing-cookie', 'testing-value' );
	}
        var_dump(EDD()->session->get( 'testing-cookie' );
}
add_action( 'init', 'test_stuff' );

Basically visit with ?test-stuff=true in the URL, and then see the cookie saved there in the session....then remove the query string, and visit the same page. My guess is, it's gone for you too?

cklosowski added a commit that referenced this issue Jan 24, 2017

@sunnyratilal

This comment has been minimized.

Member

sunnyratilal commented Jan 25, 2017

@cklosowski the cookie is gone when removing the query string

@cklosowski

This comment has been minimized.

Member

cklosowski commented Jan 25, 2017

Ok that sounds like what I was seeing too @sunnyratilal.

@pippinsplugins going to try and have to think through this, honestly, I'm ok if this doesn't make Beta 1....but it HAS to make Beta 2.

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Jan 25, 2017

honestly, I'm ok if this doesn't make Beta 1....but it HAS to make Beta 2.

Nope, this will be in beta 1 or not in at all.

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Jan 25, 2017

It's too important to the fundamental foundation of every EDD store to not have in beta 1.

@cklosowski

This comment has been minimized.

Member

cklosowski commented Jan 25, 2017

@cklosowski

This comment has been minimized.

Member

cklosowski commented Jan 26, 2017

Ok, I determined why my test wasn't working, and it actually affects many things...anything that tries to run EDD()->session->set() will typically run on init or later due to our edd_action running at that point, but this means that headers_sent() will return true, including things like Discounts Pro, Cart Resuming, and SL actions.

We cannot run $_COOKIE['cookie-name'] on things after the headers were sent. Which means, that we cannot use the method to always set the items-in-cart cookie whenever set() is run.

We'll have to find another way around this.

@pippinsplugins

This comment has been minimized.

Member

pippinsplugins commented Jan 26, 2017

:(

@sunnyratilal

This comment has been minimized.

Member

sunnyratilal commented Jan 27, 2017

Closing this in favour of #5418 as this requires a fundamental rebuild to how sessions work.

@cklosowski cklosowski removed this from the 2.7 milestone Jan 27, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment