Problem/Motivation

We use patch https://www.drupal.org/files/issues/advagg-handle-external-same-host-js-... on advagg 2.4.0 to allow external JS to pass through the optimizer without being converted to a file. With the 3.x branch, the JS option was retained but there are two issues.

  1. There is a bug in the SettingsForm that the "Preserve External" checkbox configuration is not saved. The issue is that the form_state key in the #submitForm is "js.preserve_external" but in the #buildForm it is setup with "js_preserve_external" (which is the syntax the rest of the keys use).
  2. We need the same functionality for CSS (not a bug of course. This would be an enhancement).

Proposed resolution

Per above, solution for each item is as follows.

  1. Fix the key in the form_state->getValue() to use "js_preserve_external".
  2. Add the same logic to the CSS optimizer, SettingsForm, and default settings.

Remaining tasks

None.

User interface changes

Added the same "preserve external" checkbox to CSS options as the JS options.

API changes

None.

Data model changes

Added new css.preserve_external to settings.yml

2.4.x ticket for reference

External JS hosted by same domain as Drupal converted to file
+ the addition of

Comments

sdewitt created an issue. See original summary.

sdewitt’s picture

Attached a patch that fixes #1 described in description and adds feature function for #2.

sdewitt’s picture

sdewitt’s picture

Sorry, numbered the first patch file wrong. No diff between patch 2 (comment #3 :-( ) and patch 4 - correctly numbered so no interdiff added.

nickdickinsonwilde’s picture

Assigned: sdewitt » nickdickinsonwilde
Status: Active » Needs review

Thanks for catching that bug and the new functionality, I'm reviewing it now.
In the future, no need to take the time if your patch file is numbered wrong - unless there is a ton of comments/files/long issue, its totally fine. Also, best practice to set Status to 'needs review' and unassign yourself from issue when you've put a patch up. In this case it had no effect, but on a busy issue queue, needs review is often treated as higher priority than active - less work for more benefit usually.

  • sdewitt authored 6b68dc1 on 8.x-3.x
    Issue #2943275 by sdewitt: External JS/CSS hosted by same domain as...
nickdickinsonwilde’s picture

Status: Needs review » Fixed

Patch committed, thanks.

nickdickinsonwilde’s picture

Assigned: nickdickinsonwilde » Unassigned
sdewitt’s picture

Thanks for the best practice advice @NickWilde. I will do this in the future. And thanks for the quick turnaround on the commit!

Status: Fixed » Closed (fixed)

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