keycdn / cache-enabler

A lightweight caching plugin for WordPress that makes your website faster by generating static HTML files.
https://wordpress.org/plugins/cache-enabler/
123 stars 46 forks source link

Brotli support #1

Closed PeterDaveHello closed 3 years ago

PeterDaveHello commented 7 years ago

Thanks for this really helpful plug-in first!

I think it'll be very cool if brotli or zopfli support can be added in this plug-in, not sure what do you think?

centminmod commented 6 years ago

Would love for brotli pre-compression support. I did benchmarks of pre-gzip vs pre-brotli vs on-the-fly gzip vs on-the-fly-brotli Nginx HTTP compression at https://community.centminmod.com/threads/nginx-with-cloudflare-zlib-fork-vs-nxg_brotli-compression-level-tests.13820/#post-63601 as you can see Brotli pre-compressed comes out on top. Though with Nginx static gzip and static brotli compression, if you have both pre-compressed it seems gzip takes priority when served.

picasticks commented 4 years ago

Just thought I'd comment on this old issue that I just sent a PR for a feature that adds removal of .br versions of pages on cache clear.

It doesn't actually at brotli encoding, which I do outside this plugin (by running the brotli encoder on the .html files when there is not a .br version).

PHP doesn't have a standard/native brotli module but there is a PECL package and this project. I don't use either I just mention for awareness.

As for zopfli, personally I would not recommend using it inside the PHP process, it's slow and CPU intensive.

PeterDaveHello commented 4 years ago

@picasticks yes, zopfli is for some special cases ;)

coreykn commented 4 years ago

@nlemoine we are interested in adding Brotli support and thanks for the effort you've put into this. With all of the recent changes over the last month we are nearly ready to add support for Brotli compression. As far as I'm aware this would require a third party module. About the potential PR you've mentioned in #143, just to be clear on my own tests what module have you chosen to handle the Brotli compression? (I see the brotli_compress() function is used in two different modules that I've been able to find so far.) The changes you've made look great. The only feedback I have right now would be when this is introduced we want to make it a single setting.

nlemoine commented 4 years ago

There are two parts in this feature.

Serving compressed files

This feature could be implemented right now for people who managed to compress static files on their own (e.g. outside the plugin scope, like @picasticks did).

Providing it even if the compression "feature" is missing will still be an improvement:

Compressing files

That's the hardest part. As stated by @picasticks, "PHP doesn't have a standard/native brotli module".

I chose to go for the PHP extension. That's why there's a function_exists('brotli_compress') check.

I've made that choice because It seems the most popular way for now to bring brotli compression to PHP. And mostly because this method can be "polyfilled" with: https://github.com/vdechenaux/brotli-php This package provides brotli compression if your server has brotli installed. The best part is that the author provides precompiled binaries you can easily install without having extended server permissions (I used it on a shared hosting).

I had one big issue though. All the generating files logic in Cache Enabler happens in a ob_start callback. vdechenaux/brotli-php is using symfony/process to execute brotli commands. And symfony/process is using ob_start at some point. Which triggers a Cannot use output buffering in output buffering display handlers error I was not able to solve. So I forked it: https://github.com/nlemoine/php-brotli-compress (I just installed the binary by hand for now, this a WIP).

IMO, that's the best/easiest approach to provide brotli compression feature. If you want to target standard users, you'll have to embed the binary with the plugin just like EWWW Image Optimizer did.


The only feedback I have right now would be when this is introduced we want to make it a single setting.

I don't see any reason why this setting exists (see the last part of my comment in #143) but I maybe not have all context.

coreykn commented 4 years ago

@nlemoine what you've done will work great for serving cached pages that have been compressed with Brotli. When introducing Brotli support I currently prefer to have this as a complete solution that offers the ability to compress cached pages with Brotli. That way all users can benefit from it. If you have the desire to work on this further that is always welcome and appreciated. (That would be as you pointed it out including anything necessary in the plugin itself, like the binaries.) If not, I'll have the time to research that half and what solutions are available sometime after version 1.5.0 has been released. The detailed information you've provided will be very helpful for this. Depending on the outcome of this we can either use what you have or build upon it.

The pre-compression setting exists because it controls whether or not Cache Enabler compresses cached pages. I don't believe we should do this by default.

nlemoine commented 4 years ago

Thanks for your feedback. I think I won't take it any further. I won't have much spare time to work on it soon. Furthermore, It will be hard to propose things we didn't agree on yet. I'll probably work on a private version until brotli has landed in CE.

I don't believe we should do this by default.

I strongly believe you should: 1- End users (e.g. non technical) could ignore what that setting means and leave it unchecked to be safe 2- It's all benefits (visitors, bandwidth, etc.) with absolutely no drawbacks. Browsers that don't support gzip don't exist anymore. Seriously, think about it twice. I'm sorry to insist but I don't see any valid reason to not provide gzip by default ("it controls whether or not Cache Enabler compresses cached pages" is not an argument 😉 ).

coreykn commented 3 years ago

@nlemoine, I decided your approach would be best to start and would love to get this feature added. Would you be interested in opening a PR for this support? I'd prefer the credit go to you given the work you've already done on this. I'm unsure if the output errors are still present and if Cache Enabler may need to make any adjustments to support this. If that's the case, please let me know and I can dig into that side of things.

I had this future change in mind when making the changes in 1.7.0. With that I believe all that would be required is to make changes similar to the following:

First, in Cache_Enabler_Disk::get_cache_keys():

// compression cache key
if ( Cache_Enabler_Engine::$settings['compress_cache'] ) {
    if ( function_exists( 'brotli_compress' ) && strpos( Cache_Enabler_Engine::$request_headers['Accept-Encoding'], 'br' ) !== false ) {
        $cache_keys['compression'] = '.br';
    } elseif ( strpos( Cache_Enabler_Engine::$request_headers['Accept-Encoding'], 'gzip' ) !== false ) {
        $cache_keys['compression'] = '.gz';
    }
}

Next, in Cache_Enabler_Disk::create_cache_file():

// compress page contents with Brotli or Gzip if applicable
if ( strpos( $new_cache_file_name, 'br' ) !== false ) {
    $page_contents = brotli_compress( $page_contents );

    // check if Brotli compression failed
    if ( $page_contents === false ) {
        return;
    }
} elseif ( strpos( $new_cache_file_name, 'gz' ) !== false ) {
    $page_contents = gzencode( $page_contents, 9 );

    // check if Gzip compression failed
    if ( $page_contents === false ) {
        return;
    }
}

We could maybe do a fallback to Gzip if Brotli failed, but not sure if it'd be worth it as I'm assuming that case would be quite rare if it occurs at all.

Then, add the encoding header in Cache_Enabler_Engine::deliver_cache().

Lastly, we could update the setting if the brotli_compress function exists, something like the following:

<?php ( function_exists( 'brotli_compress' ) ) ? esc_html_e( 'Create a cached version pre-compressed with Brotli or Gzip.', 'cache-enabler' ) : esc_html_e( 'Create a cached version pre-compressed with Gzip.', 'cache-enabler' ); ?>

I think that's all that would be needed. Let me know if you're interested! 🙂

coreykn commented 3 years ago

Also, now that we generate each cache file individually by default I'm fairly certain we'll be enabling compression by default here shortly. (I will make that change once we do.)

nlemoine commented 3 years ago

Hi @coreykn,

Thanks for your feedback on this! Actually, I already use brotli compression in a forked version of CE. You'll find similar code in the develop branch: https://github.com/nlemoine/cache-enabler/tree/develop/inc

Note that it contains changes to handle all files creation as opposed to current one file at a time (as discussed in #211).

I also developed a tiny package (which is a fork of https://github.com/vdechenaux/brotli-php) to make brotli_compress available: https://github.com/nlemoine/brotli-php It provides binaries for most popular OS in case brotli isn't installed on server. So anyone using composer could get brotli compression quite easily.

Regarding compression setting, brotli availability not only depends on brotli_compress existence. It also depends on SSL being enabled or not because http does not support brotli compression so maybe an additional is_ssl check would ensure the site can properly deliver brotli compressed files.

I'll submit a PR soon so you can check it.

I'm fairly certain we'll be enabling compression by default here shortly

Great news! I've always been in favor of this behavior.

coreykn commented 3 years ago

Ah, that's great to see that you already implemented this feature! Also, that's helpful seeing the fork for brotli_compress that you've made. I just converted what I remember you had before, so it looks like I was on a similar page as you. 🙂

It also depends on SSL being enabled...

You're absolutely right. Instead of is_ssl(), I'd recommend checking the $cache_keys['scheme'] cache key as the HTTPS check is done when getting that cache key in Cache_Enabler_Disk::get_cache_keys(). That check is the is_ssl() function but also checks additional request headers. If I recall correctly, is_ssl() isn't used as it's not available yet during the early cache engine start in the advanced-cache.php.

I'll submit a PR soon so you can check it.

Thank you so much! It's much appreciated.