Problem/Motivation

At times, it is difficult to figure out why the module appears to not be working, and/or why a particular address is or is not validating.

Proposed resolution

Add an option for adding debugging messages to the log and/or the current page, so that users of this module can figure out if things are working correctly or not.

Remaining tasks

  1. Fix the issue
  2. Create a patch
  3. Review the patch
  4. Commit the patch

User interface changes

There will at least be a modification to the admin UI for en/disabling the functionality, and there will be updates to the UI for the reporting.

API changes

None.

Data model changes

None.

Comments

oadaeh’s picture

Issue summary: View changes
oadaeh’s picture

Assigned: Unassigned » oadaeh

It turns out that I actually kind of need this now, so I started working on it.

oadaeh’s picture

Attached is my initial start at adding debugging logging. It appears to be working fine, but I will do more extensive testing and fine-tuning in the next day or five.

oadaeh’s picture

Status: Active » Needs review
oadaeh’s picture

StatusFileSize
new42.53 KB
new24.69 KB

An updated patch that adds setting the integer-based debugging settings to booleans and sets the defaults to FALSE.

oadaeh’s picture

Issue summary: View changes
aimeerae’s picture

Wow. Fantastic work. I only found nitpicks:

  1. +++ b/email_verify.admin.inc
    @@ -52,6 +52,53 @@ function email_verify_admin_settings($form, &$form_state) {
    +    '#states' => array('enabled' => array(':input[name="email_verify_debug_mode"]' => array('checked' => TRUE))),
    

    This is a bit hard to read. Might be better on separate lines.

  2. +++ b/email_verify.admin.inc
    @@ -52,6 +52,53 @@ function email_verify_admin_settings($form, &$form_state) {
    +    '#states' => array('enabled' => array(':input[name="email_verify_debug_mode"]' => array('checked' => TRUE))),
    

    Same as above.

  3. +++ b/email_verify.admin.inc
    @@ -52,6 +52,53 @@ function email_verify_admin_settings($form, &$form_state) {
    +//  $options = array('loose' => t('Loose (fail for some failures)'), 'medium' => t('Medium (fail for most common failures)'), 'strict' => t('Strict (fail for all failed checks)'));
    +//  $form['email_verify_modes']['email_verify_verification_mode'] = array(
    +//    '#type' => 'select',
    +//    '#title' => t('Check for any DNS records'),
    +//    '#options' => $options,
    +//    '#default_value' => variable_get('email_verify_verification_mode', 1),
    +//    '#description' => t("Use PHP's checkdnsrr() function to see if there are any DNS records associated with the email address' domain name."),
    +//  );
    

    Commented-out code.

  4. +++ b/email_verify.admin.inc
    @@ -124,7 +171,17 @@ function email_verify_admin_settings_submit($form, &$form_state) {
    +        // Log and/or display the debugging information.
    +        $message = implode('<br />', $debugging_text);
    +        if (variable_get('email_verify_debug_mode_record_log', FALSE)) {
    +          watchdog('email_verify debug', $message, array(), WATCHDOG_DEBUG);
    +        }
    +        if (variable_get('email_verify_debug_mode_display_page', FALSE)) {
    +          drupal_set_message($message);
    +        }
    +      }
    

    This is used more than once so add helper function?

  5. +++ b/email_verify.admin.inc
    @@ -124,7 +171,17 @@ function email_verify_admin_settings_submit($form, &$form_state) {
    +          watchdog('email_verify debug', $message, array(), WATCHDOG_DEBUG);
    

    Maybe just 'email_verify' would be better? I think just the module name is typically used.

  6. +++ b/email_verify.check.inc
    @@ -201,16 +201,26 @@ function _email_verify_batch_display_process_batch($user_count, $offset, $disabl
    +      // Log and/or display the debugging information.
    +      $message = implode('<br />', $check_results['debugging_text']);
    +      if (variable_get('email_verify_debug_mode_record_log', FALSE)) {
    +        watchdog('email_verify debug', $message, array(), WATCHDOG_DEBUG);
    +      }
    +      if (variable_get('email_verify_debug_mode_display_page', FALSE)) {
    +        drupal_set_message($message);
    +      }
    

    Use helper function?

  7. +++ b/email_verify.module
    @@ -126,8 +126,20 @@ function email_verify_form_registration_form_alter(&$form, &$form_state, $form_i
    +      // Log and/or display the debugging information.
    +      $message = implode('<br />', $results['debugging_text']);
    +      if (variable_get('email_verify_debug_mode_record_log', FALSE)) {
    +        watchdog('email_verify debug', $message, array(), WATCHDOG_DEBUG);
    +      }
    +      if (variable_get('email_verify_debug_mode_display_page', FALSE)) {
    +        drupal_set_message($message);
    +      }
    

    Use helper function?

We'll get this tested soon.

oadaeh’s picture

1 and 2: Done. I keep waffling on which is better.
3: Removed.
4, 6, and 7: Okay, done.
5: I did that intentionally to allow people to separate the debug messages from module's other messages.

Updated patch attached.

oadaeh’s picture

Assigned: oadaeh » Unassigned
Patrick Storey’s picture

Status: Needs review » Reviewed & tested by the community
StatusFileSize
new322.37 KB

Testing for the patch in comment #8.

Putting the debugging GUI inside /admin/config/system/email_verify is great, and allows for easy customization of the debugging options.

I tested this on a form I had and the output is very good. It printed it right at the top of the page (feels similar to how the devel module would show you debugging outputs which is a positive thing).

I also particularly like how the debugging has a blue background and the reason for why the email address was not validated has the yellow background. This makes things easier to know which is which just based off of background color. I attached a screenshot to point out what I mean by this.

This has passed testing.

  • oadaeh committed f358125 on 7.x-2.x
    Issue #2493515 by oadaeh, Patrick Storey, Aimee Degnan: Add debugging...
oadaeh’s picture

Status: Reviewed & tested by the community » Fixed

This has been committed to the 2.x dev branch. Thanks for the reviews and testing.

oadaeh’s picture

Issue summary: View changes
kristen pol’s picture

Thanks!

oadaeh’s picture

Status: Fixed » Closed (fixed)

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