8000 [Webhook] SerializerInterface is not actually needed · Issue #57567 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[Webhook] SerializerInterface is not actually needed #57567

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
jenky opened this issue Jun 28, 2024 · 6 comments · Fixed by #57881
Closed

[Webhook] SerializerInterface is not actually needed #57567

jenky opened this issue Jun 28, 2024 · 6 comments · Fixed by #57881
Labels
Feature Status: Needs Review Webhook

Comments

@jenky
Copy link
jenky commented Jun 28, 2024 •
edited
Loading

Symfony version(s) affected

7.x

Description

You cannot use the "webhook transport" service since the Serializer component is not installed. Try running "composer require symfony/serializer-pack".

A quick search shows that this line is the only place where the serializer is used in the webhook component.

https://github.com/symfony/webhook/blob/67762b59de4b9e3c6918c44a345f59030923b82d/Server/JsonBodyConfigurator.php#L24

To be fair I don't think this is a bug, however serializer is not really needed to encode the event payload since it's always array, json_encode is fine.

How to reproduce

Create a new SendWebhookMessage then dispatch it without installing the serializer

Possible Solution

Remove SerializerInterface and replace

$body = $this->serializer->serialize($event->getPayload(), 'json');

with

$body = \json_encode($event->getPayload());

Additional Context

No response
@jenky jenky added the Bug label Jun 28, 2024
@n0rbyt3
Copy link
Contributor
n0rbyt3 commented Jun 28, 2024

symfony/webhook requires symfony/remote-event which requires symfony/messenger which finally requires symfony/serializer.

symfony/messenger provides custom SerializerInterface implementations and uses the component to encode and decode Envelope instances if any serializer is set.

However, I don't see any usages of symfony/messenger inside symfony/remote-event. This needs to be checked and may can be removed, so symfony/webhook can add symfony/serializer as optional dependency then.

@jenky
Copy link
Author
jenky commented Jun 28, 2024

symfony/messenger does not require symfony/serializer as far as I know. Otherwise symfony/serializer should be installed when I install symfony/webhook

@n0rbyt3
Copy link
Contributor
n0rbyt3 commented Jun 28, 2024

Well, it does: https://github.com/symfony/messenger/blob/7.1/composer.json#L33

@jenky
Copy link
Author
jenky commented Jun 28, 2024 •
edited
Loading

It's a dev dependency, which is not installed in my case as expected. Messenger uses PhpSerializer by default if you don't specify custom serializer which doesn't need symfony/serializer to encode the message.

Anyway you missed my point, you don't need the serializer component to turn an array to JSON string. If you do really need it then make serializer optional in JsonBodyConfigurator

public function __construct(
    private readonly ?SerializerInterface $serializer = null,
) {
}

// 

$body = $this->serializer?->serialize($event->getPayload(), 'json') ?? \json_encode($event->getPayload());

@n0rbyt3
Copy link
Contributor
n0rbyt3 commented Jun 28, 2024

Sorry, I missed it's the dev dependency block. You're right. Since it's optional, SerializerInterface should be nullable and json_encode should be used as fallback.

@xabbuh xabbuh added Feature and removed Bug labels Jul 21, 2024
@xabbuh xabbuh mentioned this issue Jul 30, 2024
Merged
@xabbuh
Copy link
Member
xabbuh commented Jul 30, 2024

see #57881

@fabpot fabpot closed this as completed Aug 14, 2024
fabpot added a commit that referenced this issue Aug 14, 2024
@fabpot
feature #57881 [Webhook] decouple the Webhook component from the Seri…
092cf5f 
…alizer component (xabbuh)

This PR was merged into the 7.2 branch.

Discussion
----------

[Webhook] decouple the Webhook component from the Serializer component

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | no
| New feature?  | yes
| Deprecations? | no
| Issues        | Fix #57567
| License       | MIT

Commits
-------

ef32a22 decouple the Webhook component from the Serializer component
@alamirault alamirault mentioned this issue Mar 18, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
Feature Status: Needs Review Webhook
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Webhook] decouple the Webhook component from the Serializer component xabbuh/symfony
5 participants
@fabpot @jenky @xabbuh @n0rbyt3 @carsonbot
0