Like this module and the integrated service, it will certainly serve very valuable. This could (and should?) be enhanced so that the external validation (which may also be charged for) will only be performed if no other mechanism had invalidated the form before. What I have in mind are modules like Mollom, Honeypot and any kind of Captcha based tools. Because, if any of them refuses e.g. user registration, then there is no requirement to validate the email address.
This could be implemented by doing the following:
What do you think? If this is something you wanted to include into the module I could probably provide a patch that does that.
Having said that, I'd suggest one other change to the code of the module as well: make it comply with the Drupal coding standards. This will only be a minor and cosmetic change but it will help other contributors to participate.
Comments
Comment #1
vensiresThank you very much for your detailed opinion!
Not wasting requests to the API if something else has invalidated the form was a necessity; you were right! In 7.x-1.2 it has been partially fixed. I haven't yet completely tested your first point though (@see Set the priority of this module to a pretty high value so that it gets to the very end of the module chain, i.e. it will be executed last.) so I am leaving this issue active for a little while until I also test and implement this part too.
If you want to help me on this, let me tell you that I read the code of all modules listed in the Captcha module page and not one sets a different weight in its install file. As a result, I suspect that a weight of 1 would be sufficient but I will have to further test it since I think that #element_validate callbacks are executed before any #validate callbacks... Any help would be appreciated on this :)
PS: Also used the Coder module to make sure the module code follows the Drupal Coding Standards so don't you have to worry about this either ;)
Comment #2
jurgenhaasThanks for your feedback, that sounds really good. I guess you should just be fine with weight = 1 if all the others are not changing the priority at all. If you want somebody for testing all this, please let me know.
Regarding the coding standard I beg your pardon as I was mislead by just one issue where your code doesn't follow the standard and I didn't look at the rest of it. I can confirm, that everything is fine except the missing space between "if" or "elseif" and the following "(". In the same context, "elseif" and "else" should be at a new line and not following the "}" directly. See https://drupal.org/coding-standards#controlstruct
This catched my attention because my IDE (PhpStorm) highlighted a coding error in email_validator_validate_email() where in line 179 the variable $data is undefined and in fact, that's true because $data gets only defined when $request->code = 200 and then the program flow never gets to that watchdog. I guess you would probably want to fix that too.
Comment #3
jurgenhaasMay I ask about a possible time frame for the development of the discussed topic? Just because we're planning to integrate this module into our server farm but we won't be able to unless this has been done.
Comment #4
vensiresI just yesterday tried to find out what could be done with this issue. As I found out - and it was common sense - #element_validate callbacks are executed before #validate callbacks. #element_validate is only used in Email field implementation though. Setting a weight of 1 will be sufficient but I will still have to turn #element_validate to #validate. I suppose that I'll have it fixed by the end of next week.
Comment #5
jurgenhaasAny news on this?
Comment #6
vensiresSorry for the big delay. Well, I have no good news. Changing #element_validate to #validate callbacks isn't easy at all and I don't seem to find a clean and mostly quick way for properly handling the form elements with a #tree property set to TRUE. Try thinking of how the
_email_validator_webform_client_form_alter()function should be altered in order to support the #tree property. If you have advanced on this more than me, then please make me a patch and I'll add it to the next version of the module.Another thing that troubles me is that you asked for this module to be executed after honeypot etc. I wonder if I should do anything at all actually about it since someone else might ask for this module to be executed before honeypot etc. I don't think I should actually take a default stance but better leave it as is. I'm not yet totally sure though... Any thoughts?
Comment #7
jurgenhaasYou can simply use form_error($element, $message) instead of form_set_error() which handles all the cases, whether #tree option is used or not. In your validation function you can then just call form_get_errors() if if the result is not empty, then there have been errors in the form before, so the form won't be submitted anyway and you don't have to validate the email address externally.
Now, why should external email validation happen last? Well, if other modules identified other errors before already, there is no need for an external validation process that (1) takes time and (2) costs money. Both can be avoided and I'm sure users would appreciate that. If you were worried about that, you could still offer a checkbox in the settings where admins can turn that option off, so that email addresses get validated regardless of previous errors.
Comment #8
jurgenhaasAre you still working on this?
Comment #9
vensiresI am sorry but it seems I don't have so much time to spend on this. I suspect this will have to wait for the near/far future. Whenever you have a patch to send though, I'll make sure to publish a new release asap.
Comment #11
vensiresIt must have been fixed in version 7.x-1.3. Feel free to reopen if this doesn't work as expected.