mailgun / mailgun-php

Mailgun's Official SDK for PHP
http://www.mailgun.com
MIT License
1.1k stars 314 forks source link

Create Email Validation API #191

Closed pirogoeth closed 5 years ago

pirogoeth commented 8 years ago

Includes request dispatcher and any request//response models necessary.

DavidGarciaCat commented 8 years ago

If you're going to use this issue to validate email addresses, please consider to use this project https://github.com/egulias/EmailValidator instead of build a completely new validator. This one is a well established package and offers several options to validate email addresses.

Nyholm commented 8 years ago

There is a API endpoint for that: https://documentation.mailgun.com/api-email-validation.html

(Even though I believe that egulias email validator should be used all the time.. ) =)

DavidGarciaCat commented 8 years ago

Yeah, I know about that endpoint. The reason to use the egulias email validator is just to prevent unnecessary API calls (in the end it means a more healthy service on both sides: local code can return an invalid email address what it means we won't need to send an API request and we will save time, and the API won't get an email validation request that we already know that will fail).

shakaran commented 7 years ago

I am curious from this issue, the api email validation offered by Mailgun has some improvement or difference about the egulias library? If not I think that should be deprecated and removed for 3.0. Other option is add as dependency and overwrite the api email validation using the library (to still provide the "feature" inside the mailgun PHP-SDK

DavidGarciaCat commented 7 years ago

Hey @shakaran

This is a pending update, and I will work on it as soon as I can.

My idea is to provide a 1st check via egulias library, and if all is fine then send the request to Mailgun API, due if there's a bad formed email address then doesn't make sense to sent the request to the API validator.

I hope to have some free time the upcoming weeks, and build the updated code.

Cheers,

DavidGarciaCat commented 7 years ago

@Nyholm or @pirogoeth feel free to assign me the issue if you want

shakaran commented 7 years ago

@DavidGarciaCat LGTM as good solution and save queries to the API too (win-win) to both sides

DavidGarciaCat commented 6 years ago

Hey @Nyholm

I have started to work on the Email Validator feature, but I would like to get your POV on it.

The API docs confirms that the Email Validation endpoints have their own Public / Private keys to send the HTTP requests to the API, so we cannot reuse the same API Key we have for the other HTTP calls due it will throw an error due a wrong API Key.

1st question

Should the new Mailgun\Api\EmailValidation file extend from HttpApi in the same way we have for the other endpoints, although we won't use the same HTTP validation for these requests? (if yes, then please refer to 2nd question)

Or should we add a new process to manage these API calls to validate email addresses?

2nd question

In case we could reuse the same HTTP Client to make the calls, do you think is a good idea if we add a new parameter to the HttpApi::httpGet() method, to define if we deal with an Email Validation request, so we can change the behaviour for the HTTP Call validation? Something like this:

// Current code
protected function httpGet($path, array $parameters = [], array $requestHeaders = [])

// Potential update
protected function httpGet(
    $path,
    array $parameters = [],
    array $requestHeaders = [],
    $isEmailValidation = false
)

So in this way, by default, we won't need to update the current code, and for the Email Validation calls we will just need to pass one more argument to send the API Key in the expected way.

Thoughts?

3nd question

Do you prefer to pass the public / private key as an argument of these methods, or do you want to set them up as part of the constructor when we instantiate the Mailgun client?

Personally I guess the Mailgun client constructor can manage them in a more efficient way (reusability of the keys) however given the HTTP calls require their own validation via api_key query string then there's no sense to use the same process to make these calls.

I am looking forward to hear from you.

Cheers,

Nyholm commented 6 years ago

Thank you. I've seen your implementation.

Should the new Mailgun\Api\EmailValidation file extend from HttpApi in the same way we have for the other endpoints, although we won't use the same HTTP validation for these requests?

Why wouldn't we use the HTTP endpoint for validation?

I know that local validation is preferred, but library is an API client. It would be expected that we actually use the API.

DavidGarciaCat commented 6 years ago

🤔

On a 2nd reading, I am not 100% sure if I misunderstood the api_key query string argument.

I'll double check and will proceed

DavidGarciaCat commented 6 years ago

I know that local validation is preferred, but library is an API client. It would be expected that we actually use the API.

No, this is not what I mean, is about the API validation. As I said, will double check

carlwiedemann commented 6 years ago

It would be useful to have a configuration option to allow the egulias dependency to be optional as it requires --enable-intl. Thanks.

DavidGarciaCat commented 6 years ago

Hello @c4rl

Thanks for pointing out the PHP INTL extension on egulias/EmailValidator, however if you check the composer.json file then you'll see that this PHP Extension is flagged as suggested but not as required:

https://github.com/egulias/EmailValidator/blob/master/composer.json#L22-L33

As an external vendor, we must assume that the source code provided must have all expected checks to make sure the PHP INTL extension is installed and enabled, in order to prevent unexpected Exceptions.

If you are concerned about this, feel free to review the egulias/EmailValidator's source code, and if there are no references or checks for the PHP Extension then you can open an issue there.

Hope this helps.

Cheers,

DavidGarciaCat commented 6 years ago

Hey @c4rl

I guess this is covering the case that you pointed out:

https://github.com/egulias/EmailValidator/blob/master/EmailValidator/Validation/SpoofCheckValidation.php#L19-L21

So what I can do is to add a try ... catch ... to make sure we don't throw an exception in case your environment doesn't have the PHP INTL Extension.

Hope this makes sense.

Cheers,

VishvajitP commented 6 years ago

Why mailbox validation api gives different response for the same email id ? Response 1:

{u'parts': {u'local_part': u'xxx', u'domain': u'xxx', u'display_name': None}, u'is_role_address': False, u'reason': None, u'is_valid': True, u'address': u'xxx@xxx.xxx', u'is_disposable_address': False, u'did_you_mean': None, u'mailbox_verification': u'true'} 

Response 2:

{"address": "xxx@xxx.xxx", "did_you_mean": null, "is_disposable_address": false, "is_role_address": false, "is_valid": true, "mailbox_verification": null, "parts": {"display_name": null, "domain": "xxxx.xxx", "local_part": "xxx"}, "reason": null}
DavidGarciaCat commented 6 years ago

Hey @VishvajitP

Unless you provide an example for both calls, it's impossible to analyse and reproduce the error.

Based on your 2 responses looks like:

...so I can just speculate about a potential mistake running those calls, resulting on an unexpected output.

Please note the new feature to validate emails is still WIP - part of v3 Roadmap - and it will be implemented as soon as possible.

Best,