Problem/Motivation

There's a config:

system.performance response.gzip

...which is defined in core/modules/system/config/schema/system.schema.yml and is mapped to the D6/7 variable page_compression e.g. in core/modules/system/migration_templates/d6_system_performance.yml

However, after the changes to page cache in #2368987: Move internal page caching to a module to avoid relying on config get on runtime it seems that response.gzip doesn't actually do anything any more.

In fact, it looks like consensus was reached in #28 of that issue that this should be removed:

catch and [Wim Leers] agreed that the best way forward was to simply remove the "Compress cached pages"/response.gzip

It seems that the changes committed didn't remove response.gzip though.

Proposed resolution

Remove response.gzip and remaining references to it e.g.

core/modules/page_cache/src/Tests/PageCacheTest.php:228:    $config->set('response.gzip', 1);
core/modules/system/migration_templates/d6_system_performance.yml:17:  'response/gzip': page_compression
core/modules/page_cache/src/StackMiddleware/PageCache.php:184:   * If page_compression is enabled, a gzipped version of the page is stored in

core/modules/system/config/schema/system.schema.yml:189:        gzip:
core/modules/system/config/schema/system.schema.yml-190-          type: boolean
core/modules/system/config/schema/system.schema.yml-191-          label: 'Compress cached pages'

Remaining tasks

Review patch to remove response.gzip and remaining references to it.

User interface changes

None - the "Compress cached pages" option is already gone from the UI.

API changes

None.

Data model changes

None.

CommentFileSizeAuthor
#27 remove_response_gzip-2775381-27.patch7.62 KBmcdruid
#23 remove_response_gzip-2775381-23.patch7.5 KBmcdruid
#22 remove_response_gzip-2775381-22.patch7.54 KBmcdruid
#19 remove_response_gzip-2775381-19.patch6.99 KBmcdruid
#17 remove_response_gzip-2775381-17.patch5.58 KBmcdruid
#13 remove_response_gzip-2775381-13.patch4.58 KBmcdruid
#8 interdiff.txt631 bytesmcdruid
#8 remove_response_gzip-2775381-8.patch4.4 KBmcdruid
#2 remove_response_gzip-2775381-2.patch3.7 KBmcdruid

Comments

mcdruid created an issue. See original summary.

mcdruid’s picture

mcdruid commented
StatusFileSize
new remove_response_gzip-2775381-2.patch3.7 KB

Patch to remove response.gzip and related code / comments.

I'm not 100% sure about removing the "Negotiate whether to use compression." section from StackMiddleware/PageCache.php but AFAICS the cached response's content will never be gzipped, so I'm fairly sure this code is redundant.

mcdruid’s picture

mcdruid commented
Issue summary: View changes
Status: Active » Needs review
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Component: cache system » page_cache.module
Category: Bug report » Task
Status: Needs review » Reviewed & tested by the community

This is correct, per #2368987-28: Move internal page caching to a module to avoid relying on config get on runtime.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Also, YAY! Less code! :D

Thanks, @mcdruid!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Needs review

Shouldn't this have an update to remove the key from installed configuration?

znerol’s picture

znerol commented

I'm a bit confused about the PageCacheTest. We have assertions for the Vary response header and we test explicitly for Cookie, Accept-Encoding. I'd expect that this assertion needs to be modified when throwing out gzip support. On the other hand, there are no functional changes. Maybe the Accept-Encoding bit comes from the web server?

mcdruid’s picture

mcdruid commented
StatusFileSize
new remove_response_gzip-2775381-8.patch4.4 KB
new interdiff.txt631 bytes

Shouldn't this have an update to remove the key from installed configuration?

Good point - added.

I asked in IRC about the addtogroup updates-8.2.x and xjm mentioned that there's quite a bit of inconsistency in core... e.g. should it be 8.2.x or 8.2.0? I've gone with the former, but open to suggestions.

Status: Needs review » Needs work

The last submitted patch, 8: remove_response_gzip-2775381-8.patch, failed testing.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

#6: good point! Sorry. Was too enthusiastic about deleting code :P

mcdruid’s picture

mcdruid commented
Status: Needs work » Needs review

Not sure why the patch in #8 failed tests first time around; re-ran them and it passed this time.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mcdruid’s picture

mcdruid commented
StatusFileSize
new remove_response_gzip-2775381-13.patch4.58 KB

Needed a reroll. One reason was that a new update_hook_N has been added to system.install - and it looks like no addtogroup comments were included... 

/**
 * @} End of "addtogroup updates-8.0.0-rc".
 */

/**
 * Fix configuration overrides to not override non existing keys.
 */
function system_update_8200(&$sandbox) {

For now, I've updated this issue's update hook including the comments, and will file a new issue for the addtogroup comments which are seemingly absent from system_update_8200.

naveenvalecha’s picture

naveenvalecha
He/Him
commented
Status: Needs review » Needs work
Issue tags: +Needs tests 
+++ b/core/modules/system/system.install
@@ -1680,3 +1680,23 @@ function system_update_8200(&$sandbox) {
+ ¶
+/**
+ * @addtogroup updates-8.3.x
+ * @{
+ */
+
+/**
+ * Remove response.gzip from system module config.
+ */
+function system_update_8300() {
+  $config_factory = \Drupal::configFactory();
+  $system_performance_config = $config_factory->getEditable('system.performance');
+  $system_performance_config
+    ->clear('response.gzip')
+    ->save(TRUE);
+}
+
+/**
+ * @} End of "addtogroup updates-8.3.x.
+ */

Add the hook_post_update_NAME for the config changes so that we can also test it as well. https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented

Right, I'm afraid @naveenvalecha is right: this needs an upgrade path test. Fortunately it's fairly simple in this case. Good examples: \Drupal\field\Tests\Update\EntityReferenceHandlerSettingUpdateTest, \Drupal\system\Tests\Update\FilterHtmlUpdateTest::testAllowedHtmlUpdate, \Drupal\rest\Tests\Update\RestExportAuthUpdateTest.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mcdruid’s picture

mcdruid commented
Status: Needs work » Needs review
StatusFileSize
new remove_response_gzip-2775381-17.patch5.58 KB

Patch needed a re-roll (more hook_update_N's have been added to the system module in the meantime).

I've added an Update test which sets response.gzip, runs updates and then checks that the config no longer exists... hoping I got the right idea there.

I've not added a hook_post_update_NAME as I don't see what the purpose of that would be, but happy to look at that again if I'm missing something.

Status: Needs review » Needs work

The last submitted patch, 17: remove_response_gzip-2775381-17.patch, failed testing.

mcdruid’s picture

mcdruid commented
StatusFileSize
new remove_response_gzip-2775381-19.patch6.99 KB

Removed response.gzip from a D6 and D7 migration test, moved the hook_update_N into updates-8.4.x (instead of 8.3.x), and removed response from config as well as response.gzip in that update hook... if I'm interpreting the test failures correctly, this is necessary as we're removing the whole response config key from the schema.

mcdruid’s picture

mcdruid commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 19: remove_response_gzip-2775381-19.patch, failed testing.

mcdruid’s picture

mcdruid commented
Status: Needs work » Needs review
StatusFileSize
new remove_response_gzip-2775381-22.patch7.54 KB

Removed another mapping of response/gzip for D7 migration.

mcdruid’s picture

mcdruid commented
StatusFileSize
new remove_response_gzip-2775381-23.patch7.5 KB

Have a feeling removing response as well as response.gzip is probably not necessary; see whether tests will pass without.

Status: Needs review » Needs work

The last submitted patch, 23: remove_response_gzip-2775381-23.patch, failed testing.

mcdruid’s picture

mcdruid commented
Status: Needs work » Needs review

Ok, so looks like perhaps it is necessary to remove response in the hook_update_N, unless I'm misunderstanding what's going on. Back to the patch in #22.

mcdruid’s picture

mcdruid commented
mcdruid’s picture

mcdruid commented
StatusFileSize
new remove_response_gzip-2775381-27.patch7.62 KB

Re-roll of #22 (which is deliberate, #23 didn't pass tests without removing response entirely).

The re-roll included removing the addtogroup updates-8.4.x comments per #2860096: Remove api doc groups for updates eg. updates-8.2.x-to-8.3.x.

wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

Thanks!

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 8.4.x, thanks!

  • catch committed bae3b3d on 8.4.x 
    Issue #2775381 by mcdruid, Wim Leers, catch, znerol, naveenvalecha:...
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Issue tags: +maintainability
wim leers’s picture

wim leers
Location Ghent 🇧🇪🇪🇺
commented
Related issues: +#2892179: Follow-up for #2775381: clearing render cache in PerformanceForm::submitForm() is obsolete

Something was forgotten here: #2892179: Follow-up for #2775381: clearing render cache in PerformanceForm::submitForm() is obsolete.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.