[D7] Email Field Verification

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

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.

More details

Your module depends on the Email module, but your documentation doesn't specifically state that. Upon closer reading, I realize that I should have inferred the need to install the Email module, but being explicit about deoendecies is very important.

Upon performing a test, I got the email, but clicking the link lead to an error page saying "The mail address could not be confirmed." Debugging showed that the $hash parameter for _email_field_verification_link_confirm() was always an empty string. This is because email_field_verification_menu() defined this callback's "page arguments" value as 3, when it should have been 2. The "page arguments" value is super confusing to me, too.

Fixing that bug led to the successful "Your mail address has been confirmed" page. Yay!

So then I tried to force a late confirmation by waiting too long (I also added a 1-minute timeout option, for convenience). I got the "It took you too long to confirm this mail address" message, which is good... but I set the option to "Delete entity/content" as the timeout response, and I unfortunately got these errors as well:

Warning: Missing argument 2 for entity_delete(), called in /drupal_testing/sites/all/modules/email_field_verification/email_field_verification.module on line 604 and defined in entity_delete() (line 268 of /drupal_testing/sites/all/modules/entity/entity.module).
Notice: Undefined variable: id in entity_delete() (line 269 of /drupal_testing/sites/all/modules/entity/entity.module).
Warning: class_implements() [function.class-implements]: object or string expected in entity_delete_multiple() (line 290 of /drupal_testing/sites/all/modules/entity/entity.module).
Warning: in_array() expects parameter 2 to be array, boolean given in entity_delete_multiple() (line 290 of /drupal_testing/sites/all/modules/entity/entity.module).

That's two major functionality-breaking bugs. I'm runnign this on an almost totally clean Drupal install, so my best guess is that your testing setup may have been relying on modules you didn't know about, or your edits to satisfy PAReview may have broken something. Please repair these errors so that your module can run on a clean Drupal site, and I'll be glad to continue my review.

Good catch. Wonder nobody came up with this idea yet.

Made a quick review. Getting errors when calling verification links:

Notice: Undefined index: instance_id in _email_field_verification_link_confirm() (line 461 of /path/to/drupal-7.23/sites/all/modules/sandbox/email_field_verification/email_field_verification.module).

PDOException: SQLSTATE[42S22]: Column not found: 1054 Unknown column 'instance_id' in 'where clause': UPDATE {email_field_verification_data} SET status=:db_update_placeholder_0 WHERE (entity_id = :db_condition_placeholder_0) AND (entity_type = :db_condition_placeholder_1) AND (instance_id = :db_condition_placeholder_2) AND (email = :db_condition_placeholder_3) ; Array ( [:db_update_placeholder_0] => 1 [:db_condition_placeholder_0] => 3 [:db_condition_placeholder_1] => node [:db_condition_placeholder_2] => [:db_condition_placeholder_3] => second@web.de ) in _email_field_verification_link_confirm() (line 463 of /path/to/drupal-7.23/sites/all/modules/sandbox/email_field_verification/email_field_verification.module).

Additionally I'm wondering about the flow:

• You should mention that the default publishing state for content types is relevant. I thought your module would handle unpublishing nodes while email-field values are queued for confirmation.
• Shouldn't email-field values be suppressed or anything like that until confirmed. If not yet, maybe such an option could/should be added.

I'll have a look at your sandbox again. So far good work, hope to see this released soon.

add hook_uninstall to email_field_verification.install and remove email_field_verification_data table

Review of the 7.x-1.x branch:

• DrupalPractice has found some issues with your code, but could be false positives.
  

  
  FILE: /home/klausi/pareview_temp/email_field_verification.module
  --------------------------------------------------------------------------------
  FOUND 0 ERROR(S) AND 1 WARNING(S) AFFECTING 1 LINE(S)
  --------------------------------------------------------------------------------
   84 | WARNING | #description values usually have to run through t() for
      |         | translation
  --------------------------------------------------------------------------------
  

This automated report was generated with PAReview.sh, your friendly project application review script. You can also use the online version to check your project. You have to get a review bonus to get a review from me.

manual review:

1. email_field_verification_schema(): The comment what the "status" columns can mean should actually be in a description property of the column, so that this info is available on the database table directly (for example when looking at it with Phpmyadmin).
2. _email_field_verification_get_settings_defaults(): doc block is wrong? This should describe what your function does, at least briefly. Same at _email_field_verification_delete(), please do not add useless doc blocks.
3. "'#description' => 'Should the email require verification by link sent to it.',": all user facing text must run through t() for translation, please check all your strings.
4. email_field_verification_entity_delete(): $entity_type is missing in the query, so you could delete multiple records from other entity types, which can lead to unintended data loss.
5. email_field_verification_permission(): why is the first permission marked as "'restrict access' => TRUE,"? Please add a comment. Does this permission allow a user to take over the site?
6. email_field_verification_field_update_instance(): the column instance_id does not exist on the table email_field_verification_data, so this will throw database exceptions?
7. Your module file is 800 lines long already, you could move page callbacks and other conditionally needed functions to include files?
8. email_field_verification_cron(): this will not scale. As soon as you have more than a couple of hundreds databse rows cron will run into timeouts. I think you should construct a select query that only picks entries where the timestamp has been exceeded, that would reduce the number of entries, correct?
9. "$entity_info = entity_get_info($entity->type);": this will only work for nodes, other entities might have the bundle in a different property. use entity_extract_ids() instead.
10. _email_field_verification_resend(): this is vulnerable to CSRF exploits. You are performing a state changing write operation here without a confirmation form or checking a security token. See http://epiqo.com/en/all-your-pants-are-danger-csrf-explained . And please don't remove the security tag, we keep that for statistics and to show examples of security problems.

Removing review bonus tag, you can add it again if you have done another 3 reviews of other projects.

manual review:

1. why did you replace single quotes with double quotes in your t() strings? Single quotes are a tiny bit faster since PHP does not have to search for inline variables.
2. to split up your module file you can use the "file" property in hook_menu() to specify an include file that contains the page callback or form.
3. 'Sucessfully sent verification link.': all user facing text must run through t() for translation ;-)
4. _email_field_verification_resend(): does the token verification work? Shouldn't you call drupal_get_token with the same $value as drupal_valid_token()?
5. _email_field_verification_resend(): instead of all the empty() checks in the beginning you could just specify the route as email/verification/resend/ajax/%/%/%/%/%/% or similar in hook_menu()? Then you can be sure that the arguments are always there, otherwise Drupal core will return a 404 for you.

But otherwise looks RTBC to me.

Assigning to mitchell as he might have time to take a final look at this.

Sorry, can't help here. Good luck, all.

Somehow this fell of my radar, sorry. No objections for more than a week, so ...

Thanks for your contribution, ChristianAdamski!

I updated your account so you can promote this to a full project and also create new projects as either a sandbox or a "full" project.

Here are some recommended readings to help with excellent maintainership:

• Dries' post on Responsible maintainers
• Best practices for creating and maintaining projects
• Maintaining a drupal.org project with Git
• Commit messages - providing history and credit
• Release naming conventions.
• Helping maintainers in the issue queues

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Thanks to the dedicated reviewer(s) as well