Acquia Search is unavailable after failed connections to rpc.acquia.com

The attached patch fixes the issue by storing the derived key salt in a variable so that it is available even when the subscription data is not.

Same patch, removed whitespace differences.

Marking as "needs work" because this needs an upgrade path. The upgrade as it stands would cause a search outage until cron was run. We also probably shouldn't rely on hook_update_N(), because although it is best practice to use this for update stuff, there shouldn't be any downtime when the code is updated. Based on experience, we cannot rely on people running update.php.

Yes, it would. The approach seems solid and fault tolerant, and also avoids the need for a hook_update_N() implementation. Will test and report back.

Also, I am still unclear at to why we are not storing the derived key as an environment variable directly. I understand hat the salt can change, but the Drupal site cannot pick that up until cron is run anyways. Regenerating the derived key during the heartbeat seems like the best approach as opposed to calculating the same hash repeatedly on every search request.

Disregard my comments above. That is an implementation detail that doesn't address the immediate problem and can be changed later if necessary.

No, not yet. Posting a revised patch with some modifications. Stay tuned.

Couple of things, mostly minor...

acquia_search_derived_key_salt($refresh = TRUE); Should just be acquia_search_derived_key_salt(TRUE);

Related, I'm not sure why we need the refresh flag at all. It is a little confusing and adds some unnecessary logic. We could simply add a variable_set() in acquia_search_acquia_subscription_status(), as that hook implementation already has the required subscription data and executes on all the necessary events (i.e. on a heartbeat and when the module is enabled).

if ($subscription && isset($subscription['derived_key_salt'])) { is a minor nitpick, but it can just be if (isset($subscription['derived_key_salt'])) {. isset() will handle this gracefully whether it is an array or an integer.

We can also omit the change to the tests. Even though I added that in the original patch, tests are busted for D7 all together. See #1784990: Revive unit tests for Acquia Search. Selfishly, I don't want to have to re-roll that patch again :-).

Also, It might be helpful to add some detailed documentation to the acquia_search_derived_key_salt() docblock explaining the function and why it is necessary.

but I wanted to make sure the logic can be expanded later on

Understood, and that is a good goal. What logic are you thinking about that could be expanded?

The refresh could come in handy in an update function or when we decide to change the variable name

Yes, we could. We could also add a "set" function if necessary, too. I honestly don't see us changing the variable name, though.

Btw, I'd really love a complete drupalcamp/drupalcon session on the use of empty, isset, !$var and all other checks.

That's actually a really cool idea. The simple breakdown is that !$var and empty() will evaluate to the same thing. The difference is that it $var is undefined then !$var will throw a warning error where as empty($var) will not. The following strings are empty() (or will evaluate to FALSE with !$var)

• "" (an empty string)
• 0 (0 as an integer)
• 0.0 (0 as a float)
• "0" (0 as a string)
• NULL
• FALSE
• array() (an empty array)
• $var; (a variable declared, but without a value)

isset() evaluates differently, in that it will return TRUE for everything but FALSE, NULL, and undefined variables. Like empty, it will not throw notices if the variable passed to it is undefined.

There are conflicting reports on this, but all three methods have roughly the same performance.