[D7] Address Field Email

Hi lionguard,

First of all, Thanks for your contribution!

While manually reviewing your code, I found couple of security issues.

In views/handlers/addressfield_email_handler_field_address_email_nr.inc file,

  /**
   * Render the addresfields email field.
   */
  public function render($values) {
    $emailValues = unserialize($this->get_value($values));
    $email = $emailValues['email'];
    switch ($this->options['selected_email_type']) {
      case 1:
        return $email;
        break;
    }
  }

Don't just return the email value entered by user, use return $this->sanitize_value($email); instead of return $email;.

In views/addressfield_email.views.inc file,

/**
 * Implements hook_field_views_data_alter().
 */
function addressfield_email_field_views_data_alter(&$result, $field, $module) {
  if ($module == 'addressfield') {
    foreach ($result as $table_name => $table) {
      $field_name = $field['field_name'];
      if (isset($result[$table_name][$field_name])) {
        $field_title = $result[$table_name][$field_name]['title'];
        $group = $result[$table_name][$field_name]['group'];
        $title = $field_title . ' with email';
        $help = $result[$table_name][$field_name . '_data']['help'] . '. Address field email from ' . $field_name . ' field.';
        $result[$table_name]['table']['group'] = t('Address email');
        $result[$table_name][$field_name . '_data'] = array(
          'group' => $group,
          'title' => $title,
          'help' => $help,
          'field' => array(
            'handler' => 'addressfield_email_handler_field_address_email_nr',
            'click sortable' => TRUE,
          )
        );
      }
    }
  }
}

The user facing text should be passed through t() function and use placeholders to replace variables instead of string concatenation.

Also in module file, the t() function and use placeholders to replace variables instead of string concatenation.

Changing the status to "Needs work". Please fix the above issues and change the status back to "Needs review".

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxlionguard2660538git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

Hi lionguard,

Automated review:
http://pareview.sh/pareview/httpsgitdrupalorgsandboxlionguard2660538git
Shows that there are still some issues to fix.
(I think prepare() method in MigrateAddressEmailFieldHandler class will always return NULL. Perhaps typo?)

Manually I spotted that in the addressfield_email_addressfield_types() function it would be nice to move static $addressfields variable above the if condition.
Otherwise it will be running the db query several times on each request.
Try:

function addressfield_email_addressfield_types() {
  static $addressfields;  
  if (!isset($addressfields)) {
    ...
  }

Please update your git clone command:
git clone --branch 7.x-1.x https://git.drupal.org/sandbox/lionguard/2660538.git addressfield_email

In general the code looks pretty nice. There are some strange names like addressfield_email_handler_field_address_email_nr (an email number?) but this should not affect the working of the code.

The same is with the automated review. Some of the function documentation is missing, for further development and people writing patches it might be useful to have a bit more documentation. But again, I don't suppose that should prevent your module from getting accepted.

At some points I believe the code might be overly complicated:

class addressfield_email_handler_field_address_email_nr extends views_handler_field {
  public function option_definition() {
    // Defines one option default.
  }

  public function options_form(&$form, &$form_state) {
    // Shows the option form.
  }

  public function render($values) {
    // Renders the value for the only available option.
  }
}

If you do not plan to support multiple email types (whatever that may be), only the render function will suffice and the options are not really required.

function _addressfield_email_number_options_list() {
  $types = array(
    'email' => 'Email',
  );
  return $types;
}

I suppose that in the code above line 274 ('email' => 'Email',) in addressfield_email.module still needs to be wrapped in t()?

Apart from those issues the code looks pretty good to me. Looking forward to using this.

Oh, one more small thing. The project shortnames of address field and address field phone are addressfield and addressfield_phone; yours is address_field_email according to the git clone command.

git clone --branch 7.x-1.x https://git.drupal.org/sandbox/lionguard/2660538.git address_field_email
cd address_field_email

Perhaps it is an idea to remove the first underscore so the folder name matches the module name? This makes the module compatible with Drush and follows the other modules :)

Please add a comma on the last multiline array item. Like..

array(
'description' => $name ? t('!label of field %name', array(
'!label' => $value['label'],
'%name' => $name,
)

Please check it on all code.

@addonsolutions: looks like you forgot to change the status. Is this now RTBC after your review or are there application blockers left and this should be set to "needs work"?

@lionguard

I think first of all you need to update all the errors and warnings of automated review tools.
https://pareview.sh/node/602

Thank you.

Closing due to lack of activity. If you are still working on this application, you should fix all known problems and then set the status to "Needs review". (See also the project application workflow).

I'm a robot and this is an automated message from Project Applications Scraper.