Skip to content

Add support for content negotiation #28

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.

Sign up for GitHub

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

Jump to bottom
Closed
ocean90 opened this issue Mar 13, 2023 · 5 comments
Closed

Add support for content negotiation #28

ocean90 opened this issue Mar 13, 2023 · 5 comments

Comments

@ocean90
Copy link

ocean90 commented Mar 13, 2023

Found the issue pluginkollektiv/cachify#265 and noticed that it also applies to Surge.

This can be reproduced with the ActivityPub plugin active which serves different content based on the accept header. Example:

$ curl 'https://dominikschilling.de/notes/author/dominik/?123' -H 'Accept: application/activity+json'
# Output: {"@context":["https:\/\/www.w3.org\/ns\/activitystreams","ht ...
$ curl 'https://dominikschilling.de/notes/author/dominik/?123' -H 'Accept: text/html'
# Unexpected output: {"@context":["https:\/\/www.w3.org\/ns\/activitystreams","ht ...
$ curl 'https://dominikschilling.de/notes/author/dominik/?456' -H 'Accept: text/html'
# Output: <html lang="en-US">
$ curl 'https://dominikschilling.de/notes/author/dominik/?456' -H 'Accept: application/activity+json'
# Unexpected output: <html lang="en-US">

@kovshenin Should the accept header be added to the cache key or should the cache be skipped for non-HTML requests somewhere here? Or something else?
@kovshenin
Copy link
Owner

Hi @ocean90, thanks for your comment!

Unfortunately the Accept header isn't a great one to vary on as there is too much difference between different browsers, and even browser versions. For example for this issue page on GitHub in Chrome I get:

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,*/*;q=0.8

And in Safari I get:

Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7

This kind of variance will kill cache efficiency, so blindly varying on this header is not a great idea.

What you can do for ActivityPub is normalize and filter the header, and set a specific variant flag with a custom cache config. For example:

// Do some string manipulation with $_SERVER['HTTP_ACCEPT']
$config['variants']['activity-pub'] = $result_from_manipulation;
return $config;

I know some users do this to detect requests from smartphones based on the User-Agent header, which is also not a great one to vary on, without converting it to a less variable 1/0 or mobile/desktop value.

If you're looking for more information about custom cache configuration, my friend @roytanck published a post a while ago: https://roytanck.com/2022/01/13/tweaking-surge-by-adding-a-configuration-file/

Hope that helps!

@ocean90
Copy link
Author

ocean90 commented Mar 13, 2023

Thanks for the quick feedback. I was aware of the custom configuration file but wasn't sure if variants is what I was looking for. I'll give this a try!

@ocean90
Copy link
Author

ocean90 commented Mar 14, 2023 •
edited
Loading

Got it working with the following config for variants:

// Content negotiation.
$representation = 'generic'; // Or just 'html'.
if ( isset( $_SERVER['HTTP_ACCEPT'] ) ) {
	$accept = strtolower( $_SERVER['HTTP_ACCEPT'] );

	if ( str_contains( $accept, 'text/html' ) ) {
		$representation = 'html';
	} elseif (
		// Based on https://github.com/pfefferle/wordpress-activitypub/blob/abef17b9ad5d8e346aca92008e3b2b13aaa4ebf4/includes/class-activitypub.php#L67-L84.
		str_contains( $accept, 'application/json' ) ||
		str_contains( $accept, 'application/activity+json' ) ||
		str_contains( $accept, 'application/ld+json' )
	) {
		$representation = 'json';
	}
}
$config['variants']['representation'] = $representation;
unset( $accept, $representation );

return $config;

Thanks again!

@ocean90 ocean90 closed this as completed Mar 14, 2023
@kovshenin
Copy link
Owner

Nice, thanks for sharing @ocean90! Also IIRC you have to return $config as it's passed on to a variable which is then merged with the main config:

surge/include/common.php

Lines 53 to 56 in b0f2a2d

// Run a custom configuration file.
if ( defined( 'WP_CACHE_CONFIG' ) ) {
$_config = ( function( $config ) {
$_config = (array) include( WP_CACHE_CONFIG );

@ocean90
Copy link
Author

ocean90 commented Mar 15, 2023

Right, I already had that in my config. Updated above in case anyone wants to copy it as is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@kovshenin @ocean90