Problem/Motivation

There are other modules out there that provide forms for adding email addresses. However, it is impractical for this module to provide custom solutions for all the possible modules available.
Also, it's entirely likely that a site might have custom forms and email address fields that people want checked.

Proposed resolution

Modify the admin form and the code to provide a generic way for specifying the form and email address field, such that the admin can specify any form they want to verify the email address on. This requires specifying both the form ID and the field ID the email address is in.

Remaining tasks

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

User interface changes

The admin UI will need to be modified to allow this.

API changes

To be determined.

Data model changes

To be determined.

CommentFileSizeAuthor
#4 email_verify-flexible_form_verification-2426859-4-D7.patch8.18 KBoadaeh
#7 interdiff-2426859-4-6.txt3.68 KBoadaeh
#6 email_verify-flexible_form_verification-2426859-6-D7.patch8.9 KBoadaeh
#11 interdiff-with-w-2426859-6-11.txt5.33 KBoadaeh
#11 interdiff-2426859-6-11.txt8.06 KBoadaeh
#11 email_verify-flexible_form_verification-2426859-11-D7.patch10.17 KBoadaeh

Comments

oadaeh’s picture

oadaeh commented
Assigned: Unassigned » oadaeh

I've started working on this.

oadaeh’s picture

oadaeh commented
Version: 7.x-1.x-dev » 7.x-2.x-dev

Moving to the new current development branch.

oadaeh’s picture

oadaeh commented
Issue summary: View changes

Updating issue description and attribution.

oadaeh’s picture

oadaeh commented
Assigned: oadaeh » Unassigned
Status: Active » Needs review
StatusFileSize
new email_verify-flexible_form_verification-2426859-4-D7.patch8.18 KB

The attached patch provides a mechanism for achieving the desired results.

oadaeh’s picture

oadaeh commented

I feel I should add that a side benefit with the patch is that you can also exclude forms and/or fields by not including them in the list.

oadaeh’s picture

oadaeh commented
StatusFileSize
new email_verify-flexible_form_verification-2426859-6-D7.patch8.9 KB

Updated patch that:

  • Removes variable_get() calls from within empty() calls in the install file.
  • Separates updates into their own functions: one for updating the form/field list, and one for setting the module's weight to 1000.
  • Fixes some bugs with the form/field processing.
oadaeh’s picture

oadaeh commented
StatusFileSize
new interdiff-2426859-4-6.txt3.68 KB

Here's the interdiff.

oadaeh’s picture

oadaeh commented
Issue summary: View changes
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

This feature is super useful. I've reviewed the code:

  1. +++ b/email_verify.install
@@ -45,3 +45,56 @@ function email_verify_update_7100() {
+function email_verify_update_7201() {
+  $forms = '';

    Is it possible to go to the settings page and change it before running updatedb? If so, then the new variable should be used here even though 99.999% of the time it would be empty. 

  2. +++ b/email_verify.module
@@ -66,68 +66,56 @@ function email_verify_access_people_email_verify() {
+        foreach($forms_and_fields as $forms_and_field) {

    Minor: space missing after foreach. 

  3. +++ b/email_verify.module
@@ -66,68 +66,56 @@ function email_verify_access_people_email_verify() {
+  if (isset($form_state['values']['op']) && $form_state['values']['op'] != t('Cancel account')) {

    Might there be other "cancel" actions that should be ignored? Since any form can be added now, this is hard to know. Would it be better to just check the 'op' is 'submit'? Are their forms we should be handling where that is not the case? 

  4. +++ b/email_verify.module
@@ -66,68 +66,56 @@ function email_verify_access_people_email_verify() {
+        foreach($forms_and_fields as $forms_and_field) {

    Minor: space missing after foreach. 

  5. +++ b/email_verify.module
@@ -66,68 +66,56 @@ function email_verify_access_people_email_verify() {
+              $error = email_verify_check($form_state['values'][$field]);

    Check field exists before using it? 

  6. +++ b/email_verify.module
@@ -66,68 +66,56 @@ function email_verify_access_people_email_verify() {
+              if ($error) {

    Use if (!empty($error))? 

  7. +++ b/email_verify.module
@@ -66,68 +66,56 @@ function email_verify_access_people_email_verify() {
+                form_set_error($form_state[$field], $error);

    Should this just be:

    form_set_error($field, $error)?

  8. The only other thing is that it might be useful to have a function like:

    email_verify_get_forms_and_fields

    which gets the data from the variable and makes into a keyed array, e.g.

    array(
  'my_custom_form' => 'email_address',
  'another_form' => 'mail',
);

    and then use those in the 2 functions that need to look at this data. Then it will be there if other functions need at it as well down the road.

oadaeh’s picture

oadaeh commented
Status: Needs review » Needs work

The only other thing is that it might be useful to have a function like:
email_verify_get_forms_and_fields

which gets the data from the variable and makes into a keyed array, e.g.

array(
  'my_custom_form' => 'email_address',
  'another_form' => 'mail',
);

and then use those in the 2 functions that need to look at this data. Then it will be there if other functions need at it as well down the road.

While the idea has merit, the suggested implementation won't work for anyone who wants to verify two or more fields in a single form.

Worse, however, is that while attempting to implement this, I discovered it doesn't handle Field API fields. And while I'm at it, I should also make sure it handles multi-value fields.

oadaeh’s picture

oadaeh commented
Status: Needs work » Needs review
StatusFileSize
new email_verify-flexible_form_verification-2426859-11-D7.patch10.17 KB
new interdiff-2426859-6-11.txt8.06 KB
new interdiff-with-w-2426859-6-11.txt5.33 KB

1: Yes. I suppose. Done.
2 and 4: Fixed.
3: The reason for it's existence was to not validate a bad email address, when the user is being canceled. That is extremely frustrating. However, in this new context, that doesn't necessarily make as much sense, so I changed it.
5: Okay.
6: Yes, that's probably a better choice.
7: No, because $error is the message that is displayed to the user (and why your #6 makes more sense).
8: Okay, I came up with a solution kind of does what you say, but in a different way. The external function returns an array of fields for a given form ID, rather than the whole structure. I can't think of a reasonable situation where the whole thing would be needed at any one time.

I also addressed the Field API form field elements and multi-value fields.

Patrick Storey’s picture

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

I tested the patch in comment #11, I was able to turn the module on for a form and it caught an invalid domain email address that I entered to test.

I proceeded to removed that form, from the "FORMS TO CHECK" section of admin/config/system/email_verify. Cleared caches and tested an invalid domain email address on the form and it did pass. Proving that the form is no longer being checked by the email verify module.

Usability here seems intuitive and easy.

This has passed testing.

  • oadaeh committed 40c5911 on 7.x-2.x 
    Issue #2426859 by oadaeh, Kristen Pol, Patrick Storey: Allow for more...
oadaeh’s picture

oadaeh commented
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

oadaeh commented
Issue summary: View changes
kristen pol’s picture

kristen pol
she/her
Primary language English
Location Santa Cruz, CA, USA
commented

Thanks!

oadaeh’s picture

oadaeh commented

Status: Fixed » Closed (fixed)

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