Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#45 | 574084_bakery_page_caching_45.patch | 1.84 KB | greggles |
#39 | 574084_bakery_page_caching_39.patch | 1.58 KB | greggles |
#36 | 574084-bakery-page-caching-goto-36.patch | 952 bytes | coltrane |
#18 | 574084_bakery_page_caching.patch | 1.54 KB | greggles |
#9 | bakery.patch | 3.33 KB | killes@www.drop.org |
Comments
Comment #1
gregglesMoving 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.
Comment #2
joshk CreditAttribution: joshk commentedJust 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.
Comment #3
moshe weitzman CreditAttribution: moshe weitzman commentedI did some cowboy debugging on this but couldn't bring it home. I got past some errors by adding these lines to bakery_boot():
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.
Comment #4
portulacasubscribing
Comment #5
portulacaWhen will the remaining accounts on g.d.o be synced? I miss my groups :(
Comment #6
Steven Jones CreditAttribution: Steven Jones commentedSo 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:
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?
Comment #7
gregglesD.o doesn't :(
Comment #8
Steven Jones CreditAttribution: Steven Jones commentedWhat 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:
And the in the settings.php we set everything up. At the end of it we add:
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.
Comment #9
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedHere's a patch that redirects the user to a dedicated page which can be cached without being seen by another user.
Comment #10
Gábor HojtsyWill the next user possibly see the cached data then instead of his own error message with his own email address / name?
Comment #11
Steven Jones CreditAttribution: Steven Jones commented@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?
Comment #12
killes@www.drop.org CreditAttribution: killes@www.drop.org commented@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.
Comment #13
Steven Jones CreditAttribution: Steven Jones commentedBut 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?
Comment #14
killes@www.drop.org CreditAttribution: killes@www.drop.org commentedWhat 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.
Comment #15
moshe weitzman CreditAttribution: moshe weitzman commentedIIRC, 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.
Comment #16
gregglesThis code solves a different problem.
Comment #17
seutje CreditAttribution: seutje commentedsubscribe
Comment #18
gregglesOk, 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.
Comment #19
moshe weitzman CreditAttribution: moshe weitzman commentedIf 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?
Comment #20
gregglesThis 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.
Comment #21
atul.bhosale CreditAttribution: atul.bhosale commentedgo through this 1
http://www.drupalcoder.com/story/365-disable-drupals-page-cache-for-some...
Comment #22
greggles@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?
Comment #23
gregglesI 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.
Comment #24
Steven Jones CreditAttribution: Steven Jones commentedThis 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.
Comment #25
chx CreditAttribution: chx commentedI 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} ?
Comment #26
tormisubscribing
Comment #27
tormiIn the meantime, it would be nice to inform people that the bakery module is incompatible with caching at the performance settings page.
Tormi
Comment #28
Scott Reynolds CreditAttribution: Scott Reynolds commentedI 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.
Comment #29
marvil07 CreditAttribution: marvil07 commentedsubscribing
Comment #30
greggles@chx #25 I think that's what I did, right?
@marvil07 - can you review my proposed patch?
Comment #31
mikeker CreditAttribution: mikeker commentedIs there an ETA on this fix. As things stand, there is no way to add comments/corrections to api.drupal.org.
Comment #32
drummThis 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.
Comment #33
bht333 CreditAttribution: bht333 commenteddrupal.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.
Comment #34
gregglesTo 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.
Comment #35
gregglesActually, 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.
Comment #36
coltraneI'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 calledand 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.
Comment #37
gregglesI just deployed the patch from #36 to groups.drupal.org.
Let's see if that helps folks.
Comment #38
gregglesNope, 36 alone doesn't help much for groups.drupal.org.
Comment #39
gregglesI'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.
Comment #40
marvil07 CreditAttribution: marvil07 commentedI just tried to reproduce it(hopefully I get it right), and it still happens on g.d.o: http://blip.tv/file/4269130
Comment #41
Steven Jones CreditAttribution: Steven Jones commentedSorry to be picky, but...
Some debugging code left in the patch?
Powered by Dreditor.
Comment #42
greggles@Steven Jones: And...the rest of it you reviewed and are happy with?
Comment #43
gregglesFor 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).
Comment #44
marvil07 CreditAttribution: marvil07 commentedThe bug shown on the video stop happening :-)
thanks!
Comment #45
gregglesNow committed to 6.x-2.x-dev: http://drupal.org/cvs?commit=442764
Comment #46
gregglesAs 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.
Comment #47
gregglesI 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
Comment #48
coltrane#1107692: Port bakery 6.x-2.x to 7.x-2.x has hook_boot