The Bakery, in its current form, is not at all compatible with the page cache, because the main processing happens in bakery_init(). I see two possible solutions:

  • move the processing to bakery_boot(),
  • make the bakery a session.inc replacement.
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

greggles’s picture

Moving it to hook_boot makes a lot more sense ot me than to session.inc. session.inc feels like a really heavy solution. Do you have any sense why a session.inc solution would be better?

I've done this once recently and remember just a few small things about it that are not possible to do in hook_boot but worked in hook_init.

joshk’s picture

Just as a note, I attemped to make a hot-fix to switch to hook_boot() and it didn't "just work." I'm sure it can work, but it's not as simple as renaming the function. It's on my todo list to get a real testing environment setup for Bakery, but haven't been able to do that.

Anyway, in terms of the development here I'd like to see both fixes implemented. Architecting around hook_boot() will make the module re-usable for more folks outside the d.o family, but the truth is that Bakery really does want to be a session manager in order to do SSO correctly.

moshe weitzman’s picture

Priority: Normal » Critical

I did some cowboy debugging on this but couldn't bring it home. I got past some errors by adding these lines to bakery_boot():

  require_once './includes/form.inc';
  require_once './includes/common.inc';
  require_once './includes/theme.inc';
  drupal_load('module', 'user');

But still I end up with the not so descriptive:

PHP Fatal error: Unsupported operand types in /var/www/groups.drupal.org/htdocs/includes/form.inc on line 516

I think this needs a proper dev environment.

portulaca’s picture

subscribing

portulaca’s picture

When will the remaining accounts on g.d.o be synced? I miss my groups :(

Steven Jones’s picture

So we have this issue, which is pretty much a showstopper.

We've 'solved' it by creating own own session handler, which is just a copy of session.inc and then adding the following to the top of that file:

drupal_load('module', 'bakery');
if (_bakery_validate_cookie()) {
  $conf['cache'] = FALSE;
}

And thus disabling the page cache the if bakery cookie passes muster.

I'm sure there must be a better solution. How does d.o solve it?

greggles’s picture

How does d.o solve it?

D.o doesn't :(

Steven Jones’s picture

What we've gone for on our sites is not a replacement for the session.inc, but a redirection.

So we pop a file someplace in Drupal with the contents:


/**
 * @file
 * Bakery 'fake' session handler.
 *
 * We check to see if we should disable the page cache on this request and then
 * load the old session handler.
 */

// This disables page caching if the bakery cookie is set:
drupal_load('module', 'bakery');
if (_bakery_validate_cookie()) {
  $conf['cache'] = FALSE;
}

// Now revert to the old session handling:
require_once variable_get('session_inc_old', './includes/session.inc');

And the in the settings.php we set everything up. At the end of it we add:


/**
 * Bakery module session setup:
 */
// Save the old session handler:
if (isset($conf['session_inc'])) {
  $conf['session_inc_old'] = $conf['session_inc'];
}

// Now set up our session handler
$conf['session_inc'] = './sites/all/bakery/bakery.session.inc';

I must admit we're using a totally hacked version of bakery, so this may not work for the official one, but it might be an option? I think the only case left is if some cache system serves up the page using the 'fast cache' though I have to admit I have no idea how that works.

killes@www.drop.org’s picture

Status: Active » Needs review
FileSize
3.33 KB

Here's a patch that redirects the user to a dedicated page which can be cached without being seen by another user.

Gábor Hojtsy’s picture

Will the next user possibly see the cached data then instead of his own error message with his own email address / name?

Steven Jones’s picture

@killes was this patch for #611744: Anonymous users see each other's error messages.? Or is it part of the effort to get bakery compatible with page caching?

killes@www.drop.org’s picture

@Gabor: no, the page cache will only cache one page per user since the URLs are different.

@Steven: I consider that issue to be a duplicate of this one.

Steven Jones’s picture

But your patch seems to fix that issue, not this one. This issue is about bakery not being invoked for pages that are cached, whereas #611744 is about caching of pages once bakery has been invoked is it not?

Reviewing the patch itself, do you need to md5 the unique token, what does that add?

killes@www.drop.org’s picture

What should bakery do with cached pages anyway? Maybe I am a bit confused about the boot order here? I don't really recall what else than the cached messages the problem was that we want to fix...

no, I probably do not need the md5.

moshe weitzman’s picture

IIRC, the problem is that a user first arrives at a slave site and gets a cached page. He has a cookie from the master site but the code to read that cookie and log him in never runs. So the user is anon on the slave site until he clicks a login link or posts a form or something that triggers an uncached page view.

greggles’s picture

Status: Needs review » Needs work

This code solves a different problem.

seutje’s picture

subscribe

greggles’s picture

Status: Needs work » Needs review
FileSize
1.54 KB

Ok, here's an alternate approach.

We have to bootstrap because otherwise user_load is not available.

The redirect is required because the bootstrap happens when Drupal thinks the user is anonymous so the page is mostly rendered as if the user is anonymous. If you remove the drupal_goto you'll see that you have to hit the page twice to get it to realize that the user is actually logged in.

I will wawit for reviews from someone else before deploying this.

I know that in other instances similar to d.o (using memcache) that manually forcing a bootstrap was a bit of a problem, so the major place to test this out will be a drupal.org staging environment that has memcache set up.

moshe weitzman’s picture

If we are going to force a full bootstrap, we might as well just add a hook_requirements error if page cache is enabled and be done with it. then this code this in hook_init(). am i missing something?

greggles’s picture

This only does a bootstrap if the cookie is valid and therefore the user is about to be logged in. So, anonymous users with caching on still get the super speedy lightweight bootstrap.

atul.bhosale’s picture

greggles’s picture

@a.pappu I don't see how that would help? We don't want to disable the cache on a specific page, we want to move some code from hook_init to hook_boot so it always runs. Can you explain more?

greggles’s picture

I guess one alternative approach based on the suggestion of a.pappu is that we offer a login link that goes a page that is not cached. On that page, the bakery cookie will be detected and the user will be logged in...OR if the cookie doesn't exist it could redirect to the master site. That seems like it would work without all the bootstrap hocus pocus.

Steven Jones’s picture

This is almost what we have now on g.d.o, which is rubbish. If a user has logged in on the master site already they should just get logged in on the child sites, no extra clicking involved.

chx’s picture

I am not sure I get the problem. We surely can guess that the user needs a login and if that's the case (ie cookie exists) then we boot up and verify and log in. If you do not have a bakery cookie nothing happens.

Edit: I am not even 100% we need a full blown user load, cant we just SELECT * FROM {users} ?

tormi’s picture

subscribing

tormi’s picture

In the meantime, it would be nice to inform people that the bakery module is incompatible with caching at the performance settings page.

Tormi

Scott Reynolds’s picture

I am researching a single signon for my set of sites and this module is part of my research.

Im a little surprised at the approach for this problem. Seems to me that the hook_init() and hook_boot() are both the wrong places for this. This seems to fit squarely in a session handler function sess_read() and figure it all out.

_bakery_validate_cookie, which is at the heart of all this, does require variable_get() which could pose a problem. But seems to me that could be changed anyways to not use variable_get().

I am interested in why that approach wasn't taken.

marvil07’s picture

subscribing

greggles’s picture

@chx #25 I think that's what I did, right?

@marvil07 - can you review my proposed patch?

mikeker’s picture

Is there an ETA on this fix. As things stand, there is no way to add comments/corrections to api.drupal.org.

drumm’s picture

This shouldn't fully prevent logins to any site. After logging in to drupal.org, reloading a page at api.drupal.org should be logged-in. If it is not, we have another problem.

bht333’s picture

drupal.org is broken since September last year because of this and this is unassigned?

As someone who does not have any Drupal knowledge, I am surprised that Drupal cannot handle single sign-on with itself - within the same application even while fully in control of all master and slave sites.

It should be really easy for any Drupal developer, because it is already known that a single browser reload fixes this. If you are really running out of ideas, then please consider the suggestion in #766354: Bakery single sign-on works only if target page is reloaded manually which will definitely solve this.

greggles’s picture

Status: Needs review » Needs work

To be less cryptic - the alternate idea from bht333 is to add a ?$randomstring onto the end of the URL to make sure it will not hit the varnish nor page cache.

I like that a ton more than my solution.

greggles’s picture

Version: 7.x-1.x-dev » 6.x-1.x-dev
Status: Needs work » Needs review

Actually, the redirect with a query parameter only solves the scenario of logging in from the subsite. It doesn't help with the scenario of going from the master to the slave (e.g. by follow a link from the master).

So, my patch is still needs review.

coltrane’s picture

I've corrected first request getting 403 with the attached patch, a subset of greggle's #18 patch. This probably won't be enough if you have caching enabled on the slave but in my case the slave is OpenAtrium which serves almost no anonymous pages anyways. #18 patch wasn't working on it's own cause oddly hook_boot wasn't being called and the bootstrap wasn't necessary. Edit: I hadn't cleared cache after moving to hook_boot.

Some more testing of #18 is needed but I consider at least the forced reload to work for both user's first hitting the slave or first logging into the master.

greggles’s picture

I just deployed the patch from #36 to groups.drupal.org.

Let's see if that helps folks.

greggles’s picture

Nope, 36 alone doesn't help much for groups.drupal.org.

greggles’s picture

I'm trying to repeat this behavior on drupal.org and groups.drupal.org and am unable to do it. I tried once and my login on the slave worked fine, I had a bunch of stale cookies though so I figured one of those might be involved. I cleared all *drupal.org cookies and tried again. It worked fine again. Maybe the redesign has helped us in some way?

On the other hand, in my local testing the patch from #16 worked great, though with some offsets. Attaching a new version.

Also, I moved the drupal_bootstrap inside of the check for uid. This way it really only calls bootstrap if we're logging someone into the site based on a bakery request.

In my opinion, with this improvement it is RTBC.

marvil07’s picture

I just tried to reproduce it(hopefully I get it right), and it still happens on g.d.o: http://blip.tv/file/4269130

Steven Jones’s picture

Status: Needs review » Needs work

Sorry to be picky, but...

+++ bakery.module	19 Oct 2010 22:58:23 -0000
@@ -677,6 +677,8 @@ function _bakery_taste_chocolatechip_coo
+      drupal_set_message('bootstrapper');

Some debugging code left in the patch?

Powered by Dreditor.

greggles’s picture

@Steven Jones: And...the rest of it you reviewed and are happy with?

greggles’s picture

For the people who have this problem on groups.drupal.org, can you try again? I just added this patch over there for testing and made sure to clear the cache so that Drupal knows this is needed for bootstrapping (and I removed the bootstrapper dsm line).

marvil07’s picture

The bug shown on the video stop happening :-)

thanks!

greggles’s picture

Version: 6.x-1.x-dev » 6.x-2.x-dev
Priority: Critical » Normal
Status: Needs work » Patch (to be ported)
FileSize
1.84 KB

Now committed to 6.x-2.x-dev: http://drupal.org/cvs?commit=442764

greggles’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev

As I said in the comment I fixed this in 6.x-2.x, but it still needs to be fixed in 7.x. Updating version to reflect that.

greggles’s picture

I added some further comments to explain just how small the performance impact is. Otherwise this looks like we're negating some of the main benefits of page caching http://drupalcode.org/project/bakery.git/commit/9a2d676

coltrane’s picture

Status: Patch (to be ported) » Closed (duplicate)