Because we are planning to use the keys module at the website of the dutch and flemish drupal community (drupal.nl and drupal.be), I did a thorough review of the keys module.

I did a lot of changes (mostly code documentation, and some changes because of API changes in Drupal 7), which are all in the attached patch file.

Could you please review this patch file so we strive to a usable release of the keys module for Drupal 7?

CommentFileSizeAuthor
review.patch19.65 KBrolf van de krol

Comments

sutharsan’s picture

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

Excellent work. Visually checked the patch, checked it with coder, extracted string with potx (string was extracted from $message[t('Service')]; nice programming work, btw) and worked with the UI.

I will create separate issues for: 'Missing UI configuration link in module page' and 'Move Keys configuration into existing config section'.

sutharsan’s picture

sutharsan commented

For reference:
#1374890: Add configuration link for Keys UI
#1374894: Move Keys configuration into existing category

windmaomao’s picture

windmaomao commented

i didn't try the patch yet.

When i try to install this dev version, I got couple of erros telling me there's no key installed. And if I go to map section, the key is removed as well. So it'll be nice to copy the existing keys of gmap to keys upon installation, other than asking the user to input them again.

Let me know if this review addressed this issue.

shawn_smiley’s picture

shawn_smiley commented

The patch applies correctly. Though you get a lot of undefined variable errors if you don't have a key mapped to the current domain.

Making the following changes to keys_get_key() in keys.module (line 209) resolves the issue:

  if (!isset($key) && !$silent) {  // Changed check to use isset()
    $key = ''; // New Line
    if (!user_access('administer keys')) {