Closed (won't fix)
Project:
Drupal.org security advisory coverage applications
Component:
module
Priority:
Minor
Category:
Task
Assigned:
Unassigned
Reporter:
Created:
14 Sep 2012 at 09:37 UTC
Updated:
18 Jul 2013 at 14:40 UTC
For Drupal7
This module is for checking whether user email address has been taken on user register form,by ajax.
Link to sandbox project:
http://drupal.org/sandbox/meadhu/1778580
Git repository:
git clone http://git.drupal.org/sandbox/meadhu/1778580.git email_ajax_check
Comments
Comment #1
cubeinspire commentedGreat module! I've tested it and works as expected.
Only two issues I see.
One is that you still have some code standards problems:
http://ventral.org/pareview/httpgitdrupalorgsandboxmeadhu1778580git
The second one:
There I see a security issue as it takes the email address from $_GET without checking the source.
This could allow an external script exploit to fetch the registered users email addresses (including User1) from the database. I would add a pair key-hash to avoid that problem with a hash code on the configuration panel.
Otherwise great module I will add it to my Drupal Toolbox.
Comment #2
teknic commentedI like it overall, just a few things.
1. Your working out of master, you should have a version specific branch. 7.x-1.x etc.
2. Your module folder is called ajax_check_email but all your files are called email_ajax_check. You should pick one and rename files / functions accordingly.
3. You have a .info file called ajax_check_email.info that probably should just be deleted.
4. _email_ajax_check_is_user_exists uses one way of specifying the SQL query, then the next function (email_is_blocked) uses a different way of specifying the SQL query. How come? I think its better to standardize and pick which way you want to go.
5. You should add "Implementation of hook_form_alter()" comment to the form alter in the .module file
Comment #3
ANDiTKO commentedHello deapge and thanks for your work.
I tested your module on my local server and it works well. I also tried it with the module LoginToboggan and it seems to be working as expected.
However, when a user disables java script your module dose not work. But thats fine because it dose not break the registration functionality. Im just pointing that out so you can maybe figure something out about the users that have disabled java script or don't support it (like mobile devices).
I am changing the module status to "Needs work" so i can save the other reviewers some time.
Please work on the suggestion that the other reviewers reported and when you are done change the status back to "needs review".
Good luck!
Comment #4
deapge commentedThanks for your reply. I will fix these issue on next version.
Comment #5
deapge commentedHi,all
Please review this module,on previous version I fixed some issues.
Comment #6
fr3shw3b commentedHello,
Good work.
I tested it and it works perfectly,
I've seen no other issues apart from a few minor things in the info file.
The packaging information at the end isn't needed,
Do you need the dependency user stated?
User is a part of Drupal core and you need it to be able to use Drupal.
One other thing for future reference you don't have to add inverted commars
to the name and description in the info file, obviously it doesn't make a big difference
but I thought I'd let you know.
Thanks,
Comment #7
deapge commentedHi,fr3shw3b
Thanks for your advice.
fixed.
Comment #8
klausiWe are currently quite busy with all the project applications and I can only review projects with a review bonus. Please help me reviewing and I'll take a look at your project right away :-)
Comment #9
deapge commentedOK. Let me look at review bonus.
Comment #9.0
deapge commentedChange git clone
Comment #10
fr3shw3b commentedHello,
My first thought is that you need a .install file to implement the hook_uninstall() function to use variable_del to remove the variable created in the settings form.
You need to use comments in the JavaScript file, firstly to explain what the file does and then to help explain the code activity within.
I would of thought it would be better to use Drupal's variables set and get methods as oppose to PHP $_GET or you could use the drupal static cache as it is called during the input to store the email address and get it for the email_ajax_check_callback.
the $mail variable in email_ajax_check.unique.inc doesn't need to be checkplained twice, once is enough as the variable has not been entered between both the checkplains.
Comment #11
mrharolda commentedJust checked out your module: looks promising!
First suggestion: use .change() (or .blur()) instead of .keyup(). Or at least provide an option to disable checking half-typed email addresses ...see next comment ...Comment #12
mrharolda commentedI've added a couple patches for bug reports and feature request in the sandbox's issue queue: http://drupal.org/project/issues/1778580?categories=All
Comment #13
mrharolda commentedI'm also up for (co)maintainership, since I almost rewrote the entire module ;)
Comment #14
PA robot commentedClosing due to lack of activity. Feel free to reopen if you are still working on this application.
I'm a robot and this is an automated message from Project Applications Scraper.
Comment #15
LordOfTheUniverse commentedHi,
I am also using this module. It works fine. But its not working on edit user profile.
Although I am trying to add this on Edit profile page.
If anyone worked on same..let me know.
Thanks,
Comment #15.0
LordOfTheUniverse commentedadd git repository