system.performance variables were converted with the original/initial #1270608: File-based configuration API.

The converted configuration object + settings form need to be adjusted for the guidelines in
#1599554: Tutorial/guidelines for how to convert variables into configuration

Files: 
CommentFileSizeAuthor
#23 interdiff.txt780 bytesalexpott
#23 1669902.drupal8.config-system-performance.23.patch17.62 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 40,362 pass(es). View
#20 drupal8.config-system-performance.20.patch16.71 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 40,299 pass(es), 79 fail(s), and 1 exception(s). View
#19 drupal8.config-system-performance.19.patch18.85 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 39,807 pass(es). View
#17 drupal8.config-system-performance.17.patch19.3 KBaspilicious
PASSED: [[SimpleTest]]: [MySQL] 39,808 pass(es). View
#8 drupal8.config-system-performance.8.patch19.2 KBaspilicious
FAILED: [[SimpleTest]]: [MySQL] 39,758 pass(es), 1 fail(s), and 0 exception(s). View
#4 drupal8.config-system-performance.4.patch514 bytessun
FAILED: [[SimpleTest]]: [MySQL] 36,827 pass(es), 339 fail(s), and 14 exception(s). View

Comments

sun’s picture

Issue tags: +API change

All config system conversions are API changes, so tagging as such.

aspilicious’s picture

hmm, not sure about the naming

cache: '0'
page:
    cache_maximum_age: '0'
    compression: '0'
preprocess:
    css: '0'
    js: '0'

or

cache:
    enabled: '0'
    maximum_age: '0'
compression: '0'
preprocess:
    css: '0'
    js: '0'

or ???

sun’s picture

Status: Active » Needs review
FileSize
514 bytes
FAILED: [[SimpleTest]]: [MySQL] 36,827 pass(es), 339 fail(s), and 14 exception(s). View

Yeah, indeed a bit tough :)

How about something like this?

Status: Needs review » Needs work

The last submitted patch, drupal8.config-system-performance.4.patch, failed testing.

alexpott’s picture

I wonder if the compression variable should reflect the fact the we can respond to more than just HTTP and in order for compression to work the request header HTTP_ACCEPT_ENCODING must be set and contain the value gzip. So maybe something like:

cache:
    page:
        enabled: '0'
        max_age: '0'
bandwidth_optimization:
    http_response_gzip: '0'
preprocess:
    css: '0'
    js: '0'

Or maybe http_encode_gzip instead of http_response_gzip?

sun’s picture

Status: Needs work » Needs review

I like the more specific "gzip" instead of the overly generic "compression", but on the rest, I think that "bandwidth", "optimization", and "http" are too wordy and implementation-specific...?

I'd also kinda hate to see ->get('bandwidth_optimization.http_response_gzip')... ;)

cache:
    page:
        enabled: '0'
        max_age: '0'
response:
    gzip: '0'
preprocess:
    css: '0'
    js: '0'

It still does not feel 100% optimal to me, but nevertheless, I hope we can jump on a patch soon. (In-patch renames of keys are possible anytime.)

aspilicious’s picture

FileSize
19.2 KB
FAILED: [[SimpleTest]]: [MySQL] 39,758 pass(es), 1 fail(s), and 0 exception(s). View

Done (hopefully)

Status: Needs review » Needs work
Issue tags: -Novice, -API change, -Configuration system, -Config novice

The last submitted patch, drupal8.config-system-performance.8.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +Novice, +API change, +Configuration system, +Config novice

The last submitted patch, drupal8.config-system-performance.8.patch, failed testing.

aspilicious’s picture

This is fun...

      $parts = explode('.', $key);
      if (count($parts) == 1) {
        return isset($merged_data[$key]) ? $merged_data[$key] : NULL;
      }
      else {
        $key_exists = NULL;
        $value = drupal_array_get_nested_value($merged_data, $parts, $key_exists);
        return $key_exists ? $value : NULL;
      }

When there are multiple parts, drupal_array_get_nested_value gets called. But in early bootstrap common.inc isn't loaded yet.
So we should copy the functionality or move the function to bootstrap.inc. (or any other file that get bootstrapped soon)

Anyone has another idea?

aspilicious’s picture

And isn't there an update function somewhere for system performance settings? o_O

sun’s picture

Assigned: Unassigned » catch

Thanks for jumping on this, @aspilicious!

Created #1704196: Remove Config's dependencies on procedural Drupal code in includes/common.inc to resolve the bootstrap dependency issue.

I'm also not 100% sure on the config object keys/naming yet. One of the most knowledgeable persons I'd associate with these settings apparently is @catch... so I hope he can give this a quick review to tell us whether the proposed keys/names are technically correct and also, in line with current/upcoming changes?

aspilicious’s picture

Status: Needs work » Needs review
Issue tags: -Novice, -API change, -Configuration system, -Config novice

Status: Needs review » Needs work
Issue tags: +Novice, +API change, +Configuration system, +Config novice

The last submitted patch, drupal8.config-system-performance.8.patch, failed testing.

aspilicious’s picture

Status: Needs work » Needs review
FileSize
19.3 KB
PASSED: [[SimpleTest]]: [MySQL] 39,808 pass(es). View

Should pass now..

aspilicious’s picture

alexpott’s picture

FileSize
18.85 KB
PASSED: [[SimpleTest]]: [MySQL] 39,807 pass(es). View

We're missing an upgrade path from variables to config... and what about existing drupal 8 install?

The patch attached adds the missing upgrade from variables. Not sure how to deal with existing drupal 8 sites...

 /**
+ * Moves system performance settings from variable to config.
+ *
+ * @ingroup config_upgrade
+ */
+function system_update_8015() {
+  update_variables_to_config('system.site', array(
+    'cache' => 'cache.page.enabled',
+    'page_cache_maximum_age' => 'cache.page.max_age',
+    'page_compression' => 'response.gzip',
+    'preprocess_css' => 'preprocess.css',
+    'preprocess_js' => 'preprocess.js',
+  ));
+}

The patch also changes the key order in the YAML file to be alphabetical.

-cache: '0'
-page_cache_maximum_age: '0'
-page_compression: '0'
-preprocess_css: '0'
-preprocess_js: '0'
+cache:
+    page:
+        enabled: '0'
+        max_age: '0'
+preprocess:
+    css: '0'
+    js: '0'
+response:
+    gzip: '0'
+

For some reason the inderdiff was weird...

alexpott’s picture

Priority: Normal » Critical
FileSize
16.71 KB
FAILED: [[SimpleTest]]: [MySQL] 40,299 pass(es), 79 fail(s), and 1 exception(s). View

Rerolled patch to apply to latest 8.x

Also marking this as critical as we've broken the upgrade path as there currently is no migration of these settings for 7.x -> 8.x

Status: Needs review » Needs work

The last submitted patch, drupal8.config-system-performance.20.patch, failed testing.

catch’s picture

Assigned: catch » Unassigned

fwiw the naming here looks fine to me, and I agree with renaming maximum_age to max_age since it's actually setting that in the cache-control header.

alexpott’s picture

Status: Needs work » Needs review
FileSize
17.62 KB
PASSED: [[SimpleTest]]: [MySQL] 40,362 pass(es). View
780 bytes

Some of new code was doing $config->get('cache')

Status: Needs review » Needs work
Issue tags: -Novice, -API change, -Configuration system, -Config novice

The last submitted patch, 1669902.drupal8.config-system-performance.23.patch, failed testing.

alexpott’s picture

Status: Needs work » Needs review
Issue tags: +Novice, +API change, +Configuration system, +Config novice
tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Looks great!

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks! :D

sun’s picture

Awesome job guys! The new keys look really clean and self-descriptive to me! :)

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.