Skip to content

[Messenger] Uses a messenger serializer, not an individual encoder/decoder #28405

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
Sep 10, 2018

Conversation

sroze
Copy link
Contributor

@sroze sroze commented Sep 8, 2018

Q A
Branch? master
Bug fix? no
New feature? no
BC breaks? yes
Deprecations? no
Tests pass? yes
Fixed tickets ø
License MIT
Doc PR Will come

Makes the component-based even simpler.

Before

        $encoderDecoder = Serializer::create();
        $middleware = [new SendMessageMiddleware(new SenderLocator([
            Message::class => new AmqpTransport($encoderDecoder, $encoderDecoder, $connection),
        ]))];

After

       $middleware = [new SendMessageMiddleware(new SenderLocator([
            Message::class => new AmqpTransport(Serializer::create(), $connection),
        ]))];

@@ -62,7 +62,6 @@

<service id="messenger.transport.amqp.factory" class="Symfony\Component\Messenger\Transport\AmqpExt\AmqpTransportFactory">
<argument type="service" id="messenger.transport.encoder" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This probably need to be changed to be messenger.transport.serializer.

Copy link
Member

Choose a reason for hiding this comment

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

indeed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

/**
* @author Samuel Roze <samuel.roze@gmail.com>
*/
interface SerializerInterface extends DecoderInterface, EncoderInterface
Copy link
Member

Choose a reason for hiding this comment

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

Not sure we need to keep the decoder and encoder interface.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've removed them 👍

@sroze sroze force-pushed the uses-a-messenger-serializer branch 3 times, most recently from 95134f2 to b2ebd79 Compare September 8, 2018 14:36
Copy link
Member

@chalasr chalasr left a comment

Choose a reason for hiding this comment

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

with minor comment


/**
* Encodes an envelope content (message & items) to a common format understandable by transports.
* The encoded array should only contain scalar and arrays.
Copy link
Member

Choose a reason for hiding this comment

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

scalars and arrays

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.

@sroze Can you add an entry in the component CHANGELOG? And perhaps a note in UPGRADE file?

@sroze sroze force-pushed the uses-a-messenger-serializer branch from b2ebd79 to 5b93f5f Compare September 9, 2018 10:28
@sroze
Copy link
Contributor Author

sroze commented Sep 9, 2018

I've added changes on both CHANGELOG (Messenger & FWB) + the UPGRADE-4.2 👍

@fabpot
Copy link
Member

fabpot commented Sep 10, 2018

Thank you @sroze.

@fabpot fabpot merged commit 5b93f5f into symfony:master Sep 10, 2018
fabpot added a commit that referenced this pull request Sep 10, 2018
…dual encoder/decoder (sroze)

This PR was merged into the 4.2-dev branch.

Discussion
----------

[Messenger] Uses a messenger serializer, not an individual encoder/decoder

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | no
| BC breaks?    | yes
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | ø
| License       | MIT
| Doc PR        | Will come

Makes the component-based even simpler.

**Before**
```php
        $encoderDecoder = Serializer::create();
        $middleware = [new SendMessageMiddleware(new SenderLocator([
            Message::class => new AmqpTransport($encoderDecoder, $encoderDecoder, $connection),
        ]))];
```

**After**
```php
       $middleware = [new SendMessageMiddleware(new SenderLocator([
            Message::class => new AmqpTransport(Serializer::create(), $connection),
        ]))];
```

Commits
-------

5b93f5f Uses a messenger serializer, not an individual encoder/decoder
@nicolas-grekas nicolas-grekas modified the milestones: next, 4.2 Nov 1, 2018
This was referenced Nov 3, 2018
nicolas-grekas added a commit that referenced this pull request Dec 1, 2018
…erializer default id (ogizanagi)

This PR was merged into the 4.2 branch.

Discussion
----------

[FrameworkBundle][Messenger] Restore check for messenger serializer default id

| Q             | A
| ------------- | ---
| Branch?       | 4.2 <!-- see below -->
| Bug fix?      | yes
| New feature?  | no <!-- don't forget to update src/**/CHANGELOG.md files -->
| BC breaks?    | no     <!-- see https://symfony.com/bc -->
| Deprecations? | no <!-- don't forget to update UPGRADE-*.md and src/**/CHANGELOG.md files -->
| Tests pass?   | yes    <!-- please add some, will be required by reviewers -->
| Fixed tickets |  https://symfony-devs.slack.com/archives/C9PQ75TV3/p1543590611003500  <!-- #-prefixed issue number(s), if any -->
| License       | MIT
| Doc PR        | N/A

From Slack:

> @adamquaile [4:10 PM]
> So, I just updated to 4.2 today, and got this message:
> > The default Messenger serializer cannot be enabled as the Serializer support is not available. Try enabling it or running "composer require symfony/serializer-pack"
>
> In the docs it's stated:
> > In order to use Symfony's built-in AMQP transport, you will need the Serializer Component. Ensure that it is installed with:
>
>But I haven't yet configured AMQP - I'm using my own transport. Should I be getting this exception?

---
This check was removed in #28405, but is actually still necessary to not fail as soon as you can install the Messenger component without the Serializer one installed.

Commits
-------

1cf17c0 [FrameworkBundle][Messenger] Restore check for messenger serializer default id
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.

5 participants