Provide out-of-the-box code for Webhook notification

sylfabre commented 2 years ago

Hello

Your code is nice but it misses a factory to parse a Symfony\Component\HttpFoundation\Request (for instance, or a more generic interface) into a Webhook message DTO (one dedicated DTO per possible webhook).

Would you accept a PR for this?

DavidGarciaCat commented 2 years ago

Hello,

This SDK uses the HTTPlug integration and, thus, the PSR Request Interface.

The Mailgun PHP SDK is an abstract library. Hence, having a specific Symfony integration is not the right approach here. There are, however, other vendors out there that use this SDK and they have specific Symfony integrations.

I hope it helps,

sylfabre commented 2 years ago

Hello @DavidGarciaCat

I'm sorry, my first message was missing a word thus it wasn't very clear.

My point is not about Symfony integration and the HttpFoundation vs PSR Request interfaces debate, and I understand that your lib uses the PSR one: I'm fine with it.

My point is that your lib misses a factory to parse the webhook request sent from Mailgun to your customers' apps into a PHP Dto representing the webhook payload so it can be used in user-land code.

Something like

<?PHP
$webhookPayload = $mg->parse($request);
echo $webPayload->getTimestamp();

Have a nice day!

DavidGarciaCat commented 2 years ago

Hello @sylfabre

I am trying to say that the $request variable you are aiming to use is an instance of Symfony\Component\HttpFoundation\Request, which means it is attached to the Symfony Framework (or at least to the component that provides this class). And this is something this SDK should not do, as the SDK is a way to represent and map the official API that Mailgun provides.

If you are eager to implement your solution (and I am sure it might be helpful to some other developers too), then the best approach would be to create a new repo/project that can be an extension of this SDK, so you can require your vendor but still use all the features included in the official PHP SDK.

I hope I made myself clearer this time, as I might have caused some confusion too with my previous answer.

Enjoy the rest of your day!

sylfabre commented 2 years ago

@DavidGarciaCat The fact that Symfony uses Symfony\Component\HttpFoundation\Request and your SDK uses Psr\Http\Message\RequestInterface is not an issue as there are converters out there to go from one type to the other

And I agree with you that your SDK should not be attached to the Symfony Framework in any way. So let's stop discussing about vendors :)

My point is that your SDK misses something to parse an object implementing Psr\Http\Message\RequestInterface.

Let's consider this use-case:

I'm a Mailgun customer
I'm using a PHP framework
I'm using Mailgun PHP SDK
I've set up some webhooks with Mailgun to shoot requests to https://myapp.com/webhook/mailgun
I have a controller defined to handle this route /webhook/mailgun
As any PHP framework, my framework instantiates a $request object to be consumed by my controller

=> Right now, I have to write code in my app to parse the request's payload into a PHP DTO to do whatever my apps does when it receives a webhook

User story: As a developer Giving I have a request object implementing Psr\Http\Message\RequestInterface I would expect the SDK to parse for me the request object

DavidGarciaCat commented 2 years ago

Hi @sylfabre

Sorry, I think I didn't make myself clear. Please allow me to rephrase:

By definition, a Webhook is a way to extend a system's behaviour by running an HTTP Request from an origin System (in this case Mailgun API) and targeting another destination System (in this case, your project's webhook URL). The way your system manages the Webhook is, actually, up to you: it can be the same HTTP Request (via Controller), or you can do anything you need and handle it through your CLI (any Command).

From the Mailgun API point of view, you must give them acknowledge of their Callback ASAP. It means you should "capture" whatever you need and put it "somewhere else" (like a queue system based on the Symfony Messenger component) so you can return a 2xx Status Code straight away.

Given that queued actions don't have HTTP Requests (because your workers will run through the CLI), it means it's impossible to handle a non-HTTP Request from the terminal (unless you decide to fake it or capture the whole object into your Message).

Either way, this SDK cannot assume everyone uses Symfony (or any other Framework) because each one handles Request objects from Controllers using different objects, based on each Framework requirements. Examples:

CakePHP uses use Cake\Http\ServerRequest;
Laravel uses use Illuminate\Http\Request;
Symfony uses use Symfony\Component\HttpFoundation\Request;
Yii uses use yii\web\Request;
any other framework here uses any other use statement here

Thus, "we" (either the maintainers or the community developers) cannot even (and ever) consider this approach because each Framework will use its Request object, and we could end up adding some "non-sense code" like this:

class Webhook
{
    public function parse ($request)
    {
        // Please note we cannot even type-hint the Request object
        if ($request instanceof Cake\Http\ServerRequest) {
            // CakePHP logic goes here
        } elseif ($request instanceof Illuminate\Http\Request) {
            // Laravel logic goes here
        } elseif ($request instanceof Symfony\Component\HttpFoundation\Request) {
            // Symfony logic goes here
        } elseif ($request instanceof yii\web\Request) {
            // Yii logic goes here
        } elseif ($request instanceof <Another\Full\Qualified\Class\Name>) {
            // Another Framework logic goes here
            // ...and this list will **always** be outdated because each new logic will "demand" new additions here
        }
    }
}

Given this repository is an SDK, the code "maps the official API" but "does not provide further integrations". Framework-based code must be part of another library (another Composer dependency). You can add the required code to your project (or create your own Composer library). Something similar to this:

// src/Controller/MailgunWekbookController.php

use App\Infrastructure\Mailgun\WekhookParser;
use Symfony\Component\HttpFoundation\Request;
use Symfony\Component\HttpFoundation\Response;

class MailgunWekbookController
{
    public function yourAwesomeWebhookAction(WekhookParser $webhookParser, Request $request): Response
    {
        $whateverYouNeed = $webhookParser->parse($request);

        // Run here further logic
    }
}

// src/Infrastructure/Mailgun/WekhookParser.php

use Symfony\Component\HttpFoundation\Request;

class WekhookParser
{
    public function parse(Request $request): ????
    {
        // Add your logic here, in the same way you would expect it based on your previous comment:
        // See: https://github.com/mailgun/mailgun-php/issues/811#issuecomment-1025600650
    }
}

In any case, each project is unique and has its requirements and dependencies, so this is just an example. Still, the CLI approach (using the Messenger component) makes much more sense, and I would follow this approach. But this is a personal preference.

I hope I made myself clear about why we should not consider this approach.

Warm regards,

sylfabre commented 2 years ago

@DavidGarciaCat thank you for your time and this very precise answer.

Please please stop debate about vendors in your responses as I'm not asking for something to support all PHP frameworks out there, or attached to a given one. You're right, each framework has its own way of handling HTTP requests. And Google shows that it exists adapters to convert back and forth between frameworks-specific requests and PSR-7 requests. So this discussion about vendors is really off-topic here.

Your point about answering ASAP a 200 response to Mailgun server is a good point. And yes using a queue (like Symfony Messenger) is the best way to respond to webhook requests. I think the easiest way is to queue the whole requests' JSON payload.

Anyway, at some point, this payload must be processed by your customers' apps. Parsing this payload requires Mailgun internals knowledge: it is not project-specific.

In my opinion a good SDK should provide whatever tool to ease the integration including parsing a webhook request payload to a strictly typed DTO in a way you see fit to your needs :)

DavidGarciaCat commented 2 years ago

If I understood your point correctly, then what you are trying to say is build the object from a JSON Payload. If this is the case, then the solution is getting the ID of the webhook itself and then calling the method we have to get the latest, updated data:

https://github.com/mailgun/mailgun-php/blob/master/src/Api/Webhook.php#L79-L86

By passing both the domain and webbook id there, you will get the object you want and will make sure the data is up to date.

sylfabre commented 2 years ago

Thank you @DavidGarciaCat 👍

mailgun / mailgun-php

Provide out-of-the-box code for Webhook notification #811