Skip to content

[Webhook] decouple the Webhook component from the Serializer component #57881

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.

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

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

xabbuh
Copy link
Member

@xabbuh xabbuh commented Jul 30, 2024

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

Copy link
Member

@fabpot fabpot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Out of curiosity, why use encoder instead of serializer?

@xabbuh
Copy link
Member Author

xabbuh commented Aug 8, 2024

@fabpot I didn't want to confuse readers of the code by using the term serializer for two different things (the Serializer component and the part that I named encoder here). But I am open for other names.

@fabpot
Copy link
Member

fabpot commented Aug 13, 2024

Serialization and encoding are two different concepts. Having SerializerInterface|PayloadEncoderInterface looks weird to me. I would use PayloadSerializerInterface instead.

@xabbuh
Copy link
Member Author

xabbuh commented Aug 13, 2024

@fabpot I renamed the new classes

@fabpot
Copy link
Member

fabpot commented Aug 14, 2024

Thank you @xabbuh.

@fabpot fabpot merged commit 092cf5f into symfony:7.2 Aug 14, 2024
9 of 10 checks passed
@xabbuh xabbuh deleted the issue-57567 branch August 14, 2024 19:14
@@ -548,7 +548,7 @@ public function load(array $configs, ContainerBuilder $container): void
$this->registerProfilerConfiguration($config['profiler'], $container, $loader);

if ($this->readConfigEnabled('webhook', $container, $config['webhook'])) {
$this->registerWebhookConfiguration($config['webhook'], $container, $loader);
$this->registerWebhookConfiguration($config['webhook'], $container, $loader, $this->readConfigEnabled('serializer', $container, $config['serializer']));

// If Webhook is installed but the HttpClient or Serializer components are not available, we should throw an error
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

outdated comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fabpot added a commit that referenced this pull request Aug 17, 2024
This PR was merged into the 7.2 branch.

Discussion
----------

[FrameworkBundle][Webhook] Fix comment

| Q             | A
| ------------- | ---
| Branch?       | 7.2
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #57881 (comment)
| License       | MIT

Follows
* #57881

Commits
-------

b3bd0c6 [FrameworkBundle] Fix comment
@fabpot fabpot mentioned this pull request Oct 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Webhook] SerializerInterface is not actually needed
10 participants