The SPI data update process is opaque and unclear where to find the issue when debugging so NSPI should respond to SPI updates with contextual messages.

For some variables, Insight should allow the user to automatically make the recommended change.

Connector should verify the server response HMAC, report on NSPI messages, and if set_variables element is set then set the variables there that are in the local, approved list and that are not ignored.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

coltrane’s picture

Status: Active » Needs review
FileSize
23.91 KB

Patch bumps SPI data version so NSPI will set response HMAC.

NSPI messages are printed on the status page when manually sending SPI data as shown in this screenshot. Additionally a watchdog message is made.
http://monosnap.com/image/R2fwEKOWRCMbw1bJwLgOkXwl5

The default list of variables that can be automatically set from Insight is array('acquia_spi_set_variables_automatic', 'error_level', 'preprocess_js', 'page_cache_maximum_age', 'block_cache', 'preprocess_css', 'page_compression', 'cache', 'cache_lifetime'). The default list needs to be expanded before this is committed.

This list is in variable acquia_spi_set_variables_automatic and while the variable itself is in the list it's also explicitly added to the 'ignored' list unless the user has allowed the list to be overridden. Screenshot of override option https://monosnap.com/image/sJD7BmJO5rOY5AdS62dCa1Q9p
And text:

Allow Insight to update list of approved variables
Insight can set variables on your site to recommended values at your approval, but only from a specific list of variables. Check this box to allow Insight to update the list of approved variables. Learn more.
coltrane’s picture

Update patch includes insight send method and makes minor changes to _acquia_send_spi() menu callback.

coltrane’s picture

Minor update adds logging on variable sets.

gcassie’s picture

Status: Needs review » Needs work
  1. +++ b/acquia_agent/acquia_agent.module
    @@ -549,6 +549,8 @@ function acquia_agent_help($path, $arg) {
    +      $output .= '<dt>' . t('Allow Insight to update list of approved variables') . '</dt>';
    

    Sentence should end with a period to match (most of) the other options on the page.

  2. +++ b/acquia_spi/acquia_spi.module
    @@ -140,21 +141,39 @@ function _acquia_spi_send_access() {
    +        $message .= t(' Acquia Network returned the following messages. Further information may also be logged. <br/>');
    

    Translated strings shouldn't start with a space, and the break tag should be outside the t().

  3. +++ b/acquia_spi/acquia_spi.module
    @@ -140,21 +141,39 @@ function _acquia_spi_send_access() {
    +          $message .= check_plain($nspi_message) . '<br/>';
    

    Actually maybe just make a $messages array and then implode it in the dsm call.

  4. +++ b/acquia_spi/acquia_spi.module
    @@ -261,6 +280,12 @@ function acquia_spi_form_acquia_agent_settings_form_alter(&$form) {
    +    '#title' => t('Allow Insight to update list of approved variables'),
    

    End with a period.

  5. +++ b/acquia_spi/acquia_spi.module
    @@ -1420,3 +1466,48 @@ function acquia_spi_hash_path($path = '') {
    +      variable_set($key, $value);
    

    We should do a variable_get after the variable_set to see if the value actually changed. If it didn't that's a sign it's being set in $conf or strongarm or such and we can set a notification that the subscriber is going to have to make the change themselves.

coltrane’s picture

Updated patch for everything but the last point.

"We should do a variable_get after the variable_set to see if the value actually changed. " is a great idea, but I'll have to check on a new page refresh. Let me investigate how that could be done.

coltrane’s picture

Lightly tested D6 version.

gcassie’s picture

Why do you have to test that the variable changed on a page refresh? If you do a variable_set() the new value is immediately available in a variable_get(). So something like:

  $failures = array();
  foreach($set_variables as $key => $value) {
    // Approved variables get set immediately unless ignored.
    if (in_array($key, $whitelist) && !in_array($key, $ignored)) {
      $tmp = variable_get($key, NULL);
      $saved[] = $key;
      variable_set($key, $value);
      if (variable_get($key, NULL) != $tmp) {
        $failures[$key] = $value;
      }
    }
  }

And then report back on $failures.

Otherwise looks good.

coltrane’s picture

Committed slightly modified #5 http://drupalcode.org/project/acquia_connector.git/commit/e4613e4 for 2.x-dev testing. Will make followup patch for D7 for any changes.

coltrane’s picture

Update to allowed D7 variables.

per https://drupal.org/node/2081981#comment-7867181. While immediately checking variable_get() will show it fixed, it will be reset by the next time settings.php is loaded or Strongarm does its thang. I think I'm going to hold off right now on a solution for now and come back to this in followup. Insight itself could attempt to handle this use case.

coltrane’s picture

Status: Needs work » Needs review
FileSize
9.94 KB
1.7 KB

Updated D6 full patch and minor updates to D7

coltrane’s picture

Status: Fixed » Closed (fixed)

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