This issue has novice tasks. Therefore if you are an experienced core developer and have multiple commit mentions it is appreciated if instead of doing the task, you review the task done by a novice. Experienced people giving feedback is very valuable.

Problem/Motivation

Followup from #1824778: Convert css_gzip_compression and js_gzip_compression variables to CMI system

Proposed resolution

  • Swap extension_load() and the config() calls? - this would save loading the configuration object if it's never going to needed anyway. This needs to be done in drupal_page_set_cache() and \Drupal\Core\Asset\AssetDumper::dump().
  • Remove section from settings.php about CSS/JS aggregated file gzip compression as we now have a UI.

User interface changes

None

API changes

None

Contributor tasks needed
Task Novice task? Contributor instructions Complete?
Create a patch Instructions
Reroll the patch if it no longer applies. Instructions
Update the issue summary Instructions
Review patch to ensure that it fixes the issue, stays within scope, is properly documented, and follows coding standards Instructions
CommentFileSizeAuthor
#15 interdiff-14-15.txt642 bytesdinarcon
#15 tidy_up_gz_conf-2183075-15.patch2.72 KBdinarcon
#14 tidy_up_gz_conf-2183075-14.patch2.03 KBer.pushpinderrana
#12 tidy_up_gz_conf-2183075-12.patch2.03 KBamitgoyal
#7 tidy_up_gz_conf-2183075-7.patch2.67 KBumar-ahmad
#3 tidy_up_gzip_conf-2183075-2.patch2.65 KBvalderama
#1 tidy_up_gzip_conf-2183075-1.patch2.65 KBvalderama
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

valderama’s picture

valderama CreditAttribution: valderama commented
FileSize
tidy_up_gzip_conf-2183075-1.patch2.65 KB

Here is a first patch, which reverses the order of the extension_load() and the config() calls and removes the section in the default.settings.php

Is the patch fine like this?

valderama’s picture

valderama CreditAttribution: valderama commented
Status: Active » Needs review
valderama’s picture

valderama CreditAttribution: valderama commented
FileSize
tidy_up_gzip_conf-2183075-2.patch2.65 KB
valderama’s picture

valderama CreditAttribution: valderama commented

Removed the whitespace in the if-condition...

lucastockmann’s picture

lucastockmann CreditAttribution: lucastockmann commented

Maybe a side Issue, or could be fixed here:
drupal_page_set_cache() has wrongly documented params.

valderama’s picture

valderama CreditAttribution: valderama commented

The documentation change belongs into a new issue. Should be tagged with "Documentation". And maybe check if there is an issue about this change already.

umar-ahmad’s picture

umar-ahmad CreditAttribution: umar-ahmad commented
FileSize
tidy_up_gz_conf-2183075-7.patch2.67 KB

Rerolled the earlier patch

cilefen’s picture

cilefen CreditAttribution: cilefen commented

7: tidy_up_gz_conf-2183075-7.patch queued for re-testing.

Status: Needs review » Needs work

The last submitted patch, 7: tidy_up_gz_conf-2183075-7.patch, failed testing.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue summary: View changes

Updating for the Austin sprint. See http://www.hook42.com/blog/prepping-drupalcon-austin-sprints-sprint-lead...

dinarcon’s picture

dinarcon CreditAttribution: dinarcon commented
Assigned: Unassigned » dinarcon

Working on this.

amitgoyal’s picture

amitgoyal CreditAttribution: amitgoyal commented
Status: Needs work » Needs review
FileSize
tidy_up_gz_conf-2183075-12.patch2.03 KB

Please review revised patch as the patch in #7 no longer applies.

cilefen’s picture

cilefen CreditAttribution: cilefen commented
Issue tags: +Needs reroll
er.pushpinderrana’s picture

er.pushpinderrana CreditAttribution: er.pushpinderrana commented
Issue tags: -Needs reroll
FileSize
tidy_up_gz_conf-2183075-14.patch2.03 KB

Rerolled above #12 patch.

dinarcon’s picture

dinarcon CreditAttribution: dinarcon commented
Assigned: dinarcon » Unassigned
FileSize
tidy_up_gz_conf-2183075-15.patch2.72 KB
interdiff-14-15.txt642 bytes

The issue summary states that the condition swap should also be made in drupal_page_set_cache() This patch adds to @er.pushpinderrana's patch in #14 to account for that.

znerol’s picture

znerol CreditAttribution: znerol commented
Status: Needs review » Reviewed & tested by the community

Thanks. Patch still applies and it does address the requirements listed in the issue summary.

  • catch committed a255181 on 8.0.x 
    Issue #2183075 by valderama, dinarcon, er.pushpinderrana, amitgoyal,...
catch’s picture

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

Committed/pushed to 8.0.x, thanks!

Status: Fixed » Closed (fixed)

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