if the key can't be read throw an exception

I've added a check for the key after we retrieve it, throw an error if it's not found and then return FALSE by which the implementing module should then be able to react to. Also I've fixed a minor error in the drupal settings conf implementation that was throwing errors.

I toyed around with doing a throwing an exception but the only issue is it'll break the exisiting supported modules (including field encrypt) because no one is setup to catch the exception.

For example the implementation in field encrypt is:

function field_encrypt_encrypt($raw) {
  $encrypted = encrypt($raw, array('base64' => TRUE));
  return utf8_encode($encrypted);
}

this will throw fatal errors without having the catch. If we catch it internally what should we do then to notify the other modules that there was an error.

I agree that an exception would be the best way but it'd take patches to all existing modules to do (which could be ok since we are moving to 2.x but wouldn't be a clean upgrade path).

Thoughts?

The Townsend key management module won't be effected by this change as it's only handling the keys and encryption itself. This would add more redundancy to it as well as any future key provider modules.

I agree that now's the time to break the API to make it stronger. We'll want to make a note then to Field Encrypt (who we're still trying to get in hold of to push some changes as well) and other encryption maintainers that the 2.x branch will need them to update. The core encrypt-1.x -> 2.x shouldn't break though.

Attached a new patch for this that replaces the return FALSE with a new exception thrown that we'll have to catch in the implementing modules.

Just saw I forgot to update the status

Greg - is the issue above an error in the test or how I'm throwing the exception? I'm thinking the test needs to be altered to allow for a reply other than an encrypted value.

Yeah I did a simple test with the following code that caught the exception properly:

$text = 'This should be encrypted';
try{
  encrypt($text);
}
catch (Exception $e) {
  dpm($e);
}

I believe the error here is in the test - will debug it further to find out this afternoon.

Gotcha - I'll debug the testing locally. My guess is that we're now returning only an Exception that the test is currently not setup to catch that could be causing the issue (thus the result is the same as the input since it wasn't encrypted). I'll dive into the test and see if it needs some tweaking for the new exception handling.

Real world testing though shows that this is the fix we need, just need to make sure our own test parameters are setup to match.

6: nokey-1867114-6.patch queued for re-testing.

Looks like it passed on a restest. I tested it locally as well and all appeared to be good (though I found a typo in the encrypt.test file). Not sure as to why the test would fail the first time but it appears good now.

Would you like me to add a test into the Encryption Method Plugin Test to check this exception or just leave it as "Needs Review"?

No other feedback or reviews have happened, looks like you guys like it, want to commit it?

I'm getting errors when using the File provider after applying this patch to the latest dev version of the module, so marking as "Needs work" while I investigate.

I figured out the problem. Here's a slightly modified patch.

Tested this by moving my file key out of the directory and received the correct error:

An AJAX HTTP error occurred. HTTP Result Code: 500 Debugging information follows. Path: /batch?id=140&op=do StatusText: Service unavailable (with message) ResponseText: Exception: Key not found in _encrypt_decrypt() (line 103 of /Users/pteglia/Sites/drupal/sites/all/modules/contrib/encrypt/includes/encrypt.encrypt.inc).

Also received an error in the watchdog log:

Exception: Key not found in _encrypt_decrypt() (line 103 of /Users/pteglia/Sites/drupal/sites/all/modules/contrib/encrypt/includes/encrypt.encrypt.inc).

Looks good to me.

Great, thanks. This is committed.