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

Pages cache not working (neither stored not used) when checking for logged in user #4634

Closed
sylvainjule opened this issue Sep 8, 2022 · 8 comments · Fixed by #4646
Closed
Assignees
Labels
type: bug 🐛 Is a bug; fixes a bug
Milestone

Comments

@sylvainjule
Copy link
Contributor

sylvainjule commented Sep 8, 2022

Description

Having read @lukasbestle's explanation in #4423, I figured the following would work. This thread in the forum suggests this should work too so I'm mentioning this as an issue. Starting with a fresh starterkit, I activate the cache in config.php:

'cache' => [
    'pages' => [
        'active' => true
    ]
]

Pages are stored. Let's say I want to add the ability for my logged in users to have an edit button available on any front-end page which redirects to this very same page in the panel. I clear the cache and I add in my header.php:

<?php if(kirby()->user()): ?>
    <a class="edit" href="<?php echo $page->panel()->url() ?>">Edit content</a>
<?php endif; ?>

As soon as I do, the cache stops storing pages, the Cache-Control: no-store, private header is always set despite me not being logged in. Here's a video of this:

Enregistrement.de.l.ecran.2022-09-08.a.12.20.31.mp4

Your setup

Kirby 3.7.5

@distantnative distantnative added the type: bug 🐛 Is a bug; fixes a bug label Sep 8, 2022
@lukasbestle
Copy link
Member

lukasbestle commented Sep 8, 2022

@sylvainjule @LaurentTacco Could you please try if the following change in line 276 of kirby/src/Http/Request.php fixes it for you:

-		return $header !== null;
+		return empty($header) === false;

Explanation: The old line considers an empty but existing HTTP_AUTHORIZATION value as active, which is of course not true.

@lukasbestle lukasbestle added this to the 3.8.0 milestone Sep 8, 2022
@lukasbestle lukasbestle added the needs: information ❓ Requires more information to proceed label Sep 8, 2022
@LaurentTacco
Copy link

LaurentTacco commented Sep 9, 2022

@lukasbestle By changing this line, cache seems to finally work as expected on my side! Thanks!!

@sylvainjule
Copy link
Contributor Author

Same here! Thanks @lukasbestle!

@lukasbestle lukasbestle removed the needs: information ❓ Requires more information to proceed label Sep 9, 2022
@lukasbestle
Copy link
Member

@LaurentTacco Could you please also check if changing the SetEnvIf line in .htaccess from (.*) to (.+) fixes it (without the change in the core)?

@LaurentTacco
Copy link

@lukasbestle Without changing anything in the core and by having SetEnvIf Authorization "(.+)" HTTP_AUTHORIZATION=$1 in the .htaccess everything works perfectly.

@lukasbestle
Copy link
Member

Great, thanks for the quick reply.

Then we'll fix it in both places. The .htaccess change ensures that no empty headers are inserted if no header was passed, while the core change ensures that any empty header (no matter where it comes from) won't break the behavior.

@LaurentTacco
Copy link

You're welcome!

lukasbestle added a commit to getkirby/starterkit that referenced this issue Sep 15, 2022
lukasbestle added a commit to getkirby/getkirby.com that referenced this issue Sep 15, 2022
lukasbestle added a commit to getkirby/demokit that referenced this issue Sep 15, 2022
@lukasbestle lukasbestle linked a pull request Sep 15, 2022 that will close this issue
5 tasks
lukasbestle added a commit to getkirby/getkirby.com that referenced this issue Sep 16, 2022
lukasbestle added a commit to getkirby/demokit that referenced this issue Sep 16, 2022
@bastianallgeier
Copy link
Member

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: bug 🐛 Is a bug; fixes a bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants